Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of navigations before/during load #6714

Merged
merged 3 commits into from Jun 23, 2021

Conversation

domenic
Copy link
Member

@domenic domenic commented May 24, 2021

Several types of navigations have a special handling, where if they happen "before load", they are converted into "replace" navigations (instead of "default" navigations). However, this special handling had several interoperability problems and the specification was somewhat broken due to a bad definition of "completely loaded". The result here mostly aligns with Blink behavior.

Notable changes:

  • "completely loaded" gets flipped only after the load event has run. It is mostly used to determine "default" vs. "replace" navigations, and in almost all cases browsers treat navigations before or during the load event as getting the special "replace" treatment. This includes the below cases, as well as <iframe> src="" assignment (which is otherwise untouched). The other thing this improves is that it ensures load events on iframe/embed/object elements are only fired (or at least queued) after the corresponding load event on the contained Document.

  • location.assign() gets special handling before/during the load event; previously this special handling was only applied to the location setters. (This allows us to roll "Location-object-setter navigate" into "Location-object navigate".)

  • Location APIs now override the special handling and force "default" navigation if transient user activation is present, not just if running specifically in a click handler.

  • All form submissions can get converted to "replace" if they target a non-completely loaded document. Previously this special handling was only applied to form submissions initiated by the formElement.submit() API.

Notable non-changes (backed by tests):

  • Following hyperlinks never varies its "replace" vs. "default" behavior based on completely loaded status.

  • window.open() navigating an existing browsing context never varies its "replace" vs. "default" behavior based on completely loaded status.

Closes #6579. Closes #3247.

/cc @natechapin @rakina @domfarolino

(See WHATWG Working Mode: Changes for more details.)


/form-control-infrastructure.html ( diff )
/history.html ( diff )
/links.html ( diff )
/parsing.html ( diff )

Several types of navigations have a special handling, where if they happen "before load", they are converted into "replace" navigations (instead of "default" navigations). However, this special handling had several interoperability problems and the specification was somewhat broken due to a bad definition of "completely loaded". The result here mostly aligns with WebKit and Blink behavior.

Notable changes:

* "completely loaded" gets flipped only after the load event has run. It is mostly used to determine "default" vs. "replace" navigations, and in almost all cases browsers treat navigations before or during the load event as getting the special "replace" treatment. This includes the below cases, as well as <iframe> src="" assignment (which is otherwise untouched). The other thing this improves is that it ensures load events on iframe/embed/object elements are only fired (or at least queued) after the corresponding load event on the contained Document.

* location.assign() gets special handling before/during the load event; previously this special handling was only applied to the location setters. (This allows us to roll "Location-object-setter navigate" into "Location-object navigate".)

* Location APIs now override the special handling and force "default" navigation if transient user activation is present, not just if running specifically in a click handler.

* All form submissions can get converted to "replace" if they target a non-completely loaded document. Previously this special handling was only applied to form submissions initiated by the formElement.submit() API.

Notable non-changes (backed by tests):

* Following hyperlinks never varies its "replace" vs. "default" behavior based on completely loaded status. This matches all browsers

* window.open() navigating an existing browsing context never varies its "replace" vs. "default" behavior based on completely loaded status. This matches Blink and WebKit.

Closes #6579. Closes #3247.
@domenic domenic added normative change topic: navigation interop Implementations are not interoperable with each other labels May 24, 2021
@domenic domenic requested a review from annevk June 3, 2021 21:10
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This largely looks good to me. I'm a bit surprised with the incumbent dependency, but I don't have a better alternative. It also struck me that the Location object should probably be passed as an argument if we ever wanted to refactor this more.

I'll ping @smaug---- to see if he wants to give this a read as well.

@domenic
Copy link
Member Author

domenic commented Jun 16, 2021

I'm a bit surprised with the incumbent dependency

Yeah, unfortunately incumbent seems pretty entangled with the Location object, as it's used for the source browsing context and all the checks that stem from that. So anything else would be inconsistent.

I'll ping @smaug---- to see if he wants to give this a read as well.

That would be much appreciated. As mentioned in the OP, although this specifies Chrome's behavior it doesn't seem to match anyone else (maaaybe Safari but probably at least some of those WPT failures are legit). So getting multi-implementer interest on achieving interop here would be ideal.

Feel free to also review web-platform-tests/wpt#28480 !

@smaug----
Copy link
Collaborator

smaug---- commented Jun 17, 2021

* "completely loaded" gets flipped only after the load event has run

load or pageshow?

oh, I see, the spec has a bit weird setup for load and pageshow.
Those are separate tasks, and 'Completely finish loading' happens before those.
Is that even close to what browsers implement?

Shouldn't the 'replace' special case end only once load and/or pageshow have fired?

@domenic
Copy link
Member Author

domenic commented Jun 17, 2021

load or pageshow?

Good question. I only tested load. Let me expand the tests to also test pageshow. That may change the proposed spec.

oh, I see, the spec has a bit weird setup for load and pageshow.
Those are separate tasks, and 'Completely finish loading' happens before those.
Is that even close to what browsers implement?

The current spec is not close to what browsers implement. The revised spec matches Chromium at least.

Shouldn't the 'replace' special case end only once load and/or pageshow have fired?

Yes. That is what this PR proposes (modulo the pageshow vs. load question; it only keys off of load for now).

@domenic
Copy link
Member Author

domenic commented Jun 17, 2021

My testing reveals that browsers seem to do everything after both load and pageshow. I have updated the PR accordingly.

Also, since by code inspection Chromium fires both events in the same task, and you seemed to imply that Gecko probably does the same, I consolidated them into a single task. The difference isn't really testable since previously browsers were allowed to schedule those tasks right after each other. But now we don't allow potential differences where other tasks interleave.

task source</span> given the <code>Document</code>'s <span>relevant global object</span> to run
these steps:</p>
<li><p>Set the <code>Document</code>'s <span>load timing info</span>'s <span>load event end
time</span> to the <span>current high resolution time</span> given <var>window</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If load and pageshow happen in the same task, should this measure both or only the load event cost? cc @yoavweiss

It seems this could be gamed by just moving the work to pageshow instead.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't worry too much about gaming here, as work can already be rescheduled from onload to run in a consecutive task. Are there tests covering that onload and pageshow are now on the same task and onload runs before pageshow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same-taskness wouldn't be testable, but I realized we can test it with microtasks. I'll tack that onto my tests PR.

I'm pretty sure the ordering is already tested somewhere but I'll test that too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no, it's not testable, I forgot. When the event listeners fire, the stack is empty, so microtasks will run whether it's a single task or multiple tasks scheduled one after the other.

The ordering is already tested in https://wpt.fyi/results/html/syntax/parsing/the-end.html?label=master&label=experimental&aligned&q=the-end .

@domenic domenic merged commit 524485a into main Jun 23, 2021
@domenic domenic deleted the fix-loading-replace-behavior branch June 23, 2021 16:24
domenic added a commit to web-platform-tests/wpt that referenced this pull request Jun 24, 2021
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 27, 2021
…estonly

Automatic update from web-platform-tests
Test replace vs. push before/during load and pageshow

Follows whatwg/html#6714.
--

wpt-commits: c1bb1fdaa510c14d31ca8307f4778b015a6bb30d
wpt-pr: 28480
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jul 16, 2021
…estonly

Automatic update from web-platform-tests
Test replace vs. push before/during load and pageshow

Follows whatwg/html#6714.
--

wpt-commits: c1bb1fdaa510c14d31ca8307f4778b015a6bb30d
wpt-pr: 28480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other normative change topic: navigation
4 participants