Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1214933006:
Do not change the default Z-order for a bubble when it appears for an active window. (Closed)
DescriptionDo not change the default Z-order for a bubble when it appears for an active window.
When the bubble parent is active, no special treatment required. The bubble becomes the topmost window automatically.
When the parent is inactive, the bubble is placed just above the parent so the other windows aren't overlapped.
The CL is a follow-up for https://codereview.chromium.org/1162883003
BUG=504307
Patch Set 1 #
Total comments: 6
Depends on Patchset: Messages
Total messages: 13 (2 generated)
vasilii@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, please review. The dependence on another patchset is a bug.
https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate.cc:47: Widget* parent = Widget::GetWidgetForNativeView(bubble_params.parent); Why do we need to special case this? Shouldn't we end up with the same result?
https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate.cc:47: Widget* parent = Widget::GetWidgetForNativeView(bubble_params.parent); On 2015/06/30 16:43:13, sky wrote: > Why do we need to special case this? Shouldn't we end up with the same result? The original bug http://crbug.com/486730 was that the bubble from a background window suddenly appears in foreground. In this patch I address precisely the case of inactive window. For the active window the stacking remains correct as before.
https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate.cc:47: Widget* parent = Widget::GetWidgetForNativeView(bubble_params.parent); On 2015/06/30 18:57:03, vasilii wrote: > On 2015/06/30 16:43:13, sky wrote: > > Why do we need to special case this? Shouldn't we end up with the same result? > > The original bug http://crbug.com/486730 was that the bubble from a background > window suddenly appears in foreground. In this patch I address precisely the > case of inactive window. For the active window the stacking remains correct as > before. My question is why it matters. Won't the same thing happen if you leave the code as it was?
https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate.cc:47: Widget* parent = Widget::GetWidgetForNativeView(bubble_params.parent); On 2015/06/30 22:07:54, sky wrote: > On 2015/06/30 18:57:03, vasilii wrote: > > On 2015/06/30 16:43:13, sky wrote: > > > Why do we need to special case this? Shouldn't we end up with the same > result? > > > > The original bug http://crbug.com/486730 was that the bubble from a background > > window suddenly appears in foreground. In this patch I address precisely the > > case of inactive window. For the active window the stacking remains correct as > > before. > > My question is why it matters. Won't the same thing happen if you leave the code > as it was? No, just watch the video from http://crbug.com/504307 that is the corresponding bug for this CL.
https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate.cc:47: Widget* parent = Widget::GetWidgetForNativeView(bubble_params.parent); On 2015/07/01 09:29:40, vasilii wrote: > On 2015/06/30 22:07:54, sky wrote: > > On 2015/06/30 18:57:03, vasilii wrote: > > > On 2015/06/30 16:43:13, sky wrote: > > > > Why do we need to special case this? Shouldn't we end up with the same > > result? > > > > > > The original bug http://crbug.com/486730 was that the bubble from a > background > > > window suddenly appears in foreground. In this patch I address precisely the > > > case of inactive window. For the active window the stacking remains correct > as > > > before. > > > > My question is why it matters. Won't the same thing happen if you leave the > code > > as it was? > > No, just watch the video from http://crbug.com/504307 that is the corresponding > bug for this CL. Sorry, but you're still not explaining why this code is needed. We always want the bubble on top of the parent (if there is one) regardless of active status. What is StackAbove doing wrong in the case of an inactive window?
https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/1214933006/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate.cc:47: Widget* parent = Widget::GetWidgetForNativeView(bubble_params.parent); On 2015/07/01 16:35:17, sky (OOO until 7-20) wrote: > On 2015/07/01 09:29:40, vasilii wrote: > > On 2015/06/30 22:07:54, sky wrote: > > > On 2015/06/30 18:57:03, vasilii wrote: > > > > On 2015/06/30 16:43:13, sky wrote: > > > > > Why do we need to special case this? Shouldn't we end up with the same > > > result? > > > > > > > > The original bug http://crbug.com/486730 was that the bubble from a > > background > > > > window suddenly appears in foreground. In this patch I address precisely > the > > > > case of inactive window. For the active window the stacking remains > correct > > as > > > > before. > > > > > > My question is why it matters. Won't the same thing happen if you leave the > > code > > > as it was? > > > > No, just watch the video from http://crbug.com/504307 that is the > corresponding > > bug for this CL. > > Sorry, but you're still not explaining why this code is needed. We always want > the bubble on top of the parent (if there is one) regardless of active status. > What is StackAbove doing wrong in the case of an inactive window? The user opened a profile chooser. Then he opens the zoom bubble. The code places the bubble right above the parent and below the profile chooser which looks strange. Instead it should be placed at the default top-most position.
vasilii@chromium.org changed reviewers: + msw@chromium.org
Hi Mike, please review.
On 2015/07/02 19:56:23, vasilii wrote: > Hi Mike, > > please review. I'm not convinced this is the right fix either... I don't know that one z-ordering is preferable over the other here, and really, I'd suspect that the defect ought to be resolved in another way altogether (eg. close the profile bubble when the zoom bubble is triggered, prevent zoom while the profile bubble is showing, etc.). I'll CC groby@ on the bug, since she is taking a higher-level look at bubble management.
On 2015/07/06 18:26:19, msw wrote: > On 2015/07/02 19:56:23, vasilii wrote: > > Hi Mike, > > > > please review. > > I'm not convinced this is the right fix either... I don't know that one > z-ordering is preferable over the other here, and really, I'd suspect that the > defect ought to be resolved in another way altogether (eg. close the profile > bubble when the zoom bubble is triggered, prevent zoom while the profile bubble > is showing, etc.). I'll CC groby@ on the bug, since she is taking a higher-level > look at bubble management. It's a temporary fix while we don't have a bubble manager.
On 2015/07/07 08:47:16, vasilii wrote: > On 2015/07/06 18:26:19, msw wrote: > > On 2015/07/02 19:56:23, vasilii wrote: > > > Hi Mike, > > > > > > please review. > > > > I'm not convinced this is the right fix either... I don't know that one > > z-ordering is preferable over the other here, and really, I'd suspect that the > > defect ought to be resolved in another way altogether (eg. close the profile > > bubble when the zoom bubble is triggered, prevent zoom while the profile > bubble > > is showing, etc.). I'll CC groby@ on the bug, since she is taking a > higher-level > > look at bubble management. > > It's a temporary fix while we don't have a bubble manager. I don't think a temporary fix is necessary. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
