Skip to content

/ servo

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

Properly cancel event when beforeunload handler returns null #15142

Closed
wants to merge 2 commits into from
Closed

Properly cancel event when beforeunload handler returns null #15142

wants to merge 2 commits into from

Conversation

@nox
Copy link
Member

nox commented Jan 22, 2017 •
edited by larsbergstrom

This change is Reviewable
@highfive
Copy link

highfive commented Jan 22, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/eventtarget.rs, components/script/dom/beforeunloadevent.rs
  • @KiChjang: components/script/dom/eventtarget.rs, components/script/dom/beforeunloadevent.rs
@Ms2ger
Ms2ger reviewed Jan 23, 2017
View changes
components/script/dom/eventtarget.rs
}
None => {
event.upcast::<Event>().PreventDefault();
if let Ok(value) = handler.Call_(object, event, exception_handle) {

This comment has been minimized.

Sign in to view
Copy link
@Ms2ger

Ms2ger Jan 23, 2017

Contributor

Can you add some tests for returnValue? Stick them in event-handler-processing-algorithm.html along with the others.

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Jan 23, 2017

Author Member

What do you mean?

This comment has been minimized.

Sign in to view
Copy link
@Ms2ger

Ms2ger Jan 23, 2017

Contributor

We should have a test that uses BeforeUnloadEvent rather than Event.
@nox
Copy link
Member Author

nox commented Jan 23, 2017

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Jan 23, 2017

⌛ Trying commit 623ba9a with merge 4675448...
This was referenced Jan 23, 2017
Merged
Merged
Merged
@mbrubeck mbrubeck closed this Jan 24, 2017
@mbrubeck mbrubeck reopened this Jan 24, 2017
@mbrubeck mbrubeck closed this Jan 24, 2017
@mbrubeck mbrubeck reopened this Jan 24, 2017
@emilio emilio closed this Jan 24, 2017
@emilio emilio reopened this Jan 24, 2017
@nox nox force-pushed the nox:beforeunload branch from 623ba9a to 9808270 Jan 24, 2017
@nox
Copy link
Member Author

nox commented Jan 25, 2017

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2017

⌛ Trying commit 9808270 with merge 01dbaa5...
bors-servo added a commit that referenced this pull request Jan 25, 2017
@bors-servo
Auto merge of #15142 - nox:beforeunload, r=<try>
Loading status checks…
01dbaa5 
Properly cancel event when beforeunload handler returns null

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15142)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2017

💔 Test failed - linux-rel-wpt
@nox
Copy link
Member Author

nox commented Jan 25, 2017

Seems like #8157 is back?
@jdm
Copy link
Member

jdm commented Jan 25, 2017

In a new timeout-based form, sure. We should file a new issue for that.
@pcwalton pcwalton assigned Ms2ger and unassigned pcwalton Jan 31, 2017
@pcwalton
Copy link
Contributor

pcwalton commented Jan 31, 2017

Over to @Ms2ger
@jdm
Copy link
Member

jdm commented Jan 31, 2017

We should probably postpone this until whatwg/html#2297 is resolved.
@Ms2ger Ms2ger removed their assignment Feb 1, 2017
@domenic domenic mentioned this pull request Feb 11, 2017
Merged
@nox nox force-pushed the nox:beforeunload branch from 9808270 to 7a59471 Feb 24, 2017
nox added 2 commits Feb 24, 2017
@nox
Remove obsolete test expectation
f99cb9b
@nox
Properly cancel event when beforeunload handler returns non-null
Loading status checks…
429105b
@nox nox force-pushed the nox:beforeunload branch from 7a59471 to 429105b Feb 24, 2017
@nox
Copy link
Member Author

nox commented Feb 24, 2017

The HTML fix landed but the tests now require full unload support to run. We don't support that.
@nox nox closed this Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ms2ger Ms2ger

Assignees
No one assigned
Labels
S-awaiting-review S-needs-tests
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@nox @highfive @bors-servo @jdm @pcwalton @Ms2ger @mbrubeck @emilio
You can’t perform that action at this time.