Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1866403005:
Fix rtl shelf overflow bubble (Closed)
|
Created:
4 years, 8 months ago by Greg Levin Modified:
4 years, 7 months ago CC:
chromium-reviews, msw+watch_chromium.org, sadrul, tfarina, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, kalyank, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix rtl shelf overflow bubble
BUG=551712
TEST=Set Chrome OS with rtl language (e.g. Hebrew), position shelf on
left, add icons to shelf until it overflows. Click on overflow button,
observe that overflow shelf bubble is shown.
Committed: https://crrev.com/8e44a16c274b3cb9d1e6aa9d8cb96c7aa9b6d510
Cr-Commit-Position: refs/heads/master@{#390884}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Review: rename var, improve comment #
Total comments: 8
Patch Set 3 : Move inset swapping to BubbleBorder #
Total comments: 4
Patch Set 4 : Move fix from BubbleBorder back to BubbleFrameView #
Total comments: 2
Patch Set 5 : Swap lines of code #Patch Set 6 : Merge (bubble_delegate -> bubble_dialog_delegate) #Patch Set 7 : Add unit test #
Total comments: 2
Patch Set 8 : Move RTL adjustment (bubble_frame_view -> non_client_view) #
Total comments: 4
Patch Set 9 : Small review fixes (and merge) #
Messages
Total messages: 42 (8 generated)
glevin@chromium.org changed reviewers: + msw@chromium.org
msw@, I've just sent you an email with more details on what I'm trying to do with this CL. I expect it needs some work, but I'd like some advice on how to proceed. Please have a look! https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.h:75: bool ignore_rtl() const { return ignore_rtl_; } Don't actually need get method.
Also, check if BubbleBorder::GetInsets() is using the wrong arrow... https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... File ash/shelf/overflow_bubble_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... ash/shelf/overflow_bubble_view.cc:45: set_arrow(GetBubbleArrow()); Maybe just flip this arrow in RTL (for now, with a comment)?
> Also, check if BubbleBorder::GetInsets() is using the wrong arrow... BubbleBorder has the arrow that's passed in to "new BubbleBorder()" in BubbleDelegateView::CreateNonClientFrameView(), so it matches the actual arrow on the screen. My changes in this CL prevent RTL mirroring of this arrow for the overflow bubble, so it's LEFT_TOP for a left shelf. https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... File ash/shelf/overflow_bubble_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... ash/shelf/overflow_bubble_view.cc:45: set_arrow(GetBubbleArrow()); On 2016/04/08 19:23:08, msw wrote: > Maybe just flip this arrow in RTL (for now, with a comment)? A comment like, "We're flipping this arrow to point the wrong way here for RTL, because BubbleDelegateView::CreateNonClientFrameView() is also going to flip it, which will correct it." ? This approach seems to work: the delegate will store a backwards arrow, but it only uses that to pass it along to the BubbleBorder, and it gets reversed back to correct at that point. https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate.h (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate.h:193: bool ignore_rtl_; Reverse polarity, and rename |mirror_in_rtl| or |mirror_arrow_in_rtl|, with default = true. https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:125: // we need to ignore rtl for this bubble. This comment could be better. BubbleBorder::GetInsets() isn't doing anything wrong. The problem is that some code somewhere after GetBoundsForClientView() returns is mirroring the client bounds within the bubble bounds. If ignore_rtl_ is set, we don't want that. The code added here pre-mirrors it, so that the subsequent mirror results in a no-op.
I also wonder why the bubble is placed offscreen without any of these changes; why doesn't BubbleFrameView::MirrorArrowIfOffScreen do the right thing? https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... File ash/shelf/overflow_bubble_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... ash/shelf/overflow_bubble_view.cc:45: set_arrow(GetBubbleArrow()); On 2016/04/12 17:51:37, Greg Levin wrote: > On 2016/04/08 19:23:08, msw wrote: > > Maybe just flip this arrow in RTL (for now, with a comment)? > > A comment like, > "We're flipping this arrow to point the wrong way here for RTL, because > BubbleDelegateView::CreateNonClientFrameView() is also going to flip it, which > will correct it." > ? > > This approach seems to work: the delegate will store a backwards arrow, but it > only uses that to pass it along to the BubbleBorder, and it gets reversed back > to correct at that point. Either pre-flipping or adding |mirror_arrow_in_rtl| sgtm. https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:125: // we need to ignore rtl for this bubble. On 2016/04/12 17:51:37, Greg Levin wrote: > This comment could be better. BubbleBorder::GetInsets() isn't doing anything > wrong. The problem is that some code somewhere after GetBoundsForClientView() > returns is mirroring the client bounds within the bubble bounds. If ignore_rtl_ > is set, we don't want that. The code added here pre-mirrors it, so that the > subsequent mirror results in a no-op. It would be nice to find where/why GetBoundsForClientView() is flipped. Does setting View::EnableCanvasFlippingForRTLUI for BubbleFrameView help?
To clarify, there are two problems here (possibly related, but not directly): 1) RTL mirrors the bubble arrow. This moves the bubble off-screen, causing the original issue. My change in bubble_delegate.cc fixes this. 2) Even when I fix the arrow direction, something else is mirroring the client bound within the bubble bound (see Item 2 in https://bugs.chromium.org/p/chromium/issues/detail?id=551712#c4). My change in bubble_frame_view.cc fixes that. > I also wonder why the bubble is placed offscreen without any of these changes; > why doesn't BubbleFrameView::MirrorArrowIfOffScreen do the right thing? Because the BubbleOverflowView tells it not to (sets |adjust_if_offscreen| to false): https://code.google.com/p/chromium/codesearch#chromium/src/ash/shelf/overflow... Changing this to "true" moves the bubble back on-screen, but still leaves the arrow in the wrong place for the bottom shelf; see Item & image 1 in https://bugs.chromium.org/p/chromium/issues/detail?id=551712#c4 https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... File ash/shelf/overflow_bubble_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... ash/shelf/overflow_bubble_view.cc:45: set_arrow(GetBubbleArrow()); On 2016/04/12 18:58:19, msw wrote: > On 2016/04/12 17:51:37, Greg Levin wrote: > > On 2016/04/08 19:23:08, msw wrote: > > > Maybe just flip this arrow in RTL (for now, with a comment)? > > > > A comment like, > > "We're flipping this arrow to point the wrong way here for RTL, because > > BubbleDelegateView::CreateNonClientFrameView() is also going to flip it, which > > will correct it." > > ? > > > > This approach seems to work: the delegate will store a backwards arrow, but it > > only uses that to pass it along to the BubbleBorder, and it gets reversed back > > to correct at that point. > > Either pre-flipping or adding |mirror_arrow_in_rtl| sgtm. I like |mirror_arrow_in_rtl| better, because pre-flipping means that the delegate has the wrong arrow stored. This doesn't seem to matter in practice, because the delegate only uses it to pass to the frame, and it flips it back then. But storing correctly throughout seems better. https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:125: // we need to ignore rtl for this bubble. On 2016/04/12 18:58:19, msw wrote: > On 2016/04/12 17:51:37, Greg Levin wrote: > > This comment could be better. BubbleBorder::GetInsets() isn't doing anything > > wrong. The problem is that some code somewhere after GetBoundsForClientView() > > returns is mirroring the client bounds within the bubble bounds. If > ignore_rtl_ > > is set, we don't want that. The code added here pre-mirrors it, so that the > > subsequent mirror results in a no-op. > > It would be nice to find where/why GetBoundsForClientView() is flipped. It would be nice. But given that there are 27 functions in the call stack between CreateBubbleWidget() and BubbleFrameView::GetBoundsForClientView(), and none of that is bubble or shelf code, it could take a long time to find :-( Any advice on how I might proceed? > Does setting View::EnableCanvasFlippingForRTLUI for BubbleFrameView > help? It changes the problem, but doesn't really improve it. See Item 3 in https://bugs.chromium.org/p/chromium/issues/detail?id=551712#c4
https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... File ash/shelf/overflow_bubble_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... ash/shelf/overflow_bubble_view.cc:45: set_arrow(GetBubbleArrow()); On 2016/04/12 23:06:20, Greg Levin wrote: > On 2016/04/12 18:58:19, msw wrote: > > On 2016/04/12 17:51:37, Greg Levin wrote: > > > On 2016/04/08 19:23:08, msw wrote: > > > > Maybe just flip this arrow in RTL (for now, with a comment)? > > > > > > A comment like, > > > "We're flipping this arrow to point the wrong way here for RTL, because > > > BubbleDelegateView::CreateNonClientFrameView() is also going to flip it, > which > > > will correct it." > > > ? > > > > > > This approach seems to work: the delegate will store a backwards arrow, but > it > > > only uses that to pass it along to the BubbleBorder, and it gets reversed > back > > > to correct at that point. > > > > Either pre-flipping or adding |mirror_arrow_in_rtl| sgtm. > > I like |mirror_arrow_in_rtl| better, because pre-flipping means that the > delegate has the wrong arrow stored. This doesn't seem to matter in practice, > because the delegate only uses it to pass to the frame, and it flips it back > then. But storing correctly throughout seems better. Upload a patch set with |mirror_arrow_in_rtl|, and I'll review. https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:125: // we need to ignore rtl for this bubble. On 2016/04/12 23:06:20, Greg Levin wrote: > On 2016/04/12 18:58:19, msw wrote: > > On 2016/04/12 17:51:37, Greg Levin wrote: > > > This comment could be better. BubbleBorder::GetInsets() isn't doing > anything > > > wrong. The problem is that some code somewhere after > GetBoundsForClientView() > > > returns is mirroring the client bounds within the bubble bounds. If > > ignore_rtl_ > > > is set, we don't want that. The code added here pre-mirrors it, so that the > > > subsequent mirror results in a no-op. > > > > It would be nice to find where/why GetBoundsForClientView() is flipped. > > It would be nice. But given that there are 27 functions in the call stack > between CreateBubbleWidget() and BubbleFrameView::GetBoundsForClientView(), and > none of that is bubble or shelf code, it could take a long time to find :-( Any > advice on how I might proceed? > > > Does setting View::EnableCanvasFlippingForRTLUI for BubbleFrameView > > help? > > It changes the problem, but doesn't really improve it. See Item 3 in > https://bugs.chromium.org/p/chromium/issues/detail?id=551712#c4 Leave a more detailed comment here and this change will be okay.
Please have another look! https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... File ash/shelf/overflow_bubble_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ash/shelf/overflow_bubble_v... ash/shelf/overflow_bubble_view.cc:45: set_arrow(GetBubbleArrow()); On 2016/04/13 18:03:47, msw wrote: > On 2016/04/12 23:06:20, Greg Levin wrote: > > On 2016/04/12 18:58:19, msw wrote: > > > On 2016/04/12 17:51:37, Greg Levin wrote: > > > > On 2016/04/08 19:23:08, msw wrote: > > > > > Maybe just flip this arrow in RTL (for now, with a comment)? > > > > > > > > A comment like, > > > > "We're flipping this arrow to point the wrong way here for RTL, because > > > > BubbleDelegateView::CreateNonClientFrameView() is also going to flip it, > > which > > > > will correct it." > > > > ? > > > > > > > > This approach seems to work: the delegate will store a backwards arrow, > but > > it > > > > only uses that to pass it along to the BubbleBorder, and it gets reversed > > back > > > > to correct at that point. > > > > > > Either pre-flipping or adding |mirror_arrow_in_rtl| sgtm. > > > > I like |mirror_arrow_in_rtl| better, because pre-flipping means that the > > delegate has the wrong arrow stored. This doesn't seem to matter in practice, > > because the delegate only uses it to pass to the frame, and it flips it back > > then. But storing correctly throughout seems better. > > Upload a patch set with |mirror_arrow_in_rtl|, and I'll review. Done. https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_dele... File ui/views/bubble/bubble_delegate.h (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_dele... ui/views/bubble/bubble_delegate.h:193: bool ignore_rtl_; On 2016/04/12 17:51:37, Greg Levin wrote: > Reverse polarity, and rename |mirror_in_rtl| or |mirror_arrow_in_rtl|, with > default = true. Done. https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:125: // we need to ignore rtl for this bubble. On 2016/04/13 18:03:47, msw wrote: > On 2016/04/12 23:06:20, Greg Levin wrote: > > On 2016/04/12 18:58:19, msw wrote: > > > On 2016/04/12 17:51:37, Greg Levin wrote: > > > > This comment could be better. BubbleBorder::GetInsets() isn't doing > > anything > > > > wrong. The problem is that some code somewhere after > > GetBoundsForClientView() > > > > returns is mirroring the client bounds within the bubble bounds. If > > > ignore_rtl_ > > > > is set, we don't want that. The code added here pre-mirrors it, so that > the > > > > subsequent mirror results in a no-op. > > > > > > It would be nice to find where/why GetBoundsForClientView() is flipped. > > > > It would be nice. But given that there are 27 functions in the call stack > > between CreateBubbleWidget() and BubbleFrameView::GetBoundsForClientView(), > and > > none of that is bubble or shelf code, it could take a long time to find :-( > Any > > advice on how I might proceed? > > > > > Does setting View::EnableCanvasFlippingForRTLUI for BubbleFrameView > > > help? > > > > It changes the problem, but doesn't really improve it. See Item 3 in > > https://bugs.chromium.org/p/chromium/issues/detail?id=551712#c4 > > Leave a more detailed comment here and this change will be okay. Done. https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1866403005/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.h:75: bool ignore_rtl() const { return ignore_rtl_; } On 2016/04/08 19:02:21, Greg Levin wrote: > Don't actually need get method. Done. https://codereview.chromium.org/1866403005/diff/20001/ash/shelf/overflow_bubb... File ash/shelf/overflow_bubble_view.cc (right): https://codereview.chromium.org/1866403005/diff/20001/ash/shelf/overflow_bubb... ash/shelf/overflow_bubble_view.cc:62: AddChildView(shelf_view_); This change is from a merge
msw@chromium.org changed reviewers: + sky@chromium.org
lgtm with nits, but +sky for heads up / review. https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_delegate.h (right): https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_delegate.h:191: // Automatically mirror arrow in RTL layout. nit: "the arrow" https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:124: // After GetBoundsForClientView() returns, RTL code will swap these insets. This is an ugly hack for sure, but without knowing where the actual defect lies (if indeed the swap later is defective), then i can't imagine a better fix. +CC sky for heads up, he might have some insight or wish to block this workaround. https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.h:119: // Adjust insets to account for automatically mirrored arrow in RTL layout. optional nit: "for the mirrored"
https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:124: // After GetBoundsForClientView() returns, RTL code will swap these insets. On 2016/04/14 02:03:06, msw wrote: > This is an ugly hack for sure, but without knowing where the actual defect lies > (if indeed the swap later is defective), then i can't imagine a better fix. +CC > sky for heads up, he might have some insight or wish to block this workaround. This is definitely a hack and it should be fixed fixed in the right place. Is this behavior because of the rtl code in TrayBubbleView that needs to be disabled for this case?
https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:124: // After GetBoundsForClientView() returns, RTL code will swap these insets. On 2016/04/14 16:06:30, sky wrote: > On 2016/04/14 02:03:06, msw wrote: > > This is an ugly hack for sure, but without knowing where the actual defect > lies > > (if indeed the swap later is defective), then i can't imagine a better fix. > +CC > > sky for heads up, he might have some insight or wish to block this workaround. > > This is definitely a hack and it should be fixed fixed in the right place. Is > this behavior because of the rtl code in TrayBubbleView that needs to be > disabled for this case? Agree completely. The problem is that I can't find the right place :-P Is the overflow bubble in the shelf even using TrayBubbleView?
Not sure. You'll have to investigate that. On Thu, Apr 14, 2016 at 9:20 AM, <glevin@chromium.org> wrote: > > https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... > File ui/views/bubble/bubble_frame_view.cc (right): > > https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... > ui/views/bubble/bubble_frame_view.cc:124: // After > GetBoundsForClientView() returns, RTL code will swap these insets. > On 2016/04/14 16:06:30, sky wrote: >> On 2016/04/14 02:03:06, msw wrote: >> > This is an ugly hack for sure, but without knowing where the actual > defect >> lies >> > (if indeed the swap later is defective), then i can't imagine a > better fix. >> +CC >> > sky for heads up, he might have some insight or wish to block this > workaround. >> >> This is definitely a hack and it should be fixed fixed in the right > place. Is >> this behavior because of the rtl code in TrayBubbleView that needs to > be >> disabled for this case? > > Agree completely. The problem is that I can't find the right place :-P > > Is the overflow bubble in the shelf even using TrayBubbleView? > > https://codereview.chromium.org/1866403005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/14 16:32:56, sky wrote: > Not sure. You'll have to investigate that. > > On Thu, Apr 14, 2016 at 9:20 AM, <mailto:glevin@chromium.org> wrote: > > > > > https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... > > File ui/views/bubble/bubble_frame_view.cc (right): > > > > > https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... > > ui/views/bubble/bubble_frame_view.cc:124: // After > > GetBoundsForClientView() returns, RTL code will swap these insets. > > On 2016/04/14 16:06:30, sky wrote: > >> On 2016/04/14 02:03:06, msw wrote: > >> > This is an ugly hack for sure, but without knowing where the actual > > defect > >> lies > >> > (if indeed the swap later is defective), then i can't imagine a > > better fix. > >> +CC > >> > sky for heads up, he might have some insight or wish to block this > > workaround. > >> > >> This is definitely a hack and it should be fixed fixed in the right > > place. Is > >> this behavior because of the rtl code in TrayBubbleView that needs to > > be > >> disabled for this case? > > > > Agree completely. The problem is that I can't find the right place :-P > > > > Is the overflow bubble in the shelf even using TrayBubbleView? > > > > https://codereview.chromium.org/1866403005/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > I tested it, and TrayBubbleView::Create() is never called. So I don't think TrayBubbleView is involved at all. The problem is that there is almost no explicit RTL code in either /ash/shelf/ or /ui/views/bubble/. I'm fairly sure that this is the only such line that may be coming into play https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/bubble/bu... and my CL is already dealing with it.
Is the automatic mirroring when in rtl in views causing the problem? On Thu, Apr 14, 2016 at 9:47 AM, <glevin@chromium.org> wrote: > On 2016/04/14 16:32:56, sky wrote: >> Not sure. You'll have to investigate that. >> >> On Thu, Apr 14, 2016 at 9:20 AM, <mailto:glevin@chromium.org> wrote: >> > >> > >> > https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... >> > File ui/views/bubble/bubble_frame_view.cc (right): >> > >> > >> > https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... >> > ui/views/bubble/bubble_frame_view.cc:124: // After >> > GetBoundsForClientView() returns, RTL code will swap these insets. >> > On 2016/04/14 16:06:30, sky wrote: >> >> On 2016/04/14 02:03:06, msw wrote: >> >> > This is an ugly hack for sure, but without knowing where the actual >> > defect >> >> lies >> >> > (if indeed the swap later is defective), then i can't imagine a >> > better fix. >> >> +CC >> >> > sky for heads up, he might have some insight or wish to block this >> > workaround. >> >> >> >> This is definitely a hack and it should be fixed fixed in the right >> > place. Is >> >> this behavior because of the rtl code in TrayBubbleView that needs to >> > be >> >> disabled for this case? >> > >> > Agree completely. The problem is that I can't find the right place :-P >> > >> > Is the overflow bubble in the shelf even using TrayBubbleView? >> > >> > https://codereview.chromium.org/1866403005/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > I tested it, and TrayBubbleView::Create() is never called. So I don't think > TrayBubbleView is involved at all. > > The problem is that there is almost no explicit RTL code in either > /ash/shelf/ > or /ui/views/bubble/. I'm fairly sure that this is the only such line that > may > be coming into play > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/bubble/bu... > and my CL is already dealing with it. > > https://codereview.chromium.org/1866403005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/14 17:27:26, sky wrote: > Is the automatic mirroring when in rtl in views causing the problem? That is my guess, yes. For the overflow shelf, I don't believe that we want any mirroring behavior. Is there a way to disable mirroring for a view and all its children?
I don't believe so. If that is causing the problem, then the class that is returning the border should have a function to flip the insets. -Scott On Thu, Apr 14, 2016 at 10:32 AM, <glevin@chromium.org> wrote: > On 2016/04/14 17:27:26, sky wrote: >> Is the automatic mirroring when in rtl in views causing the problem? > > That is my guess, yes. For the overflow shelf, I don't believe that we want > any > mirroring behavior. Is there a way to disable mirroring for a view and all > its > children? > > https://codereview.chromium.org/1866403005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> I don't believe so. If that is causing the problem, then > the class that is returning the border should have a > function to flip the insets. I moved the inset-swapping into BubbleBorder. Note that doing a set-and-forget of swap_insets_rtl doesn't work (overflow shelf is offset incorrectly). We *only* want the inset swap to occur when GetBoundsForClientView() calls GetContentsBounds(), which is why swap is set and unset around this call. Does this look any better? https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_delegate.h (right): https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_delegate.h:191: // Automatically mirror arrow in RTL layout. On 2016/04/14 02:03:06, msw wrote: > nit: "the arrow" Done. https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1866403005/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.h:119: // Adjust insets to account for automatically mirrored arrow in RTL layout. On 2016/04/14 02:03:06, msw wrote: > optional nit: "for the mirrored" Done.
https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.cc:273: if (swap_insets_rtl_ && !is_arrow_on_horizontal(arrow_)) Combine this with previous if as it makes my heard thinking about the cases. https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:126: bubble_border_->set_swap_insets_rtl(true); This is awful. Could you instead plumb through context so it knows what the query is for? That said, I'm not sure I understand why the swap only needs to happen here. Can you elaborate on that?
Also, be sure and add test coverage.
https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border.cc:273: if (swap_insets_rtl_ && !is_arrow_on_horizontal(arrow_)) On 2016/04/19 19:07:44, sky wrote: > Combine this with previous if as it makes my heard thinking about the cases. I tried that, and it was even worse. The problem is, these two swaps aren't actually related. The first one simply puts the inset in the correct place based on the arrow. The second one needs to happen independently of the first, in the case that this is called by BubbleFrameView::GetBoundsForClientView(); it has to happen for left/right arrows when swap_insets_rtl_ is set, regardless of whether the first swap happened. Combining the logic of these two independent cases makes the single if nearly indecipherable. I could clarify the two with comments if this code ends up being kept. https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:126: bubble_border_->set_swap_insets_rtl(true); On 2016/04/19 19:07:44, sky wrote: > This is awful. Could you instead plumb through context so it knows what > the query is for? That said, I'm not sure I understand why the swap only > needs to happen here. Can you elaborate on that? I'm not sure what you mean by "plumb through context so it knows what the query is for". And I don't disagree that it's awful, and I don't understand why it only needs to happen here either. I just know, experimentally, that this was someplace I found where the swap fixed the problem, but doing it anywhere else in addition broke things again. I didn't really expect this version of the CL to land as is. It was just the only working solution that I found after 2 days of searching. I uploaded it to show about where the problem was, and was hoping someone could suggest a better approach based on that. When GetBoundsForClientView() is called, there are 25+ functions in the stack between it and the previous shelf/bubble function; everything inbetween is generic views/layout code. Somewhere after GetBoundsForClientView() returns, something down there reverses the insets under RTL, but I don't know the coordinate & layout code well enough to have a sense of where that might be.
Is it perhaps different depending upon whether the view is in a widget or not? I don't have any advice other than sitting down with a debugger and detecting when things go wrong. On Tue, Apr 19, 2016 at 1:04 PM, <glevin@chromium.org> wrote: > > https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... > File ui/views/bubble/bubble_border.cc (right): > > https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... > ui/views/bubble/bubble_border.cc:273: if (swap_insets_rtl_ && > !is_arrow_on_horizontal(arrow_)) > On 2016/04/19 19:07:44, sky wrote: >> Combine this with previous if as it makes my heard thinking about the > cases. > > I tried that, and it was even worse. The problem is, these two swaps > aren't actually related. The first one simply puts the inset in the > correct place based on the arrow. The second one needs to happen > independently of the first, in the case that this is called by > BubbleFrameView::GetBoundsForClientView(); it has to happen for > left/right arrows when swap_insets_rtl_ is set, regardless of whether > the first swap happened. > Combining the logic of these two independent cases makes the single if > nearly indecipherable. I could clarify the two with comments if this > code ends up being kept. > > https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... > File ui/views/bubble/bubble_frame_view.cc (right): > > https://codereview.chromium.org/1866403005/diff/40001/ui/views/bubble/bubble_... > ui/views/bubble/bubble_frame_view.cc:126: > bubble_border_->set_swap_insets_rtl(true); > On 2016/04/19 19:07:44, sky wrote: >> This is awful. Could you instead plumb through context so it knows > what >> the query is for? That said, I'm not sure I understand why the swap > only >> needs to happen here. Can you elaborate on that? > > I'm not sure what you mean by "plumb through context so it knows what > the query is for". > > And I don't disagree that it's awful, and I don't understand why it only > needs to happen here either. I just know, experimentally, that this was > someplace I found where the swap fixed the problem, but doing it > anywhere else in addition broke things again. > > I didn't really expect this version of the CL to land as is. It was > just the only working solution that I found after 2 days of searching. > I uploaded it to show about where the problem was, and was hoping > someone could suggest a better approach based on that. > > When GetBoundsForClientView() is called, there are 25+ functions in the > stack between it and the previous shelf/bubble function; everything > inbetween is generic views/layout code. Somewhere after > GetBoundsForClientView() returns, something down there reverses the > insets under RTL, but I don't know the coordinate & layout code well > enough to have a sense of where that might be. > > https://codereview.chromium.org/1866403005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm reverting this back to a version closer to Patch #2, with the fix in BubbleFrameView::GetBoundsForClientView(), and a small change that removes explicit border inset access from the CL: https://codereview.chromium.org/1866403005/diff2/20001:60001/ui/views/bubble/... After some more futzing, I don't think the BubbleBorder was the right place to fix this. BubbleBorder::GetInsets() was called 3 times from within BubbleFrameView::Paint(); when the insets were "unmirrored", the non-client bubble (background, border, and arrow) all got incorrectly shifted. The paint code seems to like the BubbleBorder as it is, so I don't think the problem is with the border. Experimentally, if we turn off RTL mirroring inside the ClientView that contains the OverflowBubbleView, everything works fine. However, this doesn't seem possible w/o modifying low level views code. So a compensating adjustment in BubbleFrameView::GetBoundsForClientView() doesn't seem like so bad a solution. I'm still not sure this is the best possible solution, but I don't currently have any better ideas. So what do you think?
https://codereview.chromium.org/1866403005/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:128: client_bounds.set_x(GetMirroredXForRect(client_bounds)); Why do you swap here and not after the client_bounds have been inset by the insets below (line 130)? I'm also inclined to think this should be done in NonClientView as it is the place the bounds are applied.
https://codereview.chromium.org/1866403005/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1866403005/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:128: client_bounds.set_x(GetMirroredXForRect(client_bounds)); On 2016/04/27 23:41:33, sky wrote: > Why do you swap here and not after the client_bounds have been inset > by the insets below (line 130)? Good catch. I hadn't thought of it, because the insets are hard-coded as symmetric, but experimentally if they're not, moving this down produces the correct result. > I'm also inclined to think this should be done in NonClientView as it is > the place the bounds are applied. I'm not sure how to do this. NonClientView is never subclassed, and can't easily be subclassed based on this code: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi... Would I put a "bool no_rtl_mirror_client_" member in NonClientView, then (a) apply it in Layout() after GetBoundsForClientView() returns, or (b) try to find where the mirroring happens, and prevent it? I'm reluctant to make any change to NonClientView for such a small bug. Also, the ClientView is exactly the class whose bounds need adjusting, and BubbleFrameView::GetBoundsForClientView(), which is already properly subclassed, seems like a reasonable place to do this, even if the method is a little hacky.
Patch 7 adds a basic unit test for arrow direction under RTL, which was the core problem. It *doesn't* test that the overflow shelf is on the screen, or the secondary issue of properly centering the client area within the bubble. Let me know if you feel either of these are worthwhile.
lgtm with a nit, but really deferring to Scott's review, and noting that it would still be nice if we actually knew what RTL code incorrectly mirrors the ClientView in the bubble... https://codereview.chromium.org/1866403005/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1866403005/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:119: // Adjust insets to account for the mirrored arrow in RTL layout. nit: // Used to adjust insets when the arrow is mirrored in RTL layout.
On Thu, Apr 28, 2016 at 8:14 AM, <glevin@chromium.org> wrote: > > https://codereview.chromium.org/1866403005/diff/60001/ui/views/bubble/bubble_... > File ui/views/bubble/bubble_frame_view.cc (right): > > https://codereview.chromium.org/1866403005/diff/60001/ui/views/bubble/bubble_... > ui/views/bubble/bubble_frame_view.cc:128: > client_bounds.set_x(GetMirroredXForRect(client_bounds)); > On 2016/04/27 23:41:33, sky wrote: >> Why do you swap here and not after the client_bounds have been inset >> by the insets below (line 130)? > > Good catch. I hadn't thought of it, because the insets are hard-coded > as symmetric, but experimentally if they're not, moving this down > produces the correct result. > >> I'm also inclined to think this should be done in NonClientView as it > is >> the place the bounds are applied. > > I'm not sure how to do this. NonClientView is never subclassed, and > can't easily be subclassed based on this code: > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi... > > Would I put a "bool no_rtl_mirror_client_" member in NonClientView, then > (a) apply it in Layout() after GetBoundsForClientView() returns, or Yes. This is what I was thinking. > (b) > try to find where the mirroring happens, and prevent it? I'm reluctant > to make any change to NonClientView for such a small bug. Just to be clear, my understanding is that you don't want the insets mirrored for that this case. Right? Basically you want the insets as they are in ltr? > Also, the ClientView is exactly the class whose bounds need adjusting, > and BubbleFrameView::GetBoundsForClientView(), which is already properly > subclassed, seems like a reasonable place to do this, even if the method > is a little hacky. > > https://codereview.chromium.org/1866403005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
>> Would I put a "bool no_rtl_mirror_client_" member in NonClientView, then >> (a) apply it in Layout() after GetBoundsForClientView() returns, or > > Yes. This is what I was thinking. Ok, how does this look? >> (b) >> try to find where the mirroring happens, and prevent it? I'm reluctant >> to make any change to NonClientView for such a small bug. > > Just to be clear, my understanding is that you don't want the insets > mirrored for that this case. Right? Basically you want the insets as > they are in ltr? That's correct, although it's not the insets themselves that are being mirrored. The client view (shelf piece) is being placed within the non-client frame based on correct insets. Some generic (non-bubble) code is subsequently mirroring the client position in RTL. I want the client to be placed as it is in LTR. https://codereview.chromium.org/1866403005/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1866403005/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:119: // Adjust insets to account for the mirrored arrow in RTL layout. On 2016/04/28 20:16:34, msw wrote: > nit: // Used to adjust insets when the arrow is mirrored in RTL layout. Moved into non_client_view.h, and comment changed to match.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1866403005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1866403005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/1866403005/diff/140001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/1866403005/diff/140001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:1012: EXPECT_EQ(bubble_view_api.GetBubbleFrameView()->bubble_border()->arrow(), nit: order for assertions is expected, actual. Isn't that the reverse of what you have here? https://codereview.chromium.org/1866403005/diff/140001/ui/views/window/non_cl... File ui/views/window/non_client_view.cc (right): https://codereview.chromium.org/1866403005/diff/140001/ui/views/window/non_cl... ui/views/window/non_client_view.cc:172: // desired, pre-mirror it here so the subsequent mirror results in a no-op. nit: you're not doing pre-mirror here, rather another mirror to get the original value.
https://codereview.chromium.org/1866403005/diff/140001/ash/shelf/shelf_view_u... File ash/shelf/shelf_view_unittest.cc (right): https://codereview.chromium.org/1866403005/diff/140001/ash/shelf/shelf_view_u... ash/shelf/shelf_view_unittest.cc:1012: EXPECT_EQ(bubble_view_api.GetBubbleFrameView()->bubble_border()->arrow(), On 2016/04/29 17:17:27, sky wrote: > nit: order for assertions is expected, actual. Isn't that the reverse of what > you have here? Done. https://codereview.chromium.org/1866403005/diff/140001/ui/views/window/non_cl... File ui/views/window/non_client_view.cc (right): https://codereview.chromium.org/1866403005/diff/140001/ui/views/window/non_cl... ui/views/window/non_client_view.cc:172: // desired, pre-mirror it here so the subsequent mirror results in a no-op. On 2016/04/29 17:17:27, sky wrote: > nit: you're not doing pre-mirror here, rather another mirror to get the original > value. Done.
The CQ bit was checked by glevin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1866403005/#ps160001 (title: "Small review fixes (and merge)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1866403005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1866403005/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Fix rtl shelf overflow bubble BUG=551712 TEST=Set Chrome OS with rtl language (e.g. Hebrew), position shelf on left, add icons to shelf until it overflows. Click on overflow button, observer that overflow shelf bubble is shown. ========== to ========== Fix rtl shelf overflow bubble BUG=551712 TEST=Set Chrome OS with rtl language (e.g. Hebrew), position shelf on left, add icons to shelf until it overflows. Click on overflow button, observer that overflow shelf bubble is shown. Committed: https://crrev.com/8e44a16c274b3cb9d1e6aa9d8cb96c7aa9b6d510 Cr-Commit-Position: refs/heads/master@{#390884} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8e44a16c274b3cb9d1e6aa9d8cb96c7aa9b6d510 Cr-Commit-Position: refs/heads/master@{#390884}
Message was sent while issue was closed.
Description was changed from ========== Fix rtl shelf overflow bubble BUG=551712 TEST=Set Chrome OS with rtl language (e.g. Hebrew), position shelf on left, add icons to shelf until it overflows. Click on overflow button, observer that overflow shelf bubble is shown. Committed: https://crrev.com/8e44a16c274b3cb9d1e6aa9d8cb96c7aa9b6d510 Cr-Commit-Position: refs/heads/master@{#390884} ========== to ========== Fix rtl shelf overflow bubble BUG=551712 TEST=Set Chrome OS with rtl language (e.g. Hebrew), position shelf on left, add icons to shelf until it overflows. Click on overflow button, observe that overflow shelf bubble is shown. Committed: https://crrev.com/8e44a16c274b3cb9d1e6aa9d8cb96c7aa9b6d510 Cr-Commit-Position: refs/heads/master@{#390884} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
