-
Notifications
You must be signed in to change notification settings - Fork 44
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
Extend browsingContext.close
params with promptUnload
#518
Extend browsingContext.close
params with promptUnload
#518
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (wait for other reviewers)
@juliandescottes @jgraham @whimboo would it be possible for you to review this one? it seems uncontroversial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks fine to me, thanks
@jgraham should we land this change or do you have concerns about exposing this? |
Co-authored-by: Alex Rudenko <OrKoN@users.noreply.github.com>
Is there a bug or PR for the corresponding WPT test? |
I think this is OK in principle. I also think we should fix this to not work by "magic" but actually modify https://html.spec.whatwg.org/#steps-to-fire-beforeunload to skip the prompt in some cases. It seems like we could reuse the existing That doesn't block landing this one, but we should at least file an issue. |
Issue on HTML spec - whatwg/html#9890 , please add anything I may have missed. |
SHA: 409761f Reason: push, by Lightning00Blade Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Lightning00Blade Thanks for your work! Are you going to also create a test for that? |
Yes I will add WPT test for it, I already updated Chromium-BiDi GoogleChromeLabs/chromium-bidi#1508 to support it, so it should be easier to create them now. |
Hi @Lightning00Blade. Quick question if you still have it in your queue. Thanks. |
FYI the tests will be added via web-platform-tests/wpt#44004. |
Closes #462
We already define this behavior on
browser.close
command. This PR extends this to thebrowsingContext.close
as a parameter.Default it chosen to not trigger
prompting to unload
with the idea that this is used more like resource cleanup rather then testingbeforeUnload
prompts and alike.Preview | Diff