Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
65733002:
Make ColorChooserAura browser modal, not always-on-top. (Closed)
|
Created:
7 years, 1 month ago by msw Modified:
7 years, 1 month ago CC:
chromium-reviews, tfarina, Ben Goodger (Google), keishi Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake ColorChooserAura browser modal, not always-on-top.
Parent the chooser widget to the content's browser window.
(ColorChooserView already returns ui::MODAL_TYPE_WINDOW)
Make it no longer always-on-top (has been since r144111).
BUG=295832, 295832
TEST=The Color chooser is browser modal, not always on top; see an example at <http://github.hubspot.com/pace/docs/welcome>;.
R=mukai@chromium.org,pkasting@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234615
Patch Set 1 #
Total comments: 8
Patch Set 2 : Make the chooser browser window modal. #Messages
Total messages: 17 (0 generated)
Hey Jun and Peter, please take a look; thanks!
I think you meant http://crrev.com/144111 . https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... File chrome/browser/ui/views/color_chooser_aura.cc (left): https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); I thought the original motivation for this was that this blocked the webpage, or something. Is the entire pipeline here fully non-blocking at this point?
Thanks for the link correction, I updated the CL description. https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... File chrome/browser/ui/views/color_chooser_aura.cc (left): https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); On 2013/11/07 23:37:40, Peter Kasting wrote: > I thought the original motivation for this was that this blocked the webpage, or > something. > > Is the entire pipeline here fully non-blocking at this point? It's non-blocking and non-modal all around on ToT afaict. You can interact with the browser, content, and other windows. Clicking a color or outside the window closes the dialog.
https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... File chrome/browser/ui/views/color_chooser_aura.cc (left): https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); On 2013/11/07 23:43:23, msw wrote: > On 2013/11/07 23:37:40, Peter Kasting wrote: > > I thought the original motivation for this was that this blocked the webpage, > or > > something. > > > > Is the entire pipeline here fully non-blocking at this point? > > It's non-blocking and non-modal all around on ToT afaict. > You can interact with the browser, content, and other windows. > Clicking a color or outside the window closes the dialog. clicking outside doesn't close the dialog. So if it's not always-on-top, the dialog can be hidden and not sure how to get it back.
adding keishi as cc
https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... File chrome/browser/ui/views/color_chooser_aura.cc (left): https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); On 2013/11/07 23:43:23, msw wrote: > On 2013/11/07 23:37:40, Peter Kasting wrote: > > I thought the original motivation for this was that this blocked the webpage, > or > > something. > > > > Is the entire pipeline here fully non-blocking at this point? > > It's non-blocking and non-modal all around on ToT afaict. > You can interact with the browser, content, and other windows. > Clicking a color or outside the window closes the dialog. Correction: The window only automatically closes on navigation or closing the tab, otherwise it only closes through an explicit window closing action.
https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... File chrome/browser/ui/views/color_chooser_aura.cc (left): https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); On 2013/11/07 23:46:57, Jun Mukai wrote: > On 2013/11/07 23:43:23, msw wrote: > > On 2013/11/07 23:37:40, Peter Kasting wrote: > > > I thought the original motivation for this was that this blocked the > webpage, > > or > > > something. > > > > > > Is the entire pipeline here fully non-blocking at this point? > > > > It's non-blocking and non-modal all around on ToT afaict. > > You can interact with the browser, content, and other windows. > > Clicking a color or outside the window closes the dialog. > > clicking outside doesn't close the dialog. > So if it's not always-on-top, the dialog can be hidden and not sure how to get > it back. Yeah, sorry I was wrong about the auto close on clicking away. If you click away, you can switch back to the chooser window just by pressing Alt+Tab, or by clicking the window (moving any windows obscuring the chooser as needed).
https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... File chrome/browser/ui/views/color_chooser_aura.cc (left): https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); On 2013/11/07 23:49:42, msw wrote: > On 2013/11/07 23:43:23, msw wrote: > > On 2013/11/07 23:37:40, Peter Kasting wrote: > > > I thought the original motivation for this was that this blocked the > webpage, > > or > > > something. > > > > > > Is the entire pipeline here fully non-blocking at this point? > > > > It's non-blocking and non-modal all around on ToT afaict. > > You can interact with the browser, content, and other windows. > > Clicking a color or outside the window closes the dialog. > > Correction: The window only automatically closes on navigation or closing the > tab, otherwise it only closes through an explicit window closing action. Yes, I think so. And I'm fine to remove always-on-top, but then probably better to make it as a modal window?
https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... File chrome/browser/ui/views/color_chooser_aura.cc (left): https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); On 2013/11/07 23:54:57, Jun Mukai wrote: > On 2013/11/07 23:49:42, msw wrote: > > On 2013/11/07 23:43:23, msw wrote: > > > On 2013/11/07 23:37:40, Peter Kasting wrote: > > > > I thought the original motivation for this was that this blocked the > > webpage, > > > or > > > > something. > > > > > > > > Is the entire pipeline here fully non-blocking at this point? > > > > > > It's non-blocking and non-modal all around on ToT afaict. > > > You can interact with the browser, content, and other windows. > > > Clicking a color or outside the window closes the dialog. > > > > Correction: The window only automatically closes on navigation or closing the > > tab, otherwise it only closes through an explicit window closing action. > > Yes, I think so. > And I'm fine to remove always-on-top, but then probably better to make it as a > modal window? Hmm, any reason why it should be modal? Modal to the content? browser window? system?
https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... File chrome/browser/ui/views/color_chooser_aura.cc (left): https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); On 2013/11/07 23:59:55, msw wrote: > On 2013/11/07 23:54:57, Jun Mukai wrote: > > On 2013/11/07 23:49:42, msw wrote: > > > On 2013/11/07 23:43:23, msw wrote: > > > > On 2013/11/07 23:37:40, Peter Kasting wrote: > > > > > I thought the original motivation for this was that this blocked the > > > webpage, > > > > or > > > > > something. > > > > > > > > > > Is the entire pipeline here fully non-blocking at this point? > > > > > > > > It's non-blocking and non-modal all around on ToT afaict. > > > > You can interact with the browser, content, and other windows. > > > > Clicking a color or outside the window closes the dialog. > > > > > > Correction: The window only automatically closes on navigation or closing > the > > > tab, otherwise it only closes through an explicit window closing action. > > > > Yes, I think so. > > And I'm fine to remove always-on-top, but then probably better to make it as a > > modal window? > > Hmm, any reason why it should be modal? > Modal to the content? browser window? system? I thought browser-modal. I'm just afraid that the dialog can be lost (hidden under the windows pile and almost impossible to find again).
On 2013/11/08 00:13:25, Jun Mukai wrote: > https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... > File chrome/browser/ui/views/color_chooser_aura.cc (left): > > https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... > chrome/browser/ui/views/color_chooser_aura.cc:60: widget_->SetAlwaysOnTop(true); > On 2013/11/07 23:59:55, msw wrote: > > On 2013/11/07 23:54:57, Jun Mukai wrote: > > > On 2013/11/07 23:49:42, msw wrote: > > > > On 2013/11/07 23:43:23, msw wrote: > > > > > On 2013/11/07 23:37:40, Peter Kasting wrote: > > > > > > I thought the original motivation for this was that this blocked the > > > > webpage, > > > > > or > > > > > > something. > > > > > > > > > > > > Is the entire pipeline here fully non-blocking at this point? > > > > > > > > > > It's non-blocking and non-modal all around on ToT afaict. > > > > > You can interact with the browser, content, and other windows. > > > > > Clicking a color or outside the window closes the dialog. > > > > > > > > Correction: The window only automatically closes on navigation or closing > > the > > > > tab, otherwise it only closes through an explicit window closing action. > > > > > > Yes, I think so. > > > And I'm fine to remove always-on-top, but then probably better to make it as > a > > > modal window? > > > > Hmm, any reason why it should be modal? > > Modal to the content? browser window? system? > > I thought browser-modal. I'm just afraid that the dialog can be lost (hidden > under the windows pile and almost impossible to find again). Does it appear as its own window in the taskbar? If not, does it automatically come above its parent window when the parent window is activated? If either of these are true, no modality is needed. If neither is true, we should consider trying to make this window "appear above" the owning window (there's some windows Z-order position that means "above window X, but not necessarily always on top").
On 2013/11/08 00:21:27, Peter Kasting wrote: > On 2013/11/08 00:13:25, Jun Mukai wrote: > > > https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... > > File chrome/browser/ui/views/color_chooser_aura.cc (left): > > > > > https://codereview.chromium.org/65733002/diff/1/chrome/browser/ui/views/color... > > chrome/browser/ui/views/color_chooser_aura.cc:60: > widget_->SetAlwaysOnTop(true); > > On 2013/11/07 23:59:55, msw wrote: > > > On 2013/11/07 23:54:57, Jun Mukai wrote: > > > > On 2013/11/07 23:49:42, msw wrote: > > > > > On 2013/11/07 23:43:23, msw wrote: > > > > > > On 2013/11/07 23:37:40, Peter Kasting wrote: > > > > > > > I thought the original motivation for this was that this blocked the > > > > > webpage, > > > > > > or > > > > > > > something. > > > > > > > > > > > > > > Is the entire pipeline here fully non-blocking at this point? > > > > > > > > > > > > It's non-blocking and non-modal all around on ToT afaict. > > > > > > You can interact with the browser, content, and other windows. > > > > > > Clicking a color or outside the window closes the dialog. > > > > > > > > > > Correction: The window only automatically closes on navigation or > closing > > > the > > > > > tab, otherwise it only closes through an explicit window closing action. > > > > > > > > Yes, I think so. > > > > And I'm fine to remove always-on-top, but then probably better to make it > as > > a > > > > modal window? > > > > > > Hmm, any reason why it should be modal? > > > Modal to the content? browser window? system? > > > > I thought browser-modal. I'm just afraid that the dialog can be lost (hidden > > under the windows pile and almost impossible to find again). > > Does it appear as its own window in the taskbar? If not, does it automatically > come above its parent window when the parent window is activated? > > If either of these are true, no modality is needed. If neither is true, we > should consider trying to make this window "appear above" the owning window > (there's some windows Z-order position that means "above window X, but not > necessarily always on top"). At least it doesn't appear on the shelf on ChromeOS. But possibly clicking the same <input type="color"> again would re-appear the dialog, so it may not be a problem.
Okay, I made ColorChooserAura modal to the browser window, like the native picker on Windows. It's always on top of the browser window to which it belongs, and blocks interaction there until it's closed, but will go behind other browser windows and still allows interaction in other browser windows while active. Please take another look; thanks!
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/65733002/200001
Message was sent while issue was closed.
Change committed as 234615
Message was sent while issue was closed.
D'oh, I meant to mention BUG=315896 here... |
