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

Snapshot fallback base URL for about:-schemed Documents #9464

Merged
merged 16 commits into from
Jul 13, 2023

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Jun 28, 2023

This PR gives an "about base URL" member to:

  • Document
  • Document state
  • Navigation params

The intention is to capture a document's creator's base URL in https://html.spec.whatwg.org/C#creating-a-new-browsing-context and preserve it to (1) the newly created document itself, and (2) the newly-created document state in both:

For the navigation case, we capture the initiator's base URL in #navigate as initiatorBaseURLSnapshot alongside initiatorOriginSnapshot. We preserve this to the document state created in #navigate when navigating to about:-scheme URLs, alongside the document state's origin. It is later used in #create-navigation-params-by-fetching and the about:srcdoc case for population of the navigation params's about base URL, so it can be transferred to the actual document in https://html.spec.whatwg.org/C#initialise-the-document-object.

Finally, we remove the concept of a browsing context's https://html.spec.whatwg.org/C#creator-base-url algorithm, and in https://html.spec.whatwg.org/C#fallback-base-url we only reference the new concept-document-about-base-URL member which is a snapshot of the creator/initiator base URL.

There are a few instances of manually-constructed navigation params outside of the normal flow:

As of now this PR passes in null for the navigation params's about base URL in these (not stricken-through) cases above, but I'm not sure if that is correct. I'm looking into it and it is perhaps the one thing blocking this PR at this case.

Closes #421. Closes #2883. Closes #3989.

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


/browsing-the-web.html ( diff )
/document-lifecycle.html ( diff )
/document-sequences.html ( diff )
/dom.html ( diff )
/urls-and-fetching.html ( diff )

@domfarolino domfarolino marked this pull request as ready for review June 30, 2023 20:35
@domfarolino domfarolino requested a review from domenic June 30, 2023 20:35
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I suspect http://localhost:8080/#the-javascript:-url-special-case also needs to copy over the about base URL, since that case creates a new document state that replaces the old one, but keeps the URL (which might be something like about:srcdoc or about:blank).

Can you be sure to write tests for that case?

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Jul 4, 2023

Let's be sure to note when merging this that it closes #421. I think this does not close #2883 or #3989 yet though, right? Or does it? Edit: hmm I think it does. Do we have WPTs for the cases mentioned there?

@domfarolino
Copy link
Member Author

domfarolino commented Jul 4, 2023

I suspect http://localhost:8080/#the-javascript:-url-special-case also needs to copy over the about base URL, since that case creates a new document state that replaces the old one, but keeps the URL (which might be something like about:srcdoc or about:blank).

Yeah I think you're right. I made two changes: copied the document state member over manually in the algorithm you linked to, and assigned the navigation params's about base URL to the target navigable's active document's about base URL, since this happens in a separate algorithm. I think that should do the trick.


Thanks for the review. I'm going to make sure tests get written for:

@domenic
Copy link
Member

domenic commented Jul 4, 2023

I'm OOO until Tuesday, but I think James already wrote tests for #3989.

@wjmaclean
Copy link

Are there document.open() + about:srcdoc scenarios we need to test in addition to document-base-url-changes-about-srcdoc-2.https.html ?

@domfarolino
Copy link
Member Author

Nope I don't think so.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with one question. Also note my nit fixes.

source Outdated Show resolved Hide resolved
@domfarolino
Copy link
Member Author

Thanks. OK I think this should be good to go, depending on what our policy for writing WPTs for <object> and <embed> these days (since they are deprecated features that we're tend to not explicitly cater to is my understanding). We can probably write them anyways, but not sure if it is blocking.

@domenic
Copy link
Member

domenic commented Jul 13, 2023

Tests for object and embed would be appreciated but definitely not blocking. There are so many interop and spec problems with them, anything in that area is just best-effort.

@domenic domenic merged commit 787191a into main Jul 13, 2023
@domenic domenic deleted the domfarolino/base-url branch July 13, 2023 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment