Open Bug 570115 Opened 12 years ago Updated 7 years ago

Showing FilePicker dialogs does not send DOMWillOpenModalDialog

Categories

(Core :: DOM: Events, defect)

x86
Linux
defect
Not set
normal

Tracking

()

People

(Reporter: steffen.imhof, Assigned: steffen.imhof)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Assignee: nobody → steffen.imhof
Attachment #449237 - Flags: review?(dougt)
Attachment #449237 - Flags: review?(dougt) → review?(vladimir)
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 :/
Make sure the events are dispatched when nsIContentUtils::IsSafeToRunScript() returns true.
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.
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.
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?
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.
Duplicate of this bug: 582391
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-
Attachment #449237 - Flags: review?(vladimir) → review-
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)
Attachment #449237 - Attachment is obsolete: true
Attachment #461141 - Flags: review?(Olli.Pettay)
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.
Attachment #461141 - Flags: review?(Olli.Pettay) → review-
(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.
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.
Ah, excellent, somehow I missed the connection between comment #3 and #4.
I switched the raw pointer to a nsWeakPtr.
Attachment #461141 - Attachment is obsolete: true
Attachment #461253 - Flags: review?(gavin.sharp)
(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.
Blocks: 584217
Comment on attachment 461253 [details]
Patch without IsSafeToRunScript and with a weak pointer

r- per comment 15
Attachment #461253 - Flags: review?(gavin.sharp) → review-
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?
Do you have any input on the usage of nsIWindowWatcher please? Is that not enough info, do you need more?
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.
(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"?
You need to log in before you can comment on or make changes to this bug.