Add navigation hooks for WebDriver BiDi history traversal #6921

jgraham · 2021-07-30T20:22:27Z

WebDriver BiDi wants to invoke the 'traverse the history by a delta`
algorithm, and get a callback whenever the algoritihm has run to
completion, either by failing or by the navigation or state
restoration completing.

w3c/webdriver-bidi#109 is the WebDriver BiDi
side of this change.

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

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

domenic · 2021-07-30T22:19:10Z

I'll review Monday (although can you try fixing the build first?).

Sadly this algorithm does not match browsers very well, but I imagine we can slot something in for now. /cc @jakearchibald as something to take into account when rewriting.

domenic

Mostly nits; great spec work as always.

source

domenic · 2021-08-02T19:51:33Z

source

+        data-x="dom-PopStateEvent-state">state</code> attribute initialized to <var>state</var>.
+        </p></li>
+
+        <li><p>Invoke <span>WebDriver BiDi pop state</span> with <var>newDocument</var>'s


Just double checking that you actually want to carry over the idiosyncratic semantics of popstate and hashchange to WebDriver BiDi consumers? Web developer feedback has been pretty adamant that these events are kinda useless; see e.g. #5562 . But I'm not sure what the WebDriver BiDi consumer use case is.

We current proposal for WebDriver doesn't actually expose these events, it's basically just ensuring that a user traverses history in a way that just pops state, we return a response to the traverse history command at the point where we're done.

Would it make sense to bundle them both into a single hook then? Maybe the same hook as page show even? But I defer to your judgment as to what are the best hook points for your spec.

Well I think it's more likely we're going to end up exposing more stuff via webdriver than less, and having seperate hooks minimises the chance we'll need further changes to HTML. Most likely I think page show will be exposed, and popstate won't, but I'm not sure (hash change is actually already exposed).

source

domenic

I pushed some editorial fixes in the hopes of moving this along. But I found several instances of referencing undeclared variables so there might be some deeper logic issues...

source

foolip

I could summarize this review as: ensuring exactly one callback for the right reasons is hard!

foolip · 2021-09-23T07:03:37Z

source

+    <span>joint session history</span>, then:</p>
+
+    <ol>
+     <li><p>Invoke <span>WebDriver BiDi navigation failed</span> with <var>source browsing


Since this will happen for all calls of this algorithm, including history.go(1000) in JS, this could incorrectly end an ongoing BiDi navigation. history.go(1000) is really a no-op, and shouldn't affect BiDi.

I see a two options:

Use a return value instead

Add a wrapper for this algorithm only used by BiDi

An option that works but isn't a candidate for me is some form of COMEFROM, maybe using navigationId to mean "called from BiDi" and not invoking the callback otherwise. Ugh.

We could also just drop navigationFailed with null navigation id on the BiDi side.

Yeah, if BiDi always passing a navigation ID and nothing else does, that would work. If some other spec later integrated with HTML and uses navigation IDs I'm not sure what will happen, but we could punt on that hypothetical.

foolip · 2021-09-23T07:06:57Z

source

-   <li><p>If the <var>specified browsing context</var>'s <span>active document</span>'s <span>unload
-   counter</span> is greater than 0, then return.</p></li>
+    <ol>
+     <li><p>Invoke <span>WebDriver BiDi navigation failed</span> with <var>source browsing


This too could presumably happen for reasons other than a BiDi command, and influence other BiDi commands that don't invoke this algorithm.

What's the concern with influencing other BiDi commands? If a BiDi command invokes this it has to provide a navigation id, and that id is used to associate these callbacks with the ongoing command.

Alright, so same answer with using the navigation ID. That should work!

foolip · 2021-09-23T07:15:02Z

source

   data-x="concept-document-url">URL</span>.</p></li>
-
-   <li><p><p>Invoke <span>WebDriver BiDi fragment navigated</span> with <var>browsingContext</var>,


It's hard to spot in the diff where different algorithms start and end, so noting for other reviewers that this is the end of "navigate to a fragment" and this "WebDriver BiDi fragment navigated" invocation ends up at the end of "traverse the history" (below) which is invoked above.

foolip · 2021-09-23T07:17:37Z

source

+       and the <code data-x="dom-HashChangeEvent-newURL">newURL</code> attribute initialized to
+       <var>newURL</var>.</p></li>
+
+       <li><p>Invoke <span>WebDriver BiDi fragment navigated</span> with <var>newDocument</var>'s


The previous invocation of "WebDriver BiDi fragment navigated" was unconditional but this one depends on hashChanged being true.

Does this mean that trying to navigate to the current URL never gets a callback? Or is there a callback somewhere and before this change we had a broken situation with two callbacks being invoked?

So is there a case where hashchanged is false, statechanged is false, and there isn't going to be a later load event (assuming the navigation isn't cancelled)? If there is such an event I think we're missing a callback, but it at least makes sense to me that we don't want to do the fragment navigation steps on the BiDi side unless the fragment is actually navigated.

Yeah, it does make sense to make this callback conditional, the previous state of things was more strange.

My "navigate to the current URL" example seems more complicated than I thought, what happens depends on whether there's a fragment, tested with window.location.href = window.location.href in DevTools. For https://whatwg.org/faq it will reload the page, and for https://whatwg.org/faq#the-whatwg it does "nothing".

I think https://whatwg.org/faq#the-whatwg is the interesting case. If BiDi's browsingContext.navigate command navigates to that URL when it's already the current URL, I think that's such a case. Or what would cause that command to eventually succeed or fail?

OK, so I added an extra clause to ensure that if the document didn't change and the hash didn't change, and there wasn't any state change, we still end up with a callback. Although now I write this I'm wondering if it's possible this breaks in the case of bfcache.

I think my worry about bfcache is unfounded, since different loads of the same URL will result in different documents, so something like A -> B -> A' (where A and A' have the same URL), and then back to A, will end up with a different document in the entry even though the URLs might match and no actual load occured.

WebDriver BiDi wants to invoke the 'traverse the history by a delta` algorithm, and get a callback whenever the algoritihm has run to completion, either by failing or by the navigation or state restoration completing. w3c/webdriver-bidi#109 is the WebDriver BiDi side of this change.

…ill be no load event. In particular if webdriver initiates a navigation to the current URL and it includes a fragment, this seems to be required to ensure the navigation completes on the WebDriver side.

foolip · 2022-09-14T09:46:40Z

@jgraham Can I help make progress on this somehow? It's blocking w3c/webdriver-bidi#109, right?

domenic · 2022-09-14T16:32:33Z

At this point, any work in this area should be done against the branch in #6315. Note that traversing can traverse multiple frames (and this is much more explicit in that branch), or maybe zero (if bfcache is not in effect and the server ends up with a 204/download). I imagine that might affect the "one callback, once" idea...

foolip · 2022-11-25T16:52:03Z

#6315 has been merged now so this could be rebased. @jgraham do you think you'll get a chance to do that? Happy to help if that would be... helpful.

foolip · 2023-02-23T08:42:13Z

I tried to resolve conflicts, but everything except the first 2-line change to link definitions conflicts. I think the best approach is to go hook-by-hook and find the right place to invoke it in the new spec:

WebDriver BiDi navigation failed
WebDriver BiDi page show
WebDriver BiDi pop state
WebDriver BiDi fragment navigated

…versal_hooks

foolip · 2023-02-23T14:22:49Z

OK, I've put everything where I think it needs to be. I'll do a self-review to point out things I'd like feedback on.

foolip · 2023-02-23T14:29:02Z

source

+       <li><p>Invoke <span>WebDriver BiDi pop state</span> with <var>document</var>'s
+       <span data-x="concept-document-bc">browsing context</span> and a new <span>WebDriver
+       BiDi navigation status</span> whose <span data-x="navigation-status-id">id</span> is
+       <var>document</var>'s <span data-x="concept-document-navigation-id">navigation id</span>,


I'm skeptical that we should use document's navigation id here and below. We should instead pass it through as an argument to "update document for history step application".

Actually I'm not sure these changes are needed, because we're inside of "update document for history step application". When that's called from "navigate to a fragment" it will be followed by invoking "WebDriver BiDi fragment navigated".

But when it's called from "apply the history step" it's much harder to follow all of the call sites. I think there are ways for "traverse the history by a delta" to end without invoking any BiDi callback. But more importantly that algorithm doesn't take a navigation id because I failed to carry that forward when resolving conflicts, see https://github.com/whatwg/html/pull/6921/files/9afc0d64707caa64ae750dc8117cbb2c4194a52a for @jgraham's original changes.

foolip · 2023-02-23T14:59:15Z

Looking some more at this, I'm not sure we we need to add additional callbacks. #6315 did a lot of refactoring and it's now easier to follow whether a callback is always reached.

I spotted one error that I'm fixing in #8939.

jgraham mentioned this pull request Jul 30, 2021

Add a browsingContext.traverseHistory command w3c/webdriver-bidi#109

Draft

domenic added the topic: history label Jul 30, 2021

domenic reviewed Aug 2, 2021

View reviewed changes

jgraham force-pushed the webdriver_hsitory_traversal_hooks branch from 6f9478b to 725ca64 Compare September 15, 2021 14:38

jgraham requested a review from domenic September 15, 2021 14:39

domenic force-pushed the webdriver_hsitory_traversal_hooks branch 2 times, most recently from 961fab4 to 2c7e630 Compare September 16, 2021 20:38

domenic requested changes Sep 16, 2021

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

foolip reviewed Sep 23, 2021

View reviewed changes

jgraham and others added 5 commits October 13, 2021 16:24

Fix review issues

8e1e0b4

Mostly-editorial fixes

c3fb352

Fix missing variables

363878c

Ensure we do a fragment navigated callback for any case where there w…

9afc0d6

…ill be no load event. In particular if webdriver initiates a navigation to the current URL and it includes a fragment, this seems to be required to ensure the navigation completes on the WebDriver side.

jgraham force-pushed the webdriver_hsitory_traversal_hooks branch from e0fc9dc to 9afc0d6 Compare October 13, 2021 15:26

foolip added 2 commits February 23, 2023 15:20

Merge remote-tracking branch 'origin/main' into webdriver_hsitory_tra…

2184984

…versal_hooks

Adapt code to new surroundings

e7883e1

foolip reviewed Feb 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add navigation hooks for WebDriver BiDi history traversal #6921

Add navigation hooks for WebDriver BiDi history traversal #6921

jgraham commented Jul 30, 2021 •

edited by pr-preview bot

Loading

domenic commented Jul 30, 2021

domenic left a comment

domenic Aug 2, 2021

jgraham Sep 15, 2021

domenic Sep 16, 2021

jgraham Sep 22, 2021

domenic left a comment

foolip left a comment

foolip Sep 23, 2021

jgraham Oct 13, 2021

foolip Oct 13, 2021

foolip Sep 23, 2021

jgraham Oct 13, 2021

foolip Oct 13, 2021

foolip Sep 23, 2021

foolip Sep 23, 2021

jgraham Oct 13, 2021

foolip Oct 13, 2021

jgraham Oct 13, 2021

jgraham Oct 13, 2021

foolip commented Sep 14, 2022

domenic commented Sep 14, 2022 •

edited

Loading

foolip commented Nov 25, 2022

foolip commented Feb 23, 2023 •

edited

Loading

foolip commented Feb 23, 2023

foolip Feb 23, 2023

foolip Feb 23, 2023 •

edited

Loading

foolip commented Feb 23, 2023

		data-x="concept-document-url">URL</span>.</p></li>

		<li><p><p>Invoke <span>WebDriver BiDi fragment navigated</span> with <var>browsingContext</var>,

Add navigation hooks for WebDriver BiDi history traversal #6921

Are you sure you want to change the base?

Add navigation hooks for WebDriver BiDi history traversal #6921

Conversation

jgraham commented Jul 30, 2021 • edited by pr-preview bot Loading

domenic commented Jul 30, 2021

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

foolip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip commented Sep 14, 2022

domenic commented Sep 14, 2022 • edited Loading

foolip commented Nov 25, 2022

foolip commented Feb 23, 2023 • edited Loading

foolip commented Feb 23, 2023

Choose a reason for hiding this comment

foolip Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

foolip commented Feb 23, 2023

jgraham commented Jul 30, 2021 •

edited by pr-preview bot

Loading

domenic commented Sep 14, 2022 •

edited

Loading

foolip commented Feb 23, 2023 •

edited

Loading

foolip Feb 23, 2023 •

edited

Loading