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

Added "onbeforeunload" event prompt #98

Merged
merged 0 commits into from
Aug 22, 2022

Conversation

RamGoel
Copy link

@RamGoel RamGoel commented Aug 22, 2022

Prompts only if user has interacted with the webpage. (prompts on closing window and Reloading both)

@RamGoel
Copy link
Author

RamGoel commented Aug 22, 2022

@lgarron onbeforeunload event has been added #32

index.js Outdated
Comment on lines 6 to 8
window.onbeforeunload = function() {
return "";
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it's unconditionally called on page load? That would interfere with all the other testing use cases, so I think it should be behind a button just like the others.

(Also, would you be able to adjust your editor to avoid formatting the existing code, and avoid any changes in the PR that are not relevant to the new functionality? This will help the PR and project history be easier to review.)

Copy link
Author

@RamGoel RamGoel Aug 22, 2022

Choose a reason for hiding this comment

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

If we put the code on a button handler then what will be supposed to happen when button clicked.

onbeforeunload event activates for page so that when user closes the tab then a popup occurs. (delayed)
or
popup occurs and if user press ok then window closes. (instant)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we put the code on a button handler then what will be supposed to happen when button clicked.

Ah, I should have been a bit more clear. I think the beforeunload handler itself should be registered when the button is pressed. That is, you can click the button and then trigger a navigation if you want to test the beforeunload prompt, but if you don't click the button then the handler is never registered and doesn't interfere with testing other cases.

(On that subject, I would also strongly recommend using window.addEventListener(...) rather than window.onbeforeunload. The sample code from https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event#examples is probably a good reference.)

@RamGoel RamGoel merged commit 29f41b8 into chromium:master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants