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

Notify WebDriver BiDi about "committed" navigations #10910

Merged
merged 9 commits into from
Jan 31, 2025

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Jan 13, 2025

This PR adds integration with the WebDriver BiDi spec to notify automation sessions when a navigation results in a new document being loaded. See w3c/webdriver-bidi#788 and w3c/webdriver-bidi#855.

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


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

Copy link

@sadym-chromium sadym-chromium left a comment

Choose a reason for hiding this comment

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

LGTM

@OrKoN OrKoN force-pushed the orkon/navigation-comitted branch from cfad3b6 to 7641acb Compare January 14, 2025 16:07
@OrKoN OrKoN marked this pull request as ready for review January 15, 2025 11:36
@jgraham
Copy link
Member

jgraham commented Jan 15, 2025

So in 6.4.1 in attempt to populate the history entry's document (covering the case of navigating to a document without a DOM) seems to run a the navigation failed steps (and so emits a navigation failed event), but the document isn't null in that case so we'll still send the navigation committed event in 7.4. Possibly we need to move emitting this event up into step 6?

@OrKoN
Copy link
Contributor Author

OrKoN commented Jan 15, 2025

So in 6.4.1 in attempt to populate the history entry's document (covering the case of navigating to a document without a DOM) seems to run a the navigation failed steps (and so emits a navigation failed event), but the document isn't null in that case so we'll still send the navigation committed event in 7.4. Possibly we need to move emitting this event up into step 6?

Thanks for spotting this. On one hand there is a new document committed by the navigation (that might be relevant for the client), on the other hand the events get confusing (failed, then committed) and IIUC this is about error pages where the only useful action is to reload. I guess moving to 6 would be an option or checking saveExtraDocumentState or document's salvageable state.

@OrKoN OrKoN closed this Jan 16, 2025
@OrKoN OrKoN reopened this Jan 16, 2025
@OrKoN OrKoN force-pushed the orkon/navigation-comitted branch 3 times, most recently from 47c48ab to 5ac7342 Compare January 16, 2025 08:43
@OrKoN OrKoN force-pushed the orkon/navigation-comitted branch from 5ac7342 to 80caa5a Compare January 16, 2025 08:44
@OrKoN
Copy link
Contributor Author

OrKoN commented Jan 16, 2025

Moved to step 6.

@OrKoN
Copy link
Contributor Author

OrKoN commented Jan 21, 2025

@domenic could you please take a look when you have a chance?

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'm a little worried about this running before the history state is updated. In particular, the active history entry etc. will still be on the old document. Is that OK, in the WebDriver BiDi context?

source Outdated Show resolved Hide resolved
@OrKoN
Copy link
Contributor Author

OrKoN commented Jan 23, 2025

I'm a little worried about this running before the history state is updated. In particular, the active history entry etc. will still be on the old document. Is that OK, in the WebDriver BiDi context?

WebDriver BiDi spec does depend on the history state for this event. In practice, I believe the history state would be completed before the event can be handled by the client. I think it would be great to emit when the state defined in the HTML spec is in the consistent state.

Is the history state usually updated in the completion steps of this algorithm? Should we move the event to after the completion steps are run? What would you recommend?

@domenic
Copy link
Member

domenic commented Jan 24, 2025

History state is updated in the monster algorithm apply the history step. Note that that algorithm is called for all history step updates, including traverses and reloads, not just the push/replace navigations you're interested in.

The algorithm you're modifying, "attempt to populate the history entry's document", is called in step 12.8.7 of "apply the history step". It basically does the fetch of the new document, but there are many steps left until we get to what I think of as the actual commit. The completion steps for it don't fully finish updating the history step: instead they enqueue an update which runs later, in step 14. Step 14.10 or 14.11 unloads the previous document, and then after that process is done it does a callback to afterPotentialUnloads.

afterPotentialUnloads (declared in step 14.12) is where things get interesting. By its step 2, the "browser process" is all updated. But the document itself is not yet updated to realize that it's active, necessarily: e.g., its navigation API state is not yet initialized, and same-document navigations or bfcache traversals haven't updated the relevant JS APIs. That happens in update document for history step application.

So I suspect what you want is to insert something either after step 2 of afterPotentialUnloads, if you only care about the "browser process", or at the end of "update document for history step application", if you care about the document's state. The latter sounds safer.

The hardest part will be filtering so that you only do this on cross-document push/replace navigations, but I think the "update document for history step application" algorithm has enough local state that you should be able to do this. (navigationType and documentIsNew should do it, I think.)

@OrKoN
Copy link
Contributor Author

OrKoN commented Jan 27, 2025

thanks @domenic It sounds like WebDriver BiDi would be interested in all situations when documentIsNew. I have updated the PR and moved the call to "update document for history step application" before any document scripts may run.

@jgraham could you please take a look at the proposal as well?

@OrKoN OrKoN force-pushed the orkon/navigation-comitted branch from 02e1537 to 4f82379 Compare January 27, 2025 16:41
source Show resolved Hide resolved
source Show resolved Hide resolved
@domenic domenic added the integration Better coordination across standards needed label Jan 31, 2025
@domenic domenic merged commit 5c7947f into whatwg:main Jan 31, 2025
2 checks passed
@OrKoN OrKoN deleted the orkon/navigation-comitted branch January 31, 2025 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration Better coordination across standards needed
Development

Successfully merging this pull request may close these issues.

4 participants