Document does not have target element set before script can access it on load event

[shannonbooth]

What is the issue with the HTML Standard?

This can be seen in the WPT test: https://github.com/web-platform-tests/wpt/blob/master/url/data-uri-fragment.html

From what I can gather from the standard, nothing will update the target element of the document by the time that:

window.addEventListener('load', function() {
    document.getElementById('foo').matches(':target')
}

Is run for the iframe.

On a new document, "Update the document for history step application" invokes "try to scroll to the fragment", which schedules a task which polls for the documents indicated part to be set. By this point, scripts may run for the document. Nothing relevant to this test appears as if it would delay the load event before setting the fragment has had the chance to complete with a <p> element inserted into the document.

There is a definite chance I am misreading / misunderstanding how this is meant to work, but it is actually unclear to me if any section of the spec is guaranteed to end up setting the target fragment at all. It does not seem like "try to scroll to the fragment" could achieve that, as by the time the task runs, the HTML parser may have stopped. Delaying the load event isn't enough, as the parser has stopped by that point from what I understand.

[shannonbooth]

A potential approach fix may be to make an attempt to scroll to the fragment syncronously somewhere before firing the load event. Though I am not sure what timing is best for this.

[domenic]

I believe another fix would be to use the DOM manipulation task source in "Try to scroll to the fragment", to ensure that the task it queues runs before the task that fires the load event.

Would you be able to investigate what browsers do today, e.g. by source inspection?

[shannonbooth]

I have no familiarity with the other browsers code, but just gave it a go!

From grepping around WebKit, it looks like the fragment parsing for this case might be done here? https://github.com/WebKit/WebKit/blob/388bd2fbd3f295c2dc374e76f0a342add2a398c1/Source/WebCore/loader/FrameLoader.cpp#L882

Which I suppose is called from here: https://github.com/WebKit/WebKit/blob/388bd2fbd3f295c2dc374e76f0a342add2a398c1/Source/WebCore/dom/Document.cpp#L7582

Which, if I have traced all of the way back correctly, appears to be called in "the end": https://github.com/WebKit/WebKit/blob/388bd2fbd3f295c2dc374e76f0a342add2a398c1/Source/WebCore/html/parser/HTMLDocumentParser.cpp#L455

From grepping around chromium, it seems that it is also invoked when parsing has finished: https://github.com/chromium/chromium/blob/f6787d7b03b4eecbe9193179c6b5cdd49d9db17c/third_party/blink/renderer/core/loader/frame_loader.cc#L456. From grepping all of the way down, this appears to be what is setting the active element etc.

From grepping around gecko, it appears to also do it after parsing has complete: https://github.com/mozilla/gecko-dev/blob/86c208f86f35d53dc824f18f8e540fe5b0663870/dom/html/nsHTMLContentSink.cpp#L680 From a comment in nsParser.cpp, this function appears to be just before the load event.

[shannonbooth]

I believe another fix would be to use the DOM manipulation task source in "Try to scroll to the fragment", to ensure that the task it queues runs before the task that fires the load event.

Using try to scroll the fragment doesn't sound right to me, since it checks parser state, the parser may have stopped running by that point, so the "stopped parsing" step of that function would need to change:

If document has no parser, or its parser has stopped parsing,

[shannonbooth]

Digging into this further, I've also found that implementations appear to disagree about the timing that the target element is set.

I've modified the mentioned test so that it checks for target to be set in DOMContentLoaded. Firefox still passes the test, Chrome does not. I don't have a WebKit based browser to test this against to see what they do (edit: I suspect they will also fail).

The MR I have open for ladybird is: LadybirdBrowser/ladybird#3258, which makes it also work in DOMContentLoaded by running in "the end" before queuing the task to fire that event, though it's not really the best fix IMO.

[annevk]

Setting it in the same task as DOMContentLoaded is dispatched would seem like a reasonable clarification for the case where we are dealing with a navigation to a new document. I would support trying to get the specification and implementations aligned on that.

[shannonbooth]

Cool! Would next steps here be making a PR against the html spec with that change, as well as a PR for WPT test that tests the DOMContentLoaded behaviour of :target (i.e what gecko appears to be doing?)

Somewhat related to this topic, I was doing some searching around WPT to see other tests for it, I happened to notice this comment which seems relevant to this case: https://github.com/web-platform-tests/wpt/blob/3fb9825ad9ae531239e31ba0ae934afb980c7331/dom/nodes/ParentNode-querySelector-All-xht.xht#L20

[annevk]

Yeah, perhaps you want to wait for @mfreed7 or someone else from Chrome to weigh in, in case they want to suggest a different model, but I suspect it's fine.

[annevk]

Hmm, what should happen when the element with the ID gets inserted after DOMContentLoaded? Or after load, even? I guess not matching is acceptable if that matches implementations, but perhaps they currently account for that somehow due to the rise in JavaScript-driven documents.

[domenic]

Right, that's why the current spec has a model which polls periodically and gives up in an implementation-defined manner (usually when the user starts scrolling).

I think the standard's current model is good in that regard. But the OP points out that due to the timing of our call into that algorithm, it's currently prohibited from having :target match earlier than a certain point. That is worth fixing, if there are indeed some browsers in which it matches earlier.

[shannonbooth]

Even if the implementation makes the implementation defined choices in try to scroll to the fragment of:

• Not waiting at all (ignoring the stated performance reasons)
• Never having any reason to believe that the user is no longer interested in scrolling to the fragment

Scrolling to the fragment may never happen as 'try to scroll the fragment' has the normative requirement of "its parser has stopped parsing,". If I understand correctly, stopping parsing happens before either the load event or DOMContentLoaded events have fired. Not scrolling so early on also doesn't seem like the intent to me.

It's a bit outside of the scope of fixing the WPT test in OP, but I think that's what Anne is referring to above - I probably would need to do some more testing here of how browsers behave.

[domenic]

Ah yes, you're right. The intent of the current spec is to allow retries up until the parser stops possibly inserting more elements. And the OP is pointing out that, because of the implementation-defined condition allowing arbitrary delays, such that even a single delay might end up after parsing stops, then "scroll to a fragment" might never happen.

But if browsers are interoperable in always passing the WPT in the OP, then we can tighten up the implementation-defined condition. Probably by adding a backstop such that we definitely run "scroll to the fragment" somewhere in #the-end.

@annevk's suggestion is to tighten this up to specifically be in step 6 of #the-end, before the current 6.1. That seems reasonable to me, although it sounds like it would only match Gecko for now.

Am I understanding correctly this time?

[annevk]

I think so, but I now also think some of the conditions in "try to scroll to the fragment" might still be applicable. If the end user has started scrolling we'd still not want to scroll.

And we'd need a way to cancel the "try to scroll to the fragment" steps I think to account for this new condition.

Now then there's still the question as to whether implementations adhere to this or if they also sometimes scroll after the parser has stopped. So for testing we probably want:

• Early target in the document, but getting to DOMContentLoaded takes long.
• Late target in the document or document essentially fits in a packet.
• Target is inserted post-parsing.