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

Invoke WebDriver BiDi history updated #10587

Merged
merged 3 commits into from
Oct 23, 2024
Merged

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Aug 29, 2024

This PR adds a callback to the WebDriver BiDi specification to notify about history updates.

WebDriver BiDi change: w3c/webdriver-bidi#740

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


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

@sadym-chromium
Copy link

LGTM (I'm not a committer, so cannot formally approve it)

@OrKoN OrKoN marked this pull request as ready for review September 18, 2024 12:27
@OrKoN
Copy link
Contributor Author

OrKoN commented Sep 18, 2024

I am working on the WPT tests but I think this change is ready for initial review.

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.

What about all the other history updates? E.g. navigation API, document.open(), updates from about:blank to about:blank#foo, ...

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

OrKoN commented Sep 25, 2024

I believe the use of the navigation API would end up in the navigate steps where we already have instrumentation using WebDriver BiDi navigation started and WebDriver BiDi fragment navigated (also, covers fragment navigations).

document.open seems to be ending up either in the navigate steps or does not change change the URL of the document? I think we might also want to cover the document.open() case but I am not 100% sure.

What I tried to implement in this PR is to track history updated but exclude any history updates internal to the HTML spec that are a side-effect of using other navigation mechanisms, e.g., of navigate steps or processing of iframe elements.

Please correct me if I am wrong about some aspects of how the spec handles history updates.

@domenic
Copy link
Member

domenic commented Sep 25, 2024

I believe the use of the navigation API would end up in the navigate steps where we already have instrumentation using WebDriver BiDi navigation started and WebDriver BiDi fragment navigated (also, covers fragment navigations).

Not if the navigation is intercepted by using navigateEvent.intercept(). Then it ends up in the URL and history update steps.

If the navigation is intercepted by using navigateEvent.intercept(), then it looks like you'll call WebDriver BiDi navigation started but never anything else to signal navigation completed. Is that OK? Specifically you'll end up in https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate step 21.5.

document.open seems to be ending up either in the navigate steps or does not change change the URL of the document? I think we might also want to cover the document.open() case but I am not 100% sure.

It changes the URL of the document in step 12.3 of https://html.spec.whatwg.org/#document-open-steps

processing of iframe elements

So in particular the case like

const iframe = document.createElement("iframe");
iframe.src = "about:blank?foo";
document.body.append(iframe);

will first navigate to about:blank, then do a "URL and history update steps" to replace the history entry with one for about:blank?foo. Do you want to capture that, or not?

@OrKoN
Copy link
Contributor Author

OrKoN commented Sep 26, 2024

If the navigation is intercepted by using navigateEvent.intercept(), then it looks like you'll call WebDriver BiDi navigation started but never anything else to signal navigation completed. Is that OK? Specifically you'll end up in https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate step 21.5.

good point, I think we would want to emit an aborted or a failed event here. cc @jgraham

It changes the URL of the document in step 12.3 of https://html.spec.whatwg.org/#document-open-steps

Interesting, when testing (in Chrome and Firefox) this I do not observe the fragment change on document.open. Do you know if there are existing WPT tests for this feature? I think if the URL can actually change here we would want to notify the client.

will first navigate to about:blank, then do a "URL and history update steps" to replace the history entry with one for about:blank?foo. Do you want to capture that, or not?

That is a special case for about:blank URLs, right? I think we should capture that, I will update the patch.

@OrKoN
Copy link
Contributor Author

OrKoN commented Sep 26, 2024

Interesting, when testing (in Chrome and Firefox) this I do not observe the fragment change on document.open. Do you know if there are existing WPT tests for this feature? I think if the URL can actually change here we would want to notify the client.

It is conditional on "If entryDocument is not document" but I am not quite sure yet when this is the case.

@OrKoN
Copy link
Contributor Author

OrKoN commented Sep 26, 2024

I think given these cases, we probably want to move the hook to https://html.spec.whatwg.org/#url-and-history-update-steps it looks like all the invocations could be actually relevant for automation.

@OrKoN
Copy link
Contributor Author

OrKoN commented Sep 26, 2024

@domenic I see notes like This is necessary in case url is something like about:blank?foo. If url is just plain about:blank, this will do nothing. when the URL and history update steps are invoked for URLs matching about:blank but it seems currententrychange could be emitted still. Perhaps I am misunderstanding how the "do nothing" part is achieved but I do not see early returns in the steps.

@domenic
Copy link
Member

domenic commented Sep 30, 2024

It changes the URL of the document in step 12.3 of https://html.spec.whatwg.org/#document-open-steps

Interesting, when testing (in Chrome and Firefox) this I do not observe the fragment change on document.open. Do you know if there are existing WPT tests for this feature? I think if the URL can actually change here we would want to notify the client.

It is conditional on "If entryDocument is not document" but I am not quite sure yet when this is the case.

The case is tested by the various files starting with url- at https://wpt.fyi/results/html/webappapis/dynamic-markup-insertion/opening-the-input-stream?label=master&label=experimental&aligned , e.g. https://github.com/web-platform-tests/wpt/blob/03c258eab4bb0dc037d0b3822049cada69a49427/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window.js#L1-L9 is a simple case.

but it seems currententrychange could be emitted still.

This is prohibited by https://html.spec.whatwg.org/#update-the-navigation-api-entries-for-a-same-document-navigation step 1.

I think given these cases, we probably want to move the hook to https://html.spec.whatwg.org/#url-and-history-update-steps it looks like all the invocations could be actually relevant for automation.

This seems like the right move to me. I'll approve this and give a day or three to see if anyone else has comments, since this is a complex area, but what you've done makes sense from my perspective.

@domenic domenic added topic: navigation integration Better coordination across standards needed labels Sep 30, 2024
@OrKoN
Copy link
Contributor Author

OrKoN commented Sep 30, 2024

Thanks for reviewing! let's keep it open for a week or two to let everyone review and I want to add more WPT tests.

Regarding https://github.com/web-platform-tests/wpt/blob/03c258eab4bb0dc037d0b3822049cada69a49427/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window.js#L1-L9: it appears that initial URL is about:blank and after a change it is still about:blank. I am also confused by this test https://github.com/web-platform-tests/wpt/blob/03c258eab4bb0dc037d0b3822049cada69a49427/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url-fragment.window.js#L1: it looks like the hash is changed on the parent, but the change is asserted on the frame document? And the second test there seems to assert the same before and after URL?

@domenic
Copy link
Member

domenic commented Oct 1, 2024

it appears that initial URL is about:blank and after a change it is still about:blank

No, on line 4 it is about:blank, and on line 7 it is https://wpt.fyi/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/url.window.html.

I am also confused by this test

The point is that document.open() copies the URL from the entry document (in this case, the parent document) to the iframe.

And the second test there seems to assert the same before and after URL?

Yes, in that case the entry document is the relevant document, so there's no change, per https://html.spec.whatwg.org/#document-open-steps step 12.2.

@OrKoN
Copy link
Contributor Author

OrKoN commented Oct 1, 2024

Thanks, I got a bit confused by DevTools, it actually does not handle this update well (still shows about:blank in the tree):

Screenshot 2024-10-01 at 17 44 27

Edit: filed https://crbug.com/370690261

@OrKoN
Copy link
Contributor Author

OrKoN commented Oct 22, 2024

We have landed the change in the WebDriver BiDi spec so this change is ready to be merged from our perspective. cc @domenic

@domenic domenic merged commit bd08d17 into whatwg:main Oct 23, 2024
2 checks passed
@OrKoN OrKoN deleted the orkon/history-updated branch October 23, 2024 05:08
AtkinsSJ added a commit to AtkinsSJ/ladybird that referenced this pull request Oct 31, 2024
AtkinsSJ added a commit to AtkinsSJ/ladybird that referenced this pull request Nov 8, 2024
awesomekling pushed a commit to LadybirdBrowser/ladybird that referenced this pull request Nov 9, 2024
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 topic: navigation
Development

Successfully merging this pull request may close these issues.

3 participants