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

[wdspec] change test_..._closes_browsing_context #49759

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

sadym-chromium
Copy link
Contributor

@sadym-chromium sadym-chromium commented Dec 18, 2024

The tests relied on the script closing browsing context which causes "Scripts may only close windows that were opened by a script".

Open context with window.open to allow it to be closed by script.

The tests relied on the script closing browsing context which causes "Scripts may only close windows that were opened by a script".

The new approach is to close browsing context by a command during actions chain. This approach can be racy though.
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is specific to handling of actions and when a event handler closes the tab/window. We should not make use of a different BiDi command here, but as you say need to open a new window first by using window.open().

Not sure why we set this preference in Firefox by default to change the default behavior which is a bit concerning and I wonder how other wpt tests actually result in when we would change that. @jgraham do you have an idea?

Open context with `window.open` to allow it to be closed by script.
@OrKoN OrKoN requested a review from whimboo December 18, 2024 13:11
@sadym-chromium
Copy link
Contributor Author

This test is specific to handling of actions and when a event handler closes the tab/window. We should not make use of a different BiDi command here, but as you say need to open a new window first by using window.open().

We need to rely on other BiDi commands. Changed approach to open a window via script, which uses script.evaluate command to open the window and browsingContext.load to verify the window is opened.

@OrKoN OrKoN dismissed whimboo’s stale review December 18, 2024 14:09

the concerns are addressed

@sadym-chromium sadym-chromium merged commit 16e6fbc into master Dec 18, 2024
19 checks passed
@sadym-chromium sadym-chromium deleted the sadym/input-tests-browsing-context-closed branch December 18, 2024 14:09
@whimboo
Copy link
Contributor

whimboo commented Dec 18, 2024

Thanks for the fix @sadym-chromium! Please note that we have similar tests for WebDriver classic as well. Would you be able to provide a similar PR?

@sadym-chromium
Copy link
Contributor Author

Thanks for the fix @sadym-chromium! Please note that we have similar tests for WebDriver classic as well. Would you be able to provide a similar PR?

Waiting for the new window to be loaded in classic is way more tricky. Do you have an idea on how to make it?

@sadym-chromium
Copy link
Contributor Author

Thanks for the fix @sadym-chromium! Please note that we have similar tests for WebDriver classic as well. Would you be able to provide a similar PR?

#49765

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants