Unclear what to do with fragments on document.open

[bokand]

I'm hoping someone can clarify intent and correct behavior when loading a document using document.open.

Step 23 of https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-open says to set the URL of the open()ed document to that of the "responsible document" and we do that by setting the URL from the document we're loading. However, if the responsible document has a fragment identifier, that gets set on the document.open(). This is problematic for the empty fragment which tries to scroll the iframe into view (the scroll into view algorithm specified to use for fragment navigation is recursive for frames). So any page loaded with an empty fragment will always try to scroll any document.open()'d iframes into view.

Should the document-open algorithm specify we should clear fragments? I've noticed a frame loaded via document.open in Firefox keeps the fragment on document.URL and document.baseURI but window.location is about:blank. In contrast, Chrome keeps the URL with fragment on window.location (as well as document.{URL,baseURI}. That seems like a discrepancy too but I'm not sure if it's the cause or incidental.

[annevk]

Just changing the URL of a document doesn't cause any scrolling to occur though. Navigation to a fragment only happens as part of the navigate algorithm, which isn't invoked there as far as I can tell.

(There are unfortunately various interoperability issues with document.open(). We need someone with time and willingness to write an extensive set of tests to figure out exactly what we want to do with it.)

[bokand]

I see, thanks. It seems Chrome attempts to scroll a fragment into view as part of loading/parsing rather than as part of navigation.

FWIW, Firefox too scrolls a document.open() frame when the parent has a fragment. The difference is that the scroll-into-view isn't recursive so the main frame doesn't get scrolled. See http://bokand.github.io/document-open-fragment.html#test. Interestingly, navigating anew from the URL bar doesn't cause it to scroll-into-view again but a refresh does.

It seems like the purpose of setting the URL in this case is to preserve origin with the parent. Preserving the fragment seems like it's never what you'd want. Could we specify that we should clear it from the URL before setting? That seems like it'd be a safe and easy fix.

[annevk]

We could do that. Though I'm not familiar enough with this algorithm to think through all the implications.

We'd still have the mismatch with when fragments take effect though.

@smaug---- how familiar are you with document.open()?

[smaug----]

This is very edge case of document.open().
But the behavior Gecko has feels reasonable, as much as using entry script can be reasonable, if the whole url is used.

Dropping the fragment might break some pages which re-write themselves using document.open/write and rely on the scrolling.
I admit the behavior is odd when document.open happens from outside the page being rewritten.

Would it be odd to have different behavior when document.open call is coming from outside the page itself?

(In general, I'd very much would like to see document.open()/write() to disappear. It is rather bad API)

[bokand]

Dropping it on a cross frame document.open sounds fine to me. FYI, I've updated Chrome's implementation to do that: https://crrev.com/2801093002/

[annevk]

FWIW, it seems that Edge has the same behavior. Firefox and Safari however both keep the fragment.

[annevk]

Tests in progress at web-platform-tests/wpt#10817.

[TimothyGu]

I have spent some time investigating this issue, and it seems like the reality is more complicated than it seems here.

The key issue here is that in Blink, Gecko, and WebKit, document.open/write/close() triggers fragment-based scrolling, while in my reading of the specification it really shouldn't. Only because of that, the fragment inherited from the parent frame can cause real issues.

In the specification, scrolling based on the fragment happens through the scroll to the fragment algorithm. This algorithm is then only called in two places: update the session history with the new page and traverse the history. These two algorithms are called in several places (like page load processing model for HTML files, which calls the former), but tracing the call chain always goes back to either the navigate algorithm or the History interface. Now, the algorithm for document.open() does not deal with either of those things, nor document.write() and document.close(). As such, per spec no scrolling should happen through these dynamic markup insertion methods.

---

The problem is that, for Blink and WebKit, the part of update the session history with the new page (which should ordinarily be called from the navigate algorithm by way of page load processing model for HTML files) that runs fragment scrolling is in fact embedded into the parser itself. Here's the relevant spec text from update the session history with the new page:

4. Fragment loop: Spin the event loop for a user-agent-defined amount of time, as desired by the user agent implementer. (This is intended to allow the user agent to optimize the user experience in the face of performance concerns.)
5. If the Document object has no parser, or its parser has stopped parsing, or the user agent has reason to believe the user is no longer interested in scrolling to the fragment, then return.
6. Scroll to the fragment given in the document's URL. If this fails to find an indicated part of the document, then return to the fragment loop step.

In short, the spec calls for the user agent to poll the document for the part of the document indicated by the fragment, while the document is still loading, in order to scroll to it. The polling interval is however user-agent-defined.

Blink and WebKit use the "user-agent-defined amount of time" concept liberally, and does not implement the polling. Instead, they only attempt to scroll just before the parser stops parsing, regardless of if the parser was created by navigation. When document.close() gets called, the HTML parser receives an explicit EOF and prepares to stop parsing, and it is at that time the renderer takes steps to try to scroll, in contrary to the spec saying the scrolling should only be done for navigation.

In fact, if one were to use current WebKit, or a version of Blink that has @bokand's CL above reverted, and tries to execute the CL's regression test but with doc.close() removed, the test would still pass.

Backtrace from Blink when fragment-scrolling is attempted due to document.close()

Line numbers based on cc5a5d79953f1bb39947a3c8c57353958c4f0141, with 783d491b68d1b5833b65accba2e4fd595f09a404 reverted.

#0  blink::LocalFrameView::ProcessUrlFragmentHelper(WTF::String const&, blink::LocalFrameView::UrlFragmentBehavior) (this=0x343d49183658, name=..., behavior=blink::LocalFrameView::kUrlFragmentScroll)
    at ../../third_party/blink/renderer/core/frame/local_frame_view.cc:1530
#1  blink::LocalFrameView::ProcessUrlFragment(blink::KURL const&, blink::LocalFrameView::UrlFragmentBehavior) (this=0x343d49183658, url=..., behavior=blink::LocalFrameView::kUrlFragmentScroll)
    at ../../third_party/blink/renderer/core/frame/local_frame_view.cc:1443
#2  blink::FrameLoader::ProcessFragment(blink::KURL const&, blink::WebFrameLoadType, blink::LoadStartType) (this=0x343d4919b908, url=..., frame_load_type=blink::WebFrameLoadType::kStandard, load_start_type=blink::kNavigationToDifferentDocument)
    at ../../third_party/blink/renderer/core/loader/frame_loader.cc:1291
#3  blink::FrameLoader::FinishedParsing() (this=0x343d4919b908)
    at ../../third_party/blink/renderer/core/loader/frame_loader.cc:411
#4  blink::Document::FinishedParsing() (this=0x3d2e90cb09d8)
    at ../../third_party/blink/renderer/core/dom/document.cc:5861
#5  blink::HTMLConstructionSite::FinishedParsing() (this=0x343d4912cce0)
    at ../../third_party/blink/renderer/core/html/parser/html_construction_site.cc:621
#6  blink::HTMLTreeBuilder::Finished() (this=0x343d4912ccb8)
    at ../../third_party/blink/renderer/core/html/parser/html_tree_builder.cc:2736
#7  blink::HTMLDocumentParser::end() (this=0x343d4912c928)
    at ../../third_party/blink/renderer/core/html/parser/html_document_parser.cc:890
#8  blink::HTMLDocumentParser::AttemptToRunDeferredScriptsAndEnd() (this=0x343d4912c928)
    at ../../third_party/blink/renderer/core/html/parser/html_document_parser.cc:905
#9  blink::HTMLDocumentParser::PrepareToStopParsing() (this=0x343d4912c928)
    at ../../third_party/blink/renderer/core/html/parser/html_document_parser.cc:238
#10 blink::HTMLDocumentParser::AttemptToEnd() (this=0x343d4912c928)
    at ../../third_party/blink/renderer/core/html/parser/html_document_parser.cc:916
#11 blink::HTMLDocumentParser::Finish() (this=0x343d4912c928)
    at ../../third_party/blink/renderer/core/html/parser/html_document_parser.cc:966
#12 blink::Document::close() (this=0x3d2e90cb09d8) at ../../third_party/blink/renderer/core/dom/document.cc:3262
#13 blink::Document::close(blink::ExceptionState&) (this=0x3d2e90cb09d8, exception_state=...)
    at ../../third_party/blink/renderer/core/dom/document.cc:3253
#14 blink::DocumentV8Internal::closeMethod(v8::FunctionCallbackInfo<v8::Value> const&) (info=...)
    at gen/third_party/blink/renderer/bindings/core/v8/v8_document.cc:3960
#15 blink::V8Document::closeMethodCallback(v8::FunctionCallbackInfo<v8::Value> const&) (info=...)
    at gen/third_party/blink/renderer/bindings/core/v8/v8_document.cc:6931
#16 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) (this=0x7ffd1cdc3198, handler=<optimized out>) at ../../v8/src/api-arguments-inl.h:95
#17 v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) (isolate=<optimized out>, function=..., new_target=..., fun_data=..., receiver=..., args=...) at ../../v8/src/builtins/builtins-api.cc:110
#18 v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (args=..., isolate=0x26f7dd3fa020) at ../../v8/src/builtins/builtins-api.cc:140
#19 v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) (args_length=5, args_object=0x7ffd1cdc33f0, isolate=0x26f7dd3fa020) at ../../v8/src/builtins/builtins-api.cc:128

---

Gecko acts slightly differently here. Like Blink and WebKit, it also takes steps to scrolls to the fragment when it shouldn't. However, the timing of scrolling in Gecko is a more faithful implementation of update the session history with the new page, and occurs during the document.write(). If one were to do the same test above (doc-write-to-iframe.html regression test modified to remove doc.close()), scrolling will still occur.

---

Edge is the most different of any browsers here. It strips all fragments when propagating the entry settings object's document's URL when document.open() is called, so scrolling never happens.

---

I am personally unsure of what to do for the original issue. Removing the fragment when executing document.open(), while seemingly a sound idea, feels more of a Band-Aid to the original problem of unexpected scrolling by masking over the fact that user agent internals don't distinguish between navigation-triggered and "vanilla" parsers, than a permanent fix.

/cc @domenic @foolip

[TimothyGu]

After thinking a bit more about this, I've become convinced that doing what Chrome is doing is the best solution to this issue. It's logically reasonable for the fragment to not get propagated to a different document, and it is the easiest way to get around the scrolling bug, which removes the implementation barrier a "proper" solution (detailed in my last comment) would possibly require.