Improve "completely loaded" specification #5797
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
domenic
added
clarification
annevk
reviewed
Aug 17, 2020
Yes, they appear to be separate, at least in the current spec. https://html.spec.whatwg.org/#stop-parsing step 7 fires the load event on the |
annevk
approved these changes
Aug 18, 2020
There was a problem hiding this comment.
I guess this is fine, but as per #5823 (comment) it does seem like more is needed here.
I'm also greatly suspect of behavior differing between browsing context containers (other than things like srcdoc).
Closes #5796. This also makes <meta http-equiv="refresh">'s discussion of "seconds elapsed since document's has completed loaded" more rigorous, by explicitly saving the timestamp in a new field. Additionally, this fixes what is either a spec bug or a serious ambiguity. Previously, <iframe> and friends, when loading the initial about:blank, would fire two load events: one from explicit prose doing so in their sections, and one implicitly because one always gets fired when you set the "completely loaded" flag. It's possible that the spec meant to distinguish "mark the document as completely loaded" from the initial about:blank's "the document is completely loaded immediately", and have only the former trigger the reaction-at-a-distance that fired load events. In that case this change is an editorial clarification only, albeit quite a nice one as by moving the load firing into the initial about:blank creation, we deduplicate spec prose from the iframe/frame, embed, and object sections.
domenic
force-pushed
the
completely-loaded
branch
from
8ca3746
to
c8d3bf8
Compare
5 tasks
bors-servo
added a commit
to servo/servo
that referenced
this pull request
Aug 7, 2021
The "process the iframe attributes" steps shouldn't enqueue the iframe load event steps (anymore) Fixes `HtmlIFrameElement::process_the_iframe_attributes` (which implements [the "process the `iframe` attributes" steps][1]) queueing an element task to execute [the iframe load event steps][2], which causes another `load` event to be fired on the iframe in addition to the one asynchronously generated by [the "create a new nested browsing context" steps][3]. The following diagram shows what happens when a `src`-less `iframe` element is parser-inserted and which step this PR intends to remove. This does not necessarily match Servo's current behavior exactly - Most notably, `T2`'s steps might run late because the "create a nested browsing context" steps' implementation actually [parses][5] a blank HTML document. ```diff T0 (networking task source) | Insert the `iframe` element, causing the following steps to be executed: | | Create a new nested browsing context | | | Create a new browsing context | | | | Completely finish loading the `iframe` element's `Document` | | | | | Queue an element task T2. | | | | Process the `iframe` attributes - | | | Queue an element task T1 - T1 (DOM manipulation task source) - | Execute the `iframe` load event steps - | | Fire `load` for the `iframe` element T2 (DOM manipulation task source) | Execute the `iframe` load event steps | | Fire `load` for the `iframe` element ``` This bug likely originates from [a bug][4] in the specification. [1]: https://html.spec.whatwg.org/multipage/#process-the-iframe-attributes [2]: https://html.spec.whatwg.org/multipage/#iframe-load-event-steps [3]: https://html.spec.whatwg.org/multipage/#creating-a-new-nested-browsing-context [4]: whatwg/html#5797 [5]: https://html.spec.whatwg.org/multipage/#the-end --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24501, #15727, and the `cross-origin-objects-on-new-window.html` part of #26299 --- - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___
bors-servo
added a commit
to servo/servo
that referenced
this pull request
Aug 7, 2021
The "process the iframe attributes" steps shouldn't enqueue the iframe load event steps (anymore) Fixes `HtmlIFrameElement::process_the_iframe_attributes` (which implements [the "process the `iframe` attributes" steps][1]) queueing an element task to execute [the iframe load event steps][2], which causes another `load` event to be fired on the iframe in addition to the one asynchronously generated by [the "create a new nested browsing context" steps][3]. The following timeline shows what happens when a `src`-less `iframe` element is parser-inserted and which step this PR intends to remove. (This does not necessarily match Servo's current behavior exactly - Most notably, `T2`'s steps might run late because the "create a nested browsing context" steps' implementation actually [parses][5] a blank HTML document.) ```diff T0 (networking task source) | Insert the `iframe` element, causing the following steps to be executed: | | Create a new nested browsing context | | | Create a new browsing context | | | | Completely finish loading the `iframe` element's `Document` | | | | | Queue element task T2. | | | | Process the `iframe` attributes - | | | Queue element task T1 - T1 (DOM manipulation task source) - | Execute the `iframe` load event steps - | | Fire `load` for the `iframe` element T2 (DOM manipulation task source) | Execute the `iframe` load event steps | | Fire `load` for the `iframe` element ``` This bug likely originates from [a bug][4] in the specification. [1]: https://html.spec.whatwg.org/multipage/#process-the-iframe-attributes [2]: https://html.spec.whatwg.org/multipage/#iframe-load-event-steps [3]: https://html.spec.whatwg.org/multipage/#creating-a-new-nested-browsing-context [4]: whatwg/html#5797 [5]: https://html.spec.whatwg.org/multipage/#the-end --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24501, #15727, and the `cross-origin-objects-on-new-window.html` part of #26299 --- - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #5796. This also makes
<meta http-equiv="refresh">'s discussionof "seconds elapsed since document's has completed loaded" more
rigorous, by explicitly saving the timestamp in a new field.
Additionally, this fixes what is either a spec bug or a serious
ambiguity. Previously,
<iframe>and friends, when loading the initialabout:blank, would fire two load events: one from explicit prose doing
so in their sections, and one implicitly because one always gets fired
when you set the "completely loaded" flag.
It's possible that the spec meant to distinguish "mark the document as
completely loaded" from the initial about:blank's "the document is
completely loaded immediately", and have only the former trigger the
reaction-at-a-distance that fired load events. In that case this change
is an editorial clarification only, albeit quite a nice one as by
moving the load firing into the initial about:blank creation, we
deduplicate spec prose from the iframe/frame, embed, and object
sections.
I'd like to merge this without having to write up tests and check what all the browsers do, as it's trying to preserve the spirit of the existing specification, with a big clarity increase. It might be a normative change (depending on how you interpret the existing text), but I fear that if we gate this on tests and interop, it'll get stuck, and we'll be left with the current spec---which is either buggy, or very unclear.
/browsers.html ( diff )
/browsing-the-web.html ( diff )
/dom.html ( diff )
/dynamic-markup-insertion.html ( diff )
/form-control-infrastructure.html ( diff )
/history.html ( diff )
/iframe-embed-object.html ( diff )
/index.html ( diff )
/obsolete.html ( diff )
/parsing.html ( diff )
/semantics.html ( diff )
/workers.html ( diff )