Closed
Bug 757182
Opened 13 years ago
Closed 13 years ago
Handle window.close in <iframe mozbrowser>
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
close is the much simpler friend of window.open.
Assignee | ||
Comment 1•13 years ago
|
||
DOM changes, so you get to review. :)
Attachment #626128 -
Flags: review?(bugs)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → justin.lebar+bug
Comment 2•13 years ago
|
||
Comment on attachment 626128 [details] [diff] [review]
Patch v1
So why do we need DOMWindowClosing and DOMWindowClose?
Couldn't BrowserElementChild listen for the latter, perhaps in
system event group and check whether preventDefault was called.
Attachment #626128 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 3•13 years ago
|
||
> Couldn't BrowserElementChild listen for the latter, perhaps in
> system event group and check whether preventDefault was called.
My concern was that someone could preventDefault on the event after I checked.
But I think this was borne out of ignorance of the event model. The right thing to do is to register a capturing listener in the system event group, right? Nobody can call prevent default after the bubbling phase?
Assignee | ||
Comment 4•13 years ago
|
||
> The right thing to do is to register a capturing listener in the system event group, right?
Ah, now I remember why I did things this way.
Our event listener needs to fire after everyone who might call preventDefault has a chance to call preventDefault, because we do not want to fire a windowclosed event on the iframe if anybody canceled the close.
If we say that chrome may preventDefault during the system bubbling phase, then my listener has to run after that. That would be during the system capture phase, but of course I can't call preventDefault there myself!
In other words, if I were to use the system bubbling phase here, that would be an assertion that I am the only one who may ever call preventDefault() during that phase. Which seems pretty wrong to me.
What's the right way to handle this in the event model?
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #626128 -
Flags: review- → feedback?(bugs)
Comment 6•13 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #4)
> > The right thing to do is to register a capturing listener in the system event group, right?
>
> Ah, now I remember why I did things this way.
>
> Our event listener needs to fire after everyone who might call
> preventDefault has a chance to call preventDefault,
So, you need system event group.
> because we do not want
> to fire a windowclosed event on the iframe if anybody canceled the close.
>
> If we say that chrome may preventDefault during the system bubbling phase,
> then my listener has to run after that.
There is no way to do that. You can add listener to bubble phase, but if something
higher up in the event target chain calls preventDefault(), that happens after you have
handled the event.
> What's the right way to handle this in the event model?
System event group is the best we have for this. Other option is to change the code so
that you dispatch the event manually and check the return value of dispatchEvent.
Assignee | ||
Comment 7•13 years ago
|
||
> Other option is to change the code so
> that you dispatch the event manually and check the return value of dispatchEvent.
Could you elaborate on what you mean here? I don't understand.
Comment 8•13 years ago
|
||
Dispatch whatever event you want in your code and check the return value of dispatchEvent()
(it indicates whether preventDefault was called)
Assignee | ||
Comment 9•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #8)
> Dispatch whatever event you want in your code and check the return value of
> dispatchEvent()
> (it indicates whether preventDefault was called)
Isn't that what this patch does?
Assignee | ||
Comment 10•13 years ago
|
||
Comment on attachment 626128 [details] [diff] [review]
Patch v1
...and back to r? because I don't understand how comment 8 is different from what this patch is doing.
Attachment #626128 -
Flags: feedback?(bugs) → review?(bugs)
Assignee | ||
Comment 11•13 years ago
|
||
Comment on attachment 626128 [details] [diff] [review]
Patch v1
Actually, this doesn't quite work in-process. Patch in a moment.
Attachment #626128 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 12•13 years ago
|
||
Works in-process too, now.
(I'm fixing the tests so they always run in- and out-of-process, so I won't overlook things like this, in bug 762049.)
Attachment #626128 -
Attachment is obsolete: true
Attachment #630722 -
Flags: review?(bugs)
Comment 13•13 years ago
|
||
Comment on attachment 630722 [details] [diff] [review]
Patch v2
So, how does DOMWindowClosing help with anything? There can still be
listener in system group/bubble phase after your listener.
(When I said dispatch your own event, I mean the the JS code would need
to dispatch it and check the return value)
Just use the existing event.
Attachment #630722 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 14•13 years ago
|
||
> (When I said dispatch your own event, I mean the the JS code would need
> to dispatch it and check the return value)
I'm still not sure what you mean. What JS code, and when?
> So, how does DOMWindowClosing help with anything? There can still be
> listener in system group/bubble phase after your listener.
I guess I did not explain this well in the code:
> + // <iframe mozbrowser> needs a chance to handle this close event after
> + // DOMWindowClose. At this point, the window is going to close; it's just a
> + // matter of whether we're going to call FinalClose() or if someone else is
> + // going to promise to close the window some other way.
My thought was that DOMWindowClosing means "the window is going away -- nothing you can do about it". The only question is whether we're going to call FinalClose().
> Just use the existing event.
Okay.
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #630722 -
Attachment is obsolete: true
Attachment #630993 -
Flags: review?(bugs)
Comment 16•13 years ago
|
||
Comment on attachment 630993 [details] [diff] [review]
Patch v3
>+ els.addSystemEventListener(global, 'DOMWindowClose',
>+ this._closeHandler.bind(this),
>+ /* useCapture = */ true);
You probably want bubble phase here.
Attachment #630993 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 17•13 years ago
|
||
> You probably want bubble phase here.
Yes, thanks.
Assignee | ||
Comment 18•13 years ago
|
||
Target Milestone: --- → mozilla16
Comment 19•13 years ago
|
||
I may only get to it on Monday but believe me: I can't wait to test this! :)
Comment 20•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0d138bc9b28e
(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•