Specify navigation committed event #855
Conversation
OrKoN
force-pushed
the
orkon/navigation-committed
branch
from
1573c2f to
31fd732
Compare
OrKoN
changed the title
| @@ -3168,7 +3173,7 @@ To <dfn>get the navigation info</dfn>, given |navigable| and |navigation status| | |||
| #### The browsingContext.ReadinessState Type #### {#type-browsingContext-ReadinessState} | |||
|
|
|||
| <pre class="cddl remote-cddl"> | |||
| browsingContext.ReadinessState = "none" / "interactive" / "complete" | |||
| browsingContext.ReadinessState = "none" / "committed" / "interactive" / "complete" | |||
There was a problem hiding this comment.
So this means that we are fine for the wait argument of browsingContext.navigate to allow committed as well? Asking because it causes a side-effect.
There was a problem hiding this comment.
Could you please clarify what you mean by the side-effect?
There was a problem hiding this comment.
So far we spoke about a committed event but not that we have another readiness state that could be used by other APIs to control the navigation flow like for browsingContext.navigate. In the current form this value for the wait argument wouldn't be denied but also not used in await a navigation because we don't have a check. So it's basically a no-op.
There was a problem hiding this comment.
I believe the value is checked on line 3045 (part of the await navigation algorithm you linked) so it does not appear to be no-op. I think it's needed for internal consistency but we can discuss it separately.
There was a problem hiding this comment.
Ah indeed. Even through I opened the preview in a different tab I actually checked the public BiDi spec page. Sorry for that.
While investigating some more Playwright tests I saw the following:
await page.goto(server.EMPTY_PAGE, { waitUntil: 'commit' })
Is that a condition that would help Puppeteer users as well that we should add to the navigate steps to return on the committed step? Or do it in a follow-up PR?
There was a problem hiding this comment.
No that is fine. So lets keep it in. Thanks.
It would be great if you could actually write some WPT tests for the event and the wait argument given that it seems to be easy for you to add support for committed (CDP already supports it).
There was a problem hiding this comment.
I filed GoogleChromeLabs/chromium-bidi#2982 to write the tests as part of the implementation.
There was a problem hiding this comment.
I tend to think we want this to continue to match the document.readyState property, but for the purposes of awaiting a navigation make "none" work like "committed", unless we have a use case where you really don't even care if the navigation started or not, but still need the return value of the command for some reason.
There was a problem hiding this comment.
I guess if we are not concerned about breaking changes, I am in favor of replacing none to mean committed (cc @sadym-chromium)
There was a problem hiding this comment.
I am in favor of replacing
noneto meancommitted
YES!
There was a problem hiding this comment.
What should we do with the browsingContext.navigationAborted event? Should we deprecate it? See #788 (comment)
| @@ -3168,7 +3173,7 @@ To <dfn>get the navigation info</dfn>, given |navigable| and |navigation status| | |||
| #### The browsingContext.ReadinessState Type #### {#type-browsingContext-ReadinessState} | |||
|
|
|||
| <pre class="cddl remote-cddl"> | |||
| browsingContext.ReadinessState = "none" / "interactive" / "complete" | |||
| browsingContext.ReadinessState = "none" / "committed" / "interactive" / "complete" | |||
There was a problem hiding this comment.
No that is fine. So lets keep it in. Thanks.
It would be great if you could actually write some WPT tests for the event and the wait argument given that it seems to be easy for you to add support for committed (CDP already supports it).
what is the specific reason for deprecating navigationAborted? I think we can discuss it separately. |
Co-authored-by: Henrik Skupin <[email protected]>
See the details in the referenced comment. That's all what I got. Happy to move it to a follow-up. |
Closes #788
HTML spec: whatwg/html#10910
Preview | Diff