Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1045443002:
Make File-Picker modal on Linux (Closed)
DescriptionMake File-Picker modal on Linux
Chromium for Linux opens a Gtk file-picker for file browsing, but it is not
modal because there is no GtkWindow as parent of file-picker.
This patch allows the X11 host window to disable input event handling to make
a file-picker modal.
BUG=408481
Committed: https://crrev.com/b440cbb929e62a238f97590d79bd78333adef7ce
Cr-Commit-Position: refs/heads/master@{#338870}
Patch Set 1 #Patch Set 2 : Applied ScopedWindowTargeter to block events #
Total comments: 3
Patch Set 3 : allow each host window to open a dialog #Patch Set 4 : rebase on master #
Total comments: 17
Patch Set 5 : Removed modal_dialog_xid_ in X11DesktopHandler and fixed others. #
Total comments: 2
Patch Set 6 : Updated comments and moved input-focus code #
Total comments: 2
Patch Set 7 : Do not call HandleNativeWidgetActivationChanged() #
Total comments: 7
Patch Set 8 : Updated for landing #
Messages
Total messages: 37 (9 generated)
joone.hur@intel.com changed reviewers: + erg@chromium.org
erg@ I tried to make a GtkWidget or GtkWindow with XID of the X11 host window, but there seems no solution. Instead, I could block input events on the X11 host window while a modal GTK+ file-picker is visible.
On 2015/03/27 23:25:46, joone wrote: > erg@ I tried to make a GtkWidget or GtkWindow with XID of the X11 host window, > but there seems no solution. Instead, I could block input events on the X11 host > window while a modal GTK+ file-picker is visible. I think this the wrong fix. A better fix would be to implement some of the behaviour, e.g. if the chrome window receives input events, or is raised, the file-select dialog should be immediately stacked above it. To redirect/block events, you should look into using ScopedWindowTargeter.
On 2015/03/27 23:34:46, sadrul wrote: > On 2015/03/27 23:25:46, joone wrote: > > erg@ I tried to make a GtkWidget or GtkWindow with XID of the X11 host window, > > but there seems no solution. Instead, I could block input events on the X11 > host > > window while a modal GTK+ file-picker is visible. > > I think this the wrong fix. A better fix would be to implement some of the > behaviour, e.g. if the chrome window receives input events, or is raised, the > file-select dialog should be immediately stacked above it. > > To redirect/block events, you should look into using ScopedWindowTargeter. sadrul@ I've updated the patch according to your suggestion, which made the CL simple.
joone.hur@intel.com changed reviewers: + sadrul@chromium.org
sadrul@ Could you review the updated patch?
https://codereview.chromium.org/1045443002/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1045443002/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:278: gtk_widget_get_window(dialog))); This makes it modal for all chrome windows. Shouldn't this be modal to only a particular chrome window (|owning_window|)? https://codereview.chromium.org/1045443002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:268: scoped_targeter_.reset(new aura::ScopedWindowTargeter(host->window(), the indent here is wrong.
https://codereview.chromium.org/1045443002/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1045443002/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:278: gtk_widget_get_window(dialog))); On 2015/04/21 19:50:01, sadrul wrote: > This makes it modal for all chrome windows. Shouldn't this be modal to only a > particular chrome window (|owning_window|)? Yes, dialog should be only modal to a particular chrome window. But, the upstream code has a focus problem with multiple modal dialogs: all modal dialogs can't get any events except only one. When we open multiple chrome windows, my patch can launch only one dialog so I will fix it.
sadrul@ please take a look at the updated patch.
https://codereview.chromium.org/1045443002/diff/60001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1045443002/diff/60001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:280: GetAuraTransientParent(dialog)->GetHost()); Looks like GetAuraTransientParent() could return null? (e.g. for open dialogs?) https://codereview.chromium.org/1045443002/diff/60001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:280: GetAuraTransientParent(dialog)->GetHost()); The static cast here is not great. Maybe you could use DesktopWindowTreeHostX11::GetHostForXID instead? https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2037: modal_dialog_xid_ = dialog; DCHECK here that modal_dialog_xid_ is None. https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2045: modal_dialog_xid_ = 0; DCHECK here that modal_dialog_xid_ != None https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:90: void DisableEventListening(XID dialog); document https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:355: scoped_ptr<aura::ScopedWindowTargeter> scoped_targeter_; Call this |targeter_for_modal_| (or something like that), so it's clear that this is only active for a modal dialog. https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:177: CurrentTime); Is it necessary to maintain a separate modal_dialog_xid_ here? Can you not query the DWTHX11 here instead? https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:229: modal_dialog_xid_ = new_host->GetModalDialog(); Should you set the input-focus here instead?
Thanks for the review. Please take a look at the updated CL. https://codereview.chromium.org/1045443002/diff/60001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc (right): https://codereview.chromium.org/1045443002/diff/60001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:280: GetAuraTransientParent(dialog)->GetHost()); On 2015/06/30 17:30:56, sadrul wrote: > The static cast here is not great. Maybe you could use > DesktopWindowTreeHostX11::GetHostForXID instead? Yes, we could get the DWTHX as follows: views::DesktopWindowTreeHostX11* host = views::DesktopWindowTreeHostX11::GetHostForXID( owning_window->GetHost()->GetAcceleratedWidget()); https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2037: modal_dialog_xid_ = dialog; On 2015/06/30 17:30:56, sadrul wrote: > DCHECK here that modal_dialog_xid_ is None. Done. https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2045: modal_dialog_xid_ = 0; On 2015/06/30 17:30:56, sadrul wrote: > DCHECK here that modal_dialog_xid_ != None Done. https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:90: void DisableEventListening(XID dialog); On 2015/06/30 17:30:56, sadrul wrote: > document Done. https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:355: scoped_ptr<aura::ScopedWindowTargeter> scoped_targeter_; On 2015/06/30 17:30:56, sadrul wrote: > Call this |targeter_for_modal_| (or something like that), so it's clear that > this is only active for a modal dialog. Done. https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:177: CurrentTime); On 2015/06/30 17:30:56, sadrul wrote: > Is it necessary to maintain a separate modal_dialog_xid_ here? Can you not query > the DWTHX11 here instead? Yes, we can query the DWTHX11. https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:229: modal_dialog_xid_ = new_host->GetModalDialog(); On 2015/06/30 17:30:56, sadrul wrote: > Should you set the input-focus here instead? The file dialog doesn't pop-up well when opening two browser windows.
https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:229: modal_dialog_xid_ = new_host->GetModalDialog(); On 2015/07/01 03:16:06, joone wrote: > On 2015/06/30 17:30:56, sadrul wrote: > > Should you set the input-focus here instead? > > The file dialog doesn't pop-up well when opening two browser windows. Can you clarify what you mean? It should be OK to have two browser windows, open a modal dialog for one of them, and then switch between the other browser window, and this modal+browser window, right? So shouldn't we want to make sure the other browser window does get deactivated correctly (ie. run the code in lines 213:218 above to deactivate the other browser window, and then instead of activating this browser window, set input-focus to the modal dialog)? https://codereview.chromium.org/1045443002/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): https://codereview.chromium.org/1045443002/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:91: void DisableEventListening(XID dialog); Document |dialog|, and what the function is supposed to do. You don't need to document what other functions call this. (applies to the rest of the functions too)
Please review the updated CL. Thanks! https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_desktop_handler.cc:229: modal_dialog_xid_ = new_host->GetModalDialog(); On 2015/07/01 03:58:14, sadrul wrote: > On 2015/07/01 03:16:06, joone wrote: > > On 2015/06/30 17:30:56, sadrul wrote: > > > Should you set the input-focus here instead? > > > > The file dialog doesn't pop-up well when opening two browser windows. > > Can you clarify what you mean? It should be OK to have two browser windows, open > a modal dialog for one of them, and then switch between the other browser > window, and this modal+browser window, right? So shouldn't we want to make sure > the other browser window does get deactivated correctly (ie. run the code in > lines 213:218 above to deactivate the other browser window, and then instead of > activating this browser window, set input-focus to the modal dialog)? Yes, each window can have its own modal dialog and they(window or window+modal dialog) can be switched among each other. Now, it is okay to set the input-focus here. There might have been a mistake to test this fix. https://codereview.chromium.org/1045443002/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): https://codereview.chromium.org/1045443002/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:91: void DisableEventListening(XID dialog); On 2015/07/01 03:58:14, sadrul wrote: > Document |dialog|, and what the function is supposed to do. You don't need to > document what other functions call this. (applies to the rest of the functions > too) Done.
https://codereview.chromium.org/1045443002/diff/100001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/100001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler.cc:220: new_host->HandleNativeWidgetActivationChanged(true); We don't actually want to call this if |new_host| has a modal dialog, right?
https://codereview.chromium.org/1045443002/diff/100001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/100001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler.cc:220: new_host->HandleNativeWidgetActivationChanged(true); On 2015/07/08 15:02:31, sadrul wrote: > We don't actually want to call this if |new_host| has a modal dialog, right? Yes, we may not need to call this. I will test it.
https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler.cc:238: We don't call new_host->HandleNativeWidgetActivationChanged(true) when |new_host| has a modal dialog
A few nits, other than that, lgtm. Would it be possible to write tests for this? Maybe in a follow-up CL? https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:91: void DisableEventListening(XID dialog); Remove the comment about |dialog| being a GTK dialog. It could just as well be any X11 window, right? https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:98: XID GetModalDialog(); make this const https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler.cc:237: new_host->HandleNativeWidgetActivationChanged(true); Use {} for both cases.
Thanks for the review! https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:91: void DisableEventListening(XID dialog); On 2015/07/14 08:13:01, sadrul wrote: > Remove the comment about |dialog| being a GTK dialog. It could just as well be > any X11 window, right? Done. https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:98: XID GetModalDialog(); On 2015/07/14 08:13:01, sadrul wrote: > make this const Done. https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/1045443002/diff/120001/ui/views/widget/deskto... ui/views/widget/desktop_aura/x11_desktop_handler.cc:237: new_host->HandleNativeWidgetActivationChanged(true); On 2015/07/14 08:13:01, sadrul wrote: > Use {} for both cases. Done.
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1045443002/#ps140001 (title: "Updated for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1045443002/140001
The CQ bit was unchecked by joone.hur@intel.com
On 2015/07/14 08:13:02, sadrul wrote: > A few nits, other than that, lgtm. > > Would it be possible to write tests for this? Maybe in a follow-up CL? > Do you want me to make a separate CL? or include a test case in this issue?
On 2015/07/14 22:41:26, joone wrote: > On 2015/07/14 08:13:02, sadrul wrote: > > A few nits, other than that, lgtm. > > > > Would it be possible to write tests for this? Maybe in a follow-up CL? > > > > Do you want me to make a separate CL? or include a test case in this issue? Either works for me.
The CQ bit was checked by joone.hur@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1045443002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/07/15 06:43:02, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) sadrul@ I'd like to land this CL first, but I need another LGTM from the Gtk owners(erg@, estade@).
joone.hur@intel.com changed reviewers: + estade@chromium.org
erg@, estade@ Could you review this CL?
The CQ bit was checked by erg@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1045443002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b440cbb929e62a238f97590d79bd78333adef7ce Cr-Commit-Position: refs/heads/master@{#338870}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1243503002/ by juncai@chromium.org. The reason for reverting is: This patch causes DCHECK failure. Refer to: https://code.google.com/p/chromium/issues/detail?id=510957. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
