Skip to content

Specify browser UI-initiated navigations better #11250

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Specify browser UI-initiated navigations better #11250

wants to merge 2 commits into from
+106 −40

Conversation

domenic
Copy link
Member

@domenic domenic commented Apr 23, 2025 •
edited by pr-preview bot
Loading

In particular, they have a null sourceDocument. We need to handle that in various places in the specification.

Closes #9133.

In addition to @domfarolino and @noamr, review from @ADKaster or other Ladybird folks who might have run into this would be much appreciated.

  • At least two implementers are interested (and none opposed):
    • This matches all implementations, as far as I can tell.
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: N/A
    • Gecko: N/A
    • WebKit: N/A
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): N/A
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): N/A
  • Corresponding HTML AAM & ARIA in HTML issues & PRs: N/A
  • MDN issue is filed: N/A
  • The top of this comment includes a clear commit message to use.

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

/browsing-the-web.html ( diff )
/infrastructure.html ( diff )

@domenic
Specify browser UI-initiated navigations better
51371a5 
In particular, they have a null sourceDocument. We need to handle that in various places in the specification.

Closes #9133.
@domenic domenic requested review from noamr and domfarolino April 23, 2025 06:13
@ADKaster
Copy link
Contributor

or other Ladybird folks who might have run into this would be much appreciated.

cc @trflynn89 @shannonbooth :)

annevk
annevk approved these changes Apr 23, 2025
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Looks good. This puts some pressure on Fetch getting its null client act together.

ADKaster
ADKaster reviewed Apr 24, 2025
View reviewed changes
ADKaster
ADKaster reviewed Apr 24, 2025
View reviewed changes
@domenic domenic mentioned this pull request Apr 25, 2025
Open
@domenic
Copy link
Member Author

domenic commented Apr 25, 2025

I will hold off on merging this until we have agreement that whatwg/fetch#1820 is the right way to fix the request's window. (For example, we might instead prefer callers to set it.)

@ADKaster ADKaster mentioned this pull request Apr 28, 2025
Open
@ADKaster
Copy link
Contributor

Implementation musings:

Implementing this in LadybirdBrowser/ladybird#4515 caused one of my tests for the state of window.navigation.canGoBack after a BrowserUI initiated navigation to fail -- the current entry index got reset to 0. I think that's... correct? Because our test runner does this:

load(about:blank)
load(test file url)
<wait for test to complete>
load(about:blank)
load(test file url)
<wait for test to complete>

Though navigating to about:blank causing all previous state to go kaput seems odd, it's not like anyone who isn't trying to stress test the browser would do that during normal operation, right? ...

@domenic
Copy link
Member Author

domenic commented Apr 30, 2025

I think that's correct too. In theory, web platform tests are meant to be run as if you've started the browser from scratch for every test. If you want to test a series of documents, you need to use iframes or popup windows.

In practice, most browser/test runner combos do not do this. (This leads to annoying changes like web-platform-tests/wpt@0a5ef44 ; see also web-platform-tests/wpt#33590.) But the closer we can get to that ideal, the better.

domenic added a commit to whatwg/fetch that referenced this pull request Apr 30, 2025
@domenic
Handle request's window when from browser UI
7070b90 
In whatwg/html#11250 we are working to specify browser UI-initiated navigations better. Without this change, it is unclear what to set their request's window to. They will have null clients, so "client" is not appropriate. They will show prompts, so "no-window" is not appropriate. And setting it to whatever's shown currently in the being-navigated window is not correct.

This introduces a new value, "from-browser-ui", which indicates that browser UI is initiating the request, and so any prompts should still be shown, but on a neutral backdrop, not associated with a specific Window object.
@domenic domenic mentioned this pull request Apr 30, 2025
Open
5 tasks
@domenic
Copy link
Member Author

domenic commented Apr 30, 2025

Marking "do not merge yet" until we get whatwg/fetch#1821 landed and revise this to use it.

@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ADKaster ADKaster ADKaster left review comments

@annevk annevk annevk approved these changes

@noamr noamr Awaiting requested review from noamr

@domfarolino domfarolino Awaiting requested review from domfarolino

Assignees
No one assigned
Labels
do not merge yet Pull request must not be merged per rationale in comment topic: navigation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Navigation sourceDocument for browser-initiated navigations
3 participants
@domenic @ADKaster @annevk