570115 - Showing FilePicker dialogs does not send DOMWillOpenModalDialog

Status: NEW

(Core :: DOM: Events, defect)

Description

[Steffen Imhof]

When writing extensions it might be interesting to know, when a new modal dialog is opened. There is a sparsely documented pair of events (DOMWillOpenModalDialog/DOMModalDialogClosed) that get fired for example by JS alerts.
These are not sent whenever a file selection dialog is opened.

Comment 1

[Steffen Imhof]

Attachment: Patch to send DOM events for modal dialog when a file picker is shown

The attached patch changed nsBaseFilePicker to implement Show() itself and fire the events, calling a new virtual method ShowNative() in between.
So any backend derived from nsBaseFilePicker that should send out the events can do its stuff in ShowNative() instead of implementing Show() directly.
This patch does this only for Qt because that's the only version I can really test.

Also I'm not 100% sure about the component and suitable reviewer, so Doug if needed, please change accordingly.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Eek, I am not a good person to review this -- I think you want someone who knows event stuff, since that's the bulk of the patch.  Not sure who to suggest :/

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Make sure the events are dispatched when nsIContentUtils::IsSafeToRunScript() returns true.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Seems like the events are dispatched at right time.

Steffen, is the change ok to toolkit/
Perhaps you could ask feedback from some toolkit reviewer? dao, or gavin?
After that I could review the patch.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Not sure what change in toolkit?  The base functionality seems fine to me, though would be nice to see a test that the events are getting dispatched.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Shouldn't the requirement that file picker impls fire these events be documented?  And do we not need to fix the impls that don't inherit from nsBaseFilePicker too?

Comment 7

[Boris Zbarsky [:bzbarsky]]

And as a note, the reason those events were added was to focus a background tab if it was about to put up a modal dialog.  Since filepickers couldn't just come up on their own (though this is changing), there was no need to fire the events for them.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 449237 [details] [diff] [review]
Patch to send DOM events for modal dialog when a file picker is shown

based on above comments, r-

Comment 10

[Steffen Imhof]

Attachment: Updated patch with changes for GTK, Qt, Windows and Mac and some minor updates

I changed the following things in the patch

 - updated to latest m-c
 - did the switch to ShowNative() also for GTK, Windows and Mac (only GTK is tested though)
 - added a note to the documentation of nsIFilePicker.idl
 - checked for IsSafeToRunScript() before dispatching the events

I would have liked to also add an automated test, but I have trouble closing the file dialog to complete the whole process. The file dialog usually is a native modal dialog with its own event loop, so it seems utils.sendKeyEvent($ESCAPE) or something does not work. There is a sendNativeKeyEvent which _might_ work, but the implementation is missing for GTK and Qt. If you have any (better) ideas, please let me hear them.

(For review I still don't really know whom to ask, as that patch touches many areas, I'm just starting with Olli, feel free to push the review around as you see fit)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 461141 [details] [diff] [review]
Updated patch with changes for GTK, Qt, Windows and Mac and some minor updates

This won't compile on non-libxul builds.
nsContentUtils is available only for gklayout.

And why is it safe to have raw nsIDOMWindow* member variable?
I think you should use nsWeakPtr.

Comment 12

[Steffen Imhof]

(In reply to comment #11)
> Comment on attachment 461141 [details] [diff] [review]
> Updated patch with changes for GTK, Qt, Windows and Mac and some minor updates
> 
> This won't compile on non-libxul builds.
> nsContentUtils is available only for gklayout.

Ok, could expand then a bit more on what you meant by comment #3? I obviously did not really understand tha, thanks.

> And why is it safe to have raw nsIDOMWindow* member variable?
> I think you should use nsWeakPtr.

I will fix that.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

You can use nsIContentUtils. But seems like the event is always dispatched at
safe time, as I said in comment 4, so you don't actually need to
use IsSafeToRunScript at all.

Comment 14

[Steffen Imhof]

Attachment: Patch without IsSafeToRunScript and with a weak pointer

Ah, excellent, somehow I missed the connection between comment #3 and #4.
I switched the raw pointer to a nsWeakPtr.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> Since filepickers couldn't just come up on their own (though this is changing)

Where is it changing, out of curiosity?

I don't think we should do this until it becomes necessary for the tab-selecting functionality. There are other existing ways to detect dialogs opening (e.g. nsIWindowWatcher::registerNotification) that can be used instead.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 461253 [details]
Patch without IsSafeToRunScript and with a weak pointer

r- per comment 15

Comment 17

[Steffen Imhof]

I asked the guys who had the initial request to support these events to check if they can use nsIWindowWatcher. But it seems they don't get a notification in this case:

> I tried this both on content side and chrome side:
> ---------8<---------------------
>
>    this.windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
>               .getService(Ci.nsIWindowWatcher);
>    this.windowWatcher.registerNotification(this);
>
> ---------8<---------------------
>  /**
>   * observe
>   */
>  observe: function observe(subject, topic, data) {
>    dump("WW:::observe(): subject=" + subject + " topic='" + topic + "'
> data='" + data + "'\n");
>    switch (topic) {
>      case "domwindowopened":
>        sendAsyncMessage("MyPlugin:DOMWillOpenModalDialog", {});
>        break;
>    }
> ---------8<---------------------
>
> But "domwindowopened" message is never received (tested with picpaste.com).
>
> So if I haven't done anything stupidly, it seems that 
> DOMWindowWillOpenModalDialog event cannot be replaced with domwindowopened
> message,

So is there anything inherently wrong with the approach above?

Comment 18

[Steffen Imhof]

Do you have any input on the usage of nsIWindowWatcher please? Is that not enough info, do you need more?

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Yeah, I was wrong. We'll only send the windowWatcher notification if we're actually opening a Gecko window, which I don't think happens anymore on any supported platform (it used to on Linux, and can still be triggered by setting ui.allow_platform_file_picker to false).

The current Firefox DOMWillOpenModalDialog handler will throw an exception if the event target is a chrome window, which would presumably be possible with that patch (File->Open from the application menu). We could of course fix that handler, but there are a bunch of other clients that also handle that event that we'd need to consider as well. I wonder whether it wouldn't just be safer to use a new event for this, to avoid the compatibility impact.

Comment 20

[Steffen Imhof]

(In reply to comment #19)

> The current Firefox DOMWillOpenModalDialog handler will throw an exception if
> the event target is a chrome window, which would presumably be possible with
> that patch (File->Open from the application menu). We could of course fix that
> handler, but there are a bunch of other clients that also handle that event
> that we'd need to consider as well. I wonder whether it wouldn't just be safer
> to use a new event for this, to avoid the compatibility impact.

That would of course work nicely for the use-case I have in mind.

I am not sure, where else this event would/should be used, what do you think about a name pair like "DOMWillOpenFileDialog"/"DOMFileDialogClosed"? Or should it be more like "DOMWillOpenNativeDialog"/"DOMNativeDialogClosed"?

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.