Make named window lookup more precise and correct BCG swap logic #10818
Conversation
|
Oh my goodness, what a great early Christmas gift!! @annevk, do you want to take a look? I can help with editorial review and tweaks but I'd like to know how you feel about the approach to normative algorithm and strategy. @kjmcnee, do you think there are web platform tests we could write for this, despite the remaining implementation-defined behavior? |
There was a problem hiding this comment.
Wow, thanks for improving this! Overall this seems solid. I wouldn't mind us being a bit more opinionated here and there to guide implementations in a certain direction, but that can also be follow-up.
@farre can hopefully also make some time to review this.
In "the rules for choosing a navigable," the method to find an existing navigable by name is vague. This updates the definition to accurately reflect what the major implementations do. There are some differences between implementations, so there remains in the spec some optional/implementation-defined behaviour, but it's much narrower. In particular, note that lookups are now explicitly scoped to browsing context groups. The previous language in the named lookup about "the user agent determines that the two browsing contexts are related enough" is now no longer a part of the lookup logic, but a consequence of the BCG swap decisions. In "obtain a browsing context to use for a navigation response," the existing spec only mentions COOP enforcement as a reason to do a browsing context group swap. Some implementations perform a swap for additional security and performance reasons. This is now reflected in the spec.
Yeah, there are some common behaviours that can be tested. I've sent you a CL here: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 |
|
Scheduling note: I'll be OOO from the 11th to January. If there are no issues, feel free to merge while I'm out. |
These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c
These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Kevin McNee <[email protected]> Cr-Commit-Position: refs/heads/main@{#1394596}
These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Kevin McNee <[email protected]> Cr-Commit-Position: refs/heads/main@{#1394596}
These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Kevin McNee <[email protected]> Cr-Commit-Position: refs/heads/main@{#1394596}
There was a problem hiding this comment.
I did a more detailed review, and this is looking great. I guess we might have missed you, so we'll probably have to finish this ourselves, but if you're able to help, one thing that would be very nice would be a clearer documentation of the implementation-defined cases.
From what I can see we have the following:
- sourceOrigin or destinationOrigin are non-HTTP(S), BCG swap is optional. Force a browsing context group swap when navigating to and from file: URLs #10842
- "browser UI", BCG swap is optional. Part of What kind of new 'session' is created on manual navigation? #6356 although that's broader
- BCG size is 1. The spec has clear notes about this one.
- subtreesToSearch choice is either (top, window) or (top, ..., window). Can you give us more detail on why this is implementation-defined? Like, does Chromium choose differently depending on certain circumstances, and if so, which circumstances? Do you know what would motivate one choice or another?
- Optionally check "allowed by sandboxing to navigate" (in two places). Why is this optional?
- Implementation-defined order for searching through the BCG by name. This is pretty clear from the spec, but in the interest of maybe nailing this down in the future, can you tell us what Chromium picks?
Hi. I'm no longer at my work computer, but I can comment on the implementation differences.
WebKit and Chromium search the requesting window's subtree then search from the top. Firefox iterates and searches from each ancestor. If there's a way to express in the spec that the implementation-defined behavior is fixed for the instance of the user agent, then that'd be appropriate here.
Firefox and Chromium do this. WebKit does not. Similarly, the choice is fixed for the implementation.
I can't remember the details, but I think Chromium and Firefox are based on creation order, or maybe reverse creation order. WebKit's ordering is arbitrary as it iterates over a hash set. |
|
Amazing, thank you so much Kevin. I've filed follow-up issues for all of those. I will now push edits to the spec that take care of source formatting, and point to those follow-up issues. Then @annevk can review. |
This uncovered a couple places where it was missing
We named that parameter "userInvolvementForNavigateEvents" specifically because we left it null in cases where navigate events don't happen. But, there are cases where we need to consult user involvement for the BCG swap. So we need to thread it through more. This is probably better anyways as it's more consistent.
domenic
added
interop
|
This is ready for review. I'd like @annevk or @domfarolino as additional reviewers, if possible, given the changes I made. Almost all of them were related to threading the "user navigation involvement" through more thoroughly. Kevin's initial draft left it as the default of "none" in some cases (e.g. error pages) that implementations might care about, and it reused userInvolvementForNavigateEvents which was often set to null. I'm not sure if the latter cases actually overlap with cases where the UA would care, but it seemed better to fix it up. So a lot of changed lines are just related to that. I'm also going OOO within the next day or two so any help getting this over the finish line is much appreciated. |
| picking a specific ordering, to achieve interoperability.</p> | ||
|
|
||
| <ol> | ||
| <li><p>If <var>currentTopLevelBrowsingContext</var> equals <var>topLevelBrowsingContext</var>, |
There was a problem hiding this comment.
Nit: maybe this should use "is".
… swap limits, a=testonly Automatic update from web-platform-tests Add WPTs for named window lookup and BCG swap limits These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Kevin McNee <[email protected]> Cr-Commit-Position: refs/heads/main@{#1394596} -- wpt-commits: 1f7679ad01691ea3885537b8b19e1b88a0e7f152 wpt-pr: 49631
… swap limits, a=testonly Automatic update from web-platform-tests Add WPTs for named window lookup and BCG swap limits These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Kevin McNee <[email protected]> Cr-Commit-Position: refs/heads/main@{#1394596} -- wpt-commits: 1f7679ad01691ea3885537b8b19e1b88a0e7f152 wpt-pr: 49631
… swap limits, a=testonly Automatic update from web-platform-tests Add WPTs for named window lookup and BCG swap limits These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 Reviewed-by: Domenic Denicola <domenicchromium.org> Commit-Queue: Kevin McNee <mcneechromium.org> Cr-Commit-Position: refs/heads/main{#1394596} -- wpt-commits: 1f7679ad01691ea3885537b8b19e1b88a0e7f152 wpt-pr: 49631 UltraBlame original commit: 7c930d5bca8252fcec21396df3f9ecc6a7ae3e02
… swap limits, a=testonly Automatic update from web-platform-tests Add WPTs for named window lookup and BCG swap limits These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 Reviewed-by: Domenic Denicola <domenicchromium.org> Commit-Queue: Kevin McNee <mcneechromium.org> Cr-Commit-Position: refs/heads/main{#1394596} -- wpt-commits: 1f7679ad01691ea3885537b8b19e1b88a0e7f152 wpt-pr: 49631 UltraBlame original commit: 7c930d5bca8252fcec21396df3f9ecc6a7ae3e02
… swap limits, a=testonly Automatic update from web-platform-tests Add WPTs for named window lookup and BCG swap limits These tests verify the behaviour discussed here: whatwg/html#10818 While there is some implementation defined behaviour, we can add tests for the behaviour that's consistent between the major implementations, namely that proactive browsing context group swaps can't be done while another context exists in the group, that named lookups are scoped to browsing context groups, and the lookup order in the case of multiple contexts with the same name. Bug: None Change-Id: I515f527fb94a711bdd8eeb6610df2127571dcc3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6081814 Reviewed-by: Domenic Denicola <domenicchromium.org> Commit-Queue: Kevin McNee <mcneechromium.org> Cr-Commit-Position: refs/heads/main{#1394596} -- wpt-commits: 1f7679ad01691ea3885537b8b19e1b88a0e7f152 wpt-pr: 49631 UltraBlame original commit: 7c930d5bca8252fcec21396df3f9ecc6a7ae3e02
In "the rules for choosing a navigable," the method to find an existing navigable by name is vague. This updates the definition to accurately reflect what the major implementations do. There are some differences between implementations, so there remains in the spec some optional/implementation-defined behavior, but it's much narrower. In particular, note that lookups are now explicitly scoped to browsing context groups. The previous language in the named lookup about "the user agent determines that the two browsing contexts are related enough" is now no longer a part of the lookup logic, but a consequence of the BCG swap decisions.
In "obtain a browsing context to use for a navigation response," the existing spec only mentions COOP enforcement as a reason to do a browsing context group swap. Some implementations perform a swap for additional security and performance reasons. This is now reflected in the spec.
For context, see:
This closes #313, but we have opened the following issues to track the remaining implementation-defined interop gaps: #6356, #10842, #10848, #10849, #10850.
(See WHATWG Working Mode: Changes for more details.)
/acknowledgements.html ( diff )
/browsers.html ( diff )
/browsing-the-web.html ( diff )
/document-lifecycle.html ( diff )
/document-sequences.html ( diff )