wdspec: add test for interrupted navigation #54452
Conversation
wpt-pr-bot
requested review from
AutomatedTester,
jgraham,
sadym-chromium,
shs96c and
whimboo
juliandescottes
force-pushed
the
issue763-example
branch
from
6ed454a to
18f0d7c
Compare
| await subscribe_events(events=[CONTEXT_LOAD_EVENT]) | ||
| on_entry = wait_for_event(CONTEXT_LOAD_EVENT) | ||
|
|
||
| result = await bidi_session.browsing_context.navigate( |
There was a problem hiding this comment.
unrelated to this specific test: do we actually have a way to trigger an error with navigate? I guess a navigate call with beforeunload handler navigating elsewhere would trigger the condition of a new navigation before a commit?
There was a problem hiding this comment.
That should work. In practice any scenario that notifies BiDi about a "canceled"/"navigation failed" status. From a quick read of the HTML spec I can see: CSP issues, beforeunload, unloading a tab, closing/destroying a document.
Eg wait=complete + closing a tab before the navigation finishes should lead to an error.
|
Copy pasting the current status for this test from w3c/webdriver-bidi#763 With the current implementation Chrome fails when
Firefox fails in the same cases as Chrome, but also in:
|
|
@OrKoN @sadym-chromium do we agree that beyond the edge cases mentioned in the inline comments, all those navigate calls should succeed? |
|
@sadym-chromium could you please review? I saw that you started looking into this in GoogleChromeLabs/chromium-bidi#3671 |
Currently, Chrome fail the command if the navigation was replaced by another navigation before the command successfully finished. According to the specification (extended in w3c/webdriver-bidi#799), those commands should be successfully finished. I agree that if we don't agree on changing that behavior, we need to align WPT with the spec. Let me take a deeper look at the WPT you proposed |
|
|
||
| any_string(result["navigation"]) | ||
|
|
||
| event = await wait_for_future_safe(on_entry) |
There was a problem hiding this comment.
This wait is tricky, as it can be triggered for the CONTEXT_LOAD_EVENT of the first navigation, the same moment the "Redirect on load" happens. I would tight the logic to browsingContext.navigationStarted events, as they should be emitted for each navigaiton.
There was a problem hiding this comment.
Sounds good, so waiting for 2 navigation started events. In addition to that I can also wait for a context load event for the second URL.
There was a problem hiding this comment.
It seems that Firefox is not capturing the 2nd navigation started, but arguably that's a bug on our end. I would propose the following: only wait for CONTEXT_LOAD_EVENT here, but for the final url (to address your initial concern) and add another navigation_started test to cover the issue I just found.
sadym-chromium
left a comment
sadym-chromium
left a comment
There was a problem hiding this comment.
Let's use browsingContext.navigationStarted to detect the second navigation is actually started.
Thanks for checking, as long as we have the same understanding of the spec (ie, all the tests here should PASS) I'm OK, I don't want to revisit this part of the spec :) But since no browser passed the tests currently I preferred to check first. |
juliandescottes
force-pushed
the
issue763-example
branch
from
18f0d7c to
efb92ea
Compare
Pull request was closed
|
Flaky test seems to be an existing issue. |
6 participants
No description provided.