Re-land fix for unload/beforeunload events #499
Conversation
jmesserly
force-pushed
the
polymer_issue_421_take2
branch
from
be21270
to
86138f9
Compare
| @@ -465,7 +476,8 @@ | |||
| function Event(type, options) { | |||
| if (type instanceof OriginalEvent) { | |||
| var impl = type; | |||
| if (!OriginalBeforeUnloadEvent && impl.type === 'beforeunload') { | |||
| if (!OriginalBeforeUnloadEvent && impl.type === 'beforeunload' && | |||
| !(this instanceof BeforeUnloadEvent)) { | |||
There was a problem hiding this comment.
BeforeUnloadEvent calls super here:
https://github.com/Polymer/ShadowDOM/blob/master/src/wrappers/events.js#L641
so we need to check that to prevent a stack overflow
There was a problem hiding this comment.
This could use a comment:
// In browsers that do not correctly support BeforeUnloadEvent we get to the generic
// Event wrapper but we still want to ensure we create a BeforeUnloadEvent. Since
// BeforeUnloadEvent calls super we need to prevent reentranctyI wonder if removing the super call would lead to cleaner code?
There was a problem hiding this comment.
good call on the comment.
I wonder if removing the super call would lead to cleaner code?
yeah, that's a really good point. It looks like the only line we'd need instead of the super call is setWrapper(impl, this);
|
btw, the branch name is wrong, but the issue linked in the commit is correct. :) |
jmesserly
force-pushed
the
polymer_issue_421_take2
branch
from
86138f9
to
55aada6
Compare
| @@ -465,7 +476,12 @@ | |||
| function Event(type, options) { | |||
| if (type instanceof OriginalEvent) { | |||
| var impl = type; | |||
| if (!OriginalBeforeUnloadEvent && impl.type === 'beforeunload') { | |||
| // In browsers that do not correctly support BeforeUnloadEvent we get to | |||
There was a problem hiding this comment.
ended up going with the comment and kept the super call in BeforeUnloadEvent ctor, since it looked like it was going to cause too much duplication to avoid the super constructor call
|
PTAL ... also, it seems like rebase+push lost the old comments ... sorry about that. Github used to say "comments discussing an outdated diff" but I don't see that anymore. I was using rebase to keep a clean commit history but maybe is not worth it. |
Re-land fix for unload/beforeunload events
The implementation code change compared to last time is fixes a stack overflow in the BeforeUnloadEvent polyfill, which the new test is hitting. This affected Safari which doesn't have a native BeforeUnloadEvent yet.
The test has several fixes for IE 10/11. IE had two different problems: e.target is window vs document in others, and the unload event won't fire in an iframe unless we setTimeout(0) before adding the handler.
This fixes https://github.com/Polymer/platform/issues/83
@kegluneq @arv mind taking another look?