search

whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.19k stars 2.72k forks source link

Behaviour of "reload" navigationType in " inner navigate event firing algorithm" algorithm #10621

Open rwlbuis opened 2 months ago

rwlbuis commented 2 months ago

What is the issue with the HTML Standard?

In https://html.spec.whatwg.org/multipage/nav-history-apis.html#inner-navigate-event-firing-algorithm step 32.7, only navigationType "push" and "replace" are stated to run "URL and history update steps". However in the chromium implementation, it is also run for "reload" navigationType, and some WPT tests assume this behavior in order for them to pass. Shouldn't this be reflected in the spec?

Relevant commit from chromium's navigate_event.cc: " // In the spec, the URL and history update steps are not called for reloads. // In our implementation, we call the corresponding function anyway ."

annevk commented 2 months ago

cc @domenic

domenic commented 2 months ago

You omitted the important part of that comment: 

  // In the spec, the URL and history update steps are not called for reloads.
  // In our implementation, we call the corresponding function anyway, but
  // |type| being a reload type makes it do none of the spec-relevant
  // steps. Instead it does stuff like the loading spinner and use counters.

Can you state which part of the URL and history update steps are run in Chromium, which the WPTs expect, but are not run in the spec? I cannot find any browsing the code myself.

rwlbuis commented 2 months ago

You omitted the important part of that comment:

  // In the spec, the URL and history update steps are not called for reloads.
  // In our implementation, we call the corresponding function anyway, but
  // |type| being a reload type makes it do none of the spec-relevant
  // steps. Instead it does stuff like the loading spinner and use counters.

Can you state which part of the URL and history update steps are run in Chromium, which the WPTs expect, but are not run in the spec? I cannot find any browsing the code myself.

Apologies for the omission, I mainly did it because the comment is a bit long, but it was misleading to leave that out.

Having said that, the comment may be wrong. This is what happens on navigation-methods/return-value/reload-intercept.html (I set a bt in NotifyAboutTheCommittedToEntry):

#1 0x798de3f50a7a base::debug::StackTrace::StackTrace()
#2 0x798dddc11e07 blink::NavigationApiMethodTracker::NotifyAboutTheCommittedToEntry()
#3 0x798dddc13df1 blink::NavigationApi::UpdateForNavigation()
#4 0x798dddb67416 blink::DocumentLoader::UpdateForSameDocumentNavigation()
#5 0x798dddb66db1 blink::DocumentLoader::RunURLAndHistoryUpdateSteps()
#6 0x798dddc0e88a blink::NavigateEvent::CommitNow()
#7 0x798dddc0eab4 blink::NavigateEvent::MaybeCommitImmediately()
#8 0x798dddc17fff blink::NavigationApi::DispatchNavigateEvent()
#9 0x798dddb8fbf2 blink::FrameLoader::StartNavigation()
#10 0x798ddd3da6ce blink::LocalFrame::Navigate()
#11 0x798dddc165ba blink::NavigationApi::PerformNonTraverseNavigation()
#12 0x798dddc16a78 blink::NavigationApi::reload()
#13 0x798dde56fd80 blink::(anonymous namespace)::v8_navigation::ReloadOperationCallback()
#14 0x798dd7ca666e Builtins_CallApiCallbackGeneric

I also debugged that the used frame load type is "reload" in the above scenario. WDYT?