The default bug view has changed. See this FAQ.

Handle window.close in <iframe mozbrowser>

RESOLVED FIXED in mozilla16

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
close is the much simpler friend of window.open.
(Assignee)

Comment 1

5 years ago
Created attachment 626128 [details] [diff] [review]
Patch v1

DOM changes, so you get to review.  :)
Attachment #626128 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

5 years ago
Depends on: 758297
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

5 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

5 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

5 years ago
Comment on attachment 626128 [details] [diff] [review]
Patch v1

f? for comment 4.
Attachment #626128 - Flags: review- → feedback?(bugs)
(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

5 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.
Dispatch whatever event you want in your code and check the return value of dispatchEvent()
(it indicates whether preventDefault was called)
(Assignee)

Comment 9

5 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

5 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

5 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

5 years ago
Created attachment 630722 [details] [diff] [review]
Patch v2

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)
(Assignee)

Updated

5 years ago
Blocks: 762049
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

5 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

5 years ago
Created attachment 630993 [details] [diff] [review]
Patch v3
Attachment #630722 - Attachment is obsolete: true
Attachment #630993 - Flags: review?(bugs)
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

5 years ago
> You probably want bubble phase here.

Yes, thanks.
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d138bc9b28e
Target Milestone: --- → mozilla16
I may only get to it on Monday but believe me: I can't wait to test this! :)
https://hg.mozilla.org/mozilla-central/rev/0d138bc9b28e

(Merged by Ed Morley)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 764718
You need to log in before you can comment on or make changes to this bug.