Add hooks for other specifications to observe navigation #6496
Conversation
|
Cross-linking to #6429. I'll try to help review this soon, but also discuss over there. |
|
On the multiple "other applicable specifications" hooks, I wonder if we could call out WebDriver BiDi specifically, but keep the hook names generic. This way, any other specs that wants to start using this would need to raise it here, or monkey-patch in a pretty obvious way. (Making the mechanism very generic and inviting for use right away seems risky.) |
source
Outdated
| <dl> | ||
| <dt><dfn data-x="navigation-status-id">id</dfn></dt> | ||
| <dd>The <span data-x="navigation-params-id">navigation id</span> for the navigation, or null if | ||
| the navigation is canceled early or is a synchronous fragment navigation</dd> |
There was a problem hiding this comment.
I guess the way to detect "synchronous fragment navigation" is that status is "complete" but there's no ID? Or is it possible for a "synchronous fragment navigation" to be canceled in some manner, leading to ambiguity?
There was a problem hiding this comment.
w3c/webdriver-bidi#93 refers to "fragment navigated steps" which aren't defined anywhere, but maybe that's the right approach, it would be more explicit even status could be used to distinguish.
Increasingly I'm thinking that the status should only be used for the regular navigation hook that goes through multiple stages, not the one-off hooks.
There was a problem hiding this comment.
I think complete with no id only happens in the fragment case.
source
Outdated
| data-x="navigation-status-id">id</span> is the <code>Document</code> object's <span | ||
| data-x="concept-document-navigation-id">navigation id</span>, <span | ||
| data-x="navigation-status-status">status</span> is "<code data-x="navigation-status-pending" | ||
| >pending</code>", and <span data-x="navigation-status-url">url</span> is the |
There was a problem hiding this comment.
Might it be important to distinguish this DOMContentLoaded hook from the start of navigation hook? The hook is different, but the navigation status struct has the same contents.
I guess that's fine, but the status actually seems like it's not needed here or for the "load complete steps" since they'll never be called with any other status. Perhaps making status nullable would make it more clear that it's not always meaningful?
There was a problem hiding this comment.
Yeah, I'm not sure. To me it seemed easier to have a consistent data structure where some of the fields are only really relevant in some cases (but the data is correct). If we were going to make the status null in the cases where you're really using the event name, it might be better to just have (id, url) in the struct and the status only in the sync return value for the navigate algorithm.
There was a problem hiding this comment.
This is like the "download started steps" steps, where the hook itself is conveying part of the information. This is all fine. I guess other options would be (1) just one hook and conveying everything through the struct or (2) each hook has its own argument list and no shared struct.
|
I updated this a bit. I don't quite understand how you want the hooks phrased in the end, so if you can give a concrete example of wording that would help. |
source
Outdated
| <code>Document</code>. User agents may do this before the complete document has been parsed (thus | ||
| achieving <i>incremental rendering</i>), and must do this before any scripts are to be | ||
| executed.</p> | ||
| <p>Then, with <var>document</var>, the user agent must <span>update the session history with the |
There was a problem hiding this comment.
| <p>Then, with <var>document</var>, the user agent must <span>update the session history with the | |
| <p>Then, the user agent must <span>update the session history with the |
The "with document" here seemed off to me, almost as if we're going for a JavaScript or Python with statement.
|
Alright, I think this is in good shape now, and here's what needs to be resolved to land it:
|
I'd prefer calls into Bidi :) |
|
Let's go with that then :) @jgraham is it unambiguous what that change would look like, other than naming choices? |
|
Well I can copy the existing examples. I assume it just means moving the definitions into BiDi and using wording like e.g. "Call the navigation started steps defined in [WebDriver-BiDi]". |
I think that's a question for the BiDi side of the patch, but I don't think it should be handled specially at all (or at least I think the event should always be emitted; we still need a consensus on the order). Not emitting the event requires strong coupling at the client level between the code that's starting a navigation and the code recieving events (e.g. it makes it impossible to have a tool that just listens for navigation events active at the same time as a tool that runs tests). |
Yeah, with "navigation started steps" being a cross-spec-link. (You could also give them more descriptive names if that'd be helpful, saying what the steps do instead of naming them after their location within the calling algorithm.) |
Aha, I see what you're intending. That makes sense as long as the ordering will end up the desired one. If we want the order to be something other than the emergent one, then it might be a bit complicated, but we'll see. We can change the HTML side of this if it turns out unsightly in BiDi ultimately. |
source
Outdated
| @@ -86123,6 +86209,14 @@ interface <dfn>Location</dfn> { // but see also <a href="#the-location-interface | |||
| <li><p>If <var>allowedToDownload</var> is true, then handle <var>response</var> <span>as a | |||
| download</span>.</p></li> | |||
|
|
|||
| <li><p>Call <span>WebDriver-BiDi download started</span> with <var>browsingContext</var> and | |||
There was a problem hiding this comment.
Note for other reviewers: yes communicating this case as a suddenly-complete navigation will make for mysterious-looking logic on the BiDi side, and maybe it should be more explicit, but we'll see when that side is written if it's fine or not.
source
Outdated
| data-x="concept-document-navigation-id">navigation id</span> is not null, let | ||
| <var>navigationId</var> be <var>browingContext</var>'s <span>active document</span>'s | ||
| <span data-x="concept-document-navigation-id">navigation id</span>. Otherwise let <var>navigation | ||
| id</var> be the string representation of a UUID based on truly random, or pseudo-random numbers |
There was a problem hiding this comment.
Here and below, I think referencing the UUID URN spec isn't quite right. Instead I suggest using generate a random UUID.
There was a problem hiding this comment.
I'm not sure why it's wrong. The RFC specifies an algorithm for generating the UUID and a string representation that's independent of the use as a URN. In particular the ABNF for the string representation in section 3 only includes the formatted hex digits, and not the urn:uuid: prefix. I am definitely more inclined to link to that than some WICG draft which has unknown levels of buy-in, and for which the proposed API surface isn't otherwise a dependency of this.
There was a problem hiding this comment.
I'm happy to switch the reference if you're not comfortable doing so; that's the web platform definition we're planning on using in HTML and other specs since it's more rigorous and clear.
There was a problem hiding this comment.
I agree that definition is clearer, but I'm not keen to be the first one to link to WICG drafts from HTML. Indeed, if the plan is to use that definition everywhere I'd propose we lift just the UUID generation algorithm into another spec e.g. infra.
There was a problem hiding this comment.
As I said, I'm happy to push a commit to this PR to update the reference if you're not comfortable doing so.
source
Outdated
| also create a corresponding <span>XML parser</span>. <ref spec=XML> <ref spec=XMLNS> <ref | ||
| spec=RFC7303> <ref spec=DOM></p> | ||
| data-x="create-the-document-object">create and initialize a <code>Document</code> object</span>, | ||
| <var>document</var> given "<code data-x="">xml</code>", <var>type</var>, and |
source
Outdated
| this before the complete document has been parsed (thus achieving <i>incremental rendering</i>), | ||
| and must do this before any scripts are to be executed.</p> | ||
|
|
||
| <p>Once parsing is complete, the user agent must set <var>document</var>'s <span |
There was a problem hiding this comment.
Isn't it null by default? Couldn't this whole section be left as-is?
There was a problem hiding this comment.
We set it when we initalize the document.
There was a problem hiding this comment.
I see. Why is it important to set it to null for XML documents in particular? Maybe worth a note?
There was a problem hiding this comment.
For HTML documents we end up setting it to null when we abort the document or finish parsing. Since parsing XML seems to still be undefined, I don't know where else to set it back to null once the navigation is complete.
There was a problem hiding this comment.
I see, that makes sense then. Can you add a note to that effect?
source
Outdated
| <var>document</var>'s <span data-x="concept-document-navigation-id">navigation id</span>, <span | ||
| data-x="navigation-status-status">status</span> is "<code data-x="navigation-status-canceled" | ||
| >canceled</code>", and <span data-x="navigation-status-url">url</span> is | ||
| <var>document</var>'s <span data-x="concept-document-url">URL</span>.</p></li> |
There was a problem hiding this comment.
I'm not so sure about this section. I'm guessing it's trying to handle the stop button but I think it's doing it wrong?
The stop button can do one of two things:
-
Abort a navigation which hasn't yet created a document. In this case it'll happen via step 3 of https://html.spec.whatwg.org/#stop-document-loading .
-
Abort the parser while it's busy filling the document. In that case it'll hit this abort algorithm. (
window.stop()goes down the same path.)
Which ones do you want to capture here? Do you want to capture window.stop()?
There was a problem hiding this comment.
I think this was just in the wrong order; we should emit the event before unsetting the navigation id. I think we want to always emit something to indicate that the navigation was interrupted (in particular: if it's a WebDriver-initiated navigation we need to be sure we return from the command even if stop() is called).
There was a problem hiding this comment.
My question is what you mean by "navigation is interrupted". As you've specced it here, you will fire "canceled" if the navigation response comes back, the document is created, etc., but then the user or script on the web page aborts the parser while it's still parsing. And you will also fire "canceled" if the navigate response comes back, the document is created and totally finishes parsing, but the user or script on the webpage aborts while some slow subresource is downloading.
Is that the intent? It doesn't map to how I normally think of "navigation", but maybe it maps to what WebDriver BiDi wants.
There was a problem hiding this comment.
I changed this to call a specific hook and let the WebDriver side deal with the semantics.
source
Outdated
| <dd>A status code that is either | ||
| "<dfn data-x="navigation-status-canceled"><code>canceled</code></dfn>", | ||
| "<dfn data-x="navigation-status-pending"><code>pending</code></dfn>", or | ||
| "<dfn data-x="navigation-status-complete"><code>complete</code></dfn>".</dd> |
There was a problem hiding this comment.
I guess the majority of navigations will never reach "complete", right? Maybe that's worth noting here?
There was a problem hiding this comment.
A majority by what measure? Anything that ends emitting a load event will reach complete, unless there's a mistake in the patch.
There was a problem hiding this comment.
Well, that actually will be a separate navigation status that gets created, disconnected from the first one which stays as "pending". I can see how that works for Web BiDi, where "navigation status" is just a packaging for sending information but it seems like it would make it hard to use this struct as something reflecting the actual ongoing status of the navigation throughout all callers and specs.
Given that, maybe it'd make the most sense to move this struct into the WebDriver BiDi spec and give it a name like WebDriver BiDi navigation signal?
There was a problem hiding this comment.
Oh, you mean that it's a stateless thing rather than being something stateful? Yes that's true. I've added a note to clarify that. Design wise communicating by broadcasting a message at defined points in the navigation lifecycle seems better than adding additional internal state that needs to be kept up to date (and closer to modern browser architecture where there can be multiple processes involved and synchronous access to internal state isn't possible).
There was a problem hiding this comment.
OK. Since this message-broadcasting infrastructure isn't something we can use in other specs or other parts of HTML, can you make the change to move the struct to Web BiDi and give it a name like WebDriver BiDi navigation signal?
There was a problem hiding this comment.
It would make more sense to me to try and align on a single set of lifecycle events for navigation, and allow different to consumers to extract the information they require, rather than having a bespoke set of events for every spec. That seems more likely to end up with a platform that behaves consistently across different APIs.
Anyway I guess I will refactor if that's what's needed to get this landed, but it isn't very encouraging to be three months, a hundred+ comments, and one approval, into a PR, only to be asked to make a large change that's essentially editorial in nature.
There was a problem hiding this comment.
Some more detailed questions:
- Is it OK for HTML to define Navigation ID? We end up storing that on the document, and passing it around the various navigation functions, so it seems pretty strange for it to be defined elsewhere, but like the struct it's currently only used by WebDriver.
- Is it OK for the navigate algorithm to continue to return a "WebDriver BiDi Navigation Signal", or do we also need additional changes to communicate the synchronous result of calling navigate via integration points rather than just using the call stack?
There was a problem hiding this comment.
Defining navigation ID in HTML seems reasonable.
I can't actually find anything that uses the return value of the navigate algorithm being the signal. Can you help me find where the return value is used?
There was a problem hiding this comment.
https://github.com/w3c/webdriver-bidi/pull/93/files#diff-5e793325cd2bfc452e268a4aa2f02b4024dd9584bd1db3c2595f61f1ecf7b985R1862-R1863 (all the WebDriver bits are currently blocked on this PR).
There was a problem hiding this comment.
I changed this. I'm honestly not convined it makes things better; it now means that if you add an extra return point from navigate you don't just need to get the return type of the algorithm right, you also need to add in an extra WebDriver BiDI call otherwise we will wait forever for the navigation to complete.
source
Outdated
| data-x="concept-document-navigation-id">navigation id</span> is not null, let | ||
| <var>navigationId</var> be <var>browingContext</var>'s <span>active document</span>'s | ||
| <span data-x="concept-document-navigation-id">navigation id</span>. Otherwise let <var>navigation | ||
| id</var> be the string representation of a UUID based on truly random, or pseudo-random numbers |
There was a problem hiding this comment.
I'm happy to switch the reference if you're not comfortable doing so; that's the web platform definition we're planning on using in HTML and other specs since it's more rigorous and clear.
source
Outdated
| this before the complete document has been parsed (thus achieving <i>incremental rendering</i>), | ||
| and must do this before any scripts are to be executed.</p> | ||
|
|
||
| <p>Once parsing is complete, the user agent must set <var>document</var>'s <span |
There was a problem hiding this comment.
I see. Why is it important to set it to null for XML documents in particular? Maybe worth a note?
source
Outdated
| <dd>A status code that is either | ||
| "<dfn data-x="navigation-status-canceled"><code>canceled</code></dfn>", | ||
| "<dfn data-x="navigation-status-pending"><code>pending</code></dfn>", or | ||
| "<dfn data-x="navigation-status-complete"><code>complete</code></dfn>".</dd> |
There was a problem hiding this comment.
Well, that actually will be a separate navigation status that gets created, disconnected from the first one which stays as "pending". I can see how that works for Web BiDi, where "navigation status" is just a packaging for sending information but it seems like it would make it hard to use this struct as something reflecting the actual ongoing status of the navigation throughout all callers and specs.
Given that, maybe it'd make the most sense to move this struct into the WebDriver BiDi spec and give it a name like WebDriver BiDi navigation signal?
This change is motivated by specifications, notably WebDriver-BiDi, that want to observe the progress of navigations. Currently specifications that want to do this effectively monkeypatch HTML by writing "when <some stageis reached>, run the following:". The aim here is to provide more definite integration points. Implementation wise, this adds a single new concept; a navigation id which is a unique id generated for each navigation. This enables consumers to tell when events are from the same navigation. On top of that, a navigation status struct is defined, with an id, a URL and a status. This is used as a uniform interface for communicating the navigation progress through all the integration points, even though not all fields are useful in all cases. The actual integration points added are: * Navigation started * Navigation failed * Download started * DOM load complete * Load complete In addition the navigate algorithm itself returns a navigation status, so that external specifications which initiate a navigation can get the associated navigation id (or find out that the navigation completed in the case of a fragment navigation).
jgraham
force-pushed
the
navigation_hooks
branch
from
83cb55b
to
596fe5f
Compare
There was a problem hiding this comment.
Thanks! This looks a lot cleaner now. I appreciate all the work you did to revise this to integrate better.
Do you want us to merge this right away, or wait for w3c/webdriver-bidi#93 ? I assume they should land at around the same time.
|
w3c/webdriver-bidi#93 is good to go now, so I'll merge both. |
|
Here's the commit message I'm going with: I hope (and think) that captures all of the recent changes. |
This change is motivated by specifications, notably WebDriver-BiDi,
that want to observe the progress of navigations. Currently
specifications that want to do this effectively monkeypatch HTML by
writing "when , run the following:". The aim
here is to provide more definite integration points.
Implementation wise, this adds a single new concept; a navigation id
which is a unique id generated for each navigation. This enables
consumers to tell when events are from the same navigation.
On top of that, a navigation status struct is defined, with an id, a
URL and a status. This is used as a uniform interface for
communicating the navigation progress through all the integration
points, even though not all fields are useful in all cases.
The actual integration points added are:
In addition the navigate algorithm itself returns a navigation status,
so that external specifications which initiate a navigation can get
the associated navigation id (or find out that the navigation
completed in the case of a fragment navigation).
Closes #6429.
💥 Error: connect ETIMEDOUT 128.30.52.141:443 💥
PR Preview failed to build. (Last tried on Jul 8, 2021, 7:11 PM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 HTML Diff Service - The HTML Diff Service is used to create HTML diffs of the spec changes suggested in a pull request.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.