Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1802073002:
Makes vertical alignment of location bar bubbles same (Closed)
|
Created:
4 years, 9 months ago by varkha Modified:
4 years, 9 months ago CC:
chromium-reviews, tfarina, gcasto+watchlist_chromium.org, noyau+watch_chromium.org, vabr+watchlistpasswordmanager_chromium.org, mkwst+watchlist-passwords_chromium.org, tdanderson Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMakes vertical alignment of location bar bubbles same
All LocationBarBubbleDelegateView descendants should use the same offset.
BookmarkBubbleView offset is updated to match the rest of the bubbles.
This CL changes the visible offset for the following bubbles:
BookmarkBubbleView (bookmark star - down by 3 pixels)
SaveCardBubbleViews (save credit card - up by 5 pixels)
TranslateBubbleView (Google translate suggest - up by 5 pixels)
I plan to follow this up with a change to add a GetLayoutConstant and have
all location bar bubbles use 7 with MD instead of 5 to better connect the
bubble with the bottom edge of the activated ink drop.
BUG=566115
TEST=Open zoom bubble by pressing [Ctrl-+] or [Ctrl--]. Note the vertical position of the bubble (ignoring the arrow).
Open the same bubble by clicking the magnifying glass icon in location bar. Verify that the bubble itself appears at the same exact position - when shown with or without the arrow.
Committed: https://crrev.com/e2d602b62dd9bf59125dbcee534f95a2f63cbe80
Cr-Commit-Position: refs/heads/master@{#382010}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Makes vertical alignment of location bar bubbles same (comments) #
Total comments: 8
Patch Set 3 : Makes vertical alignment of location bar bubbles same (better clickability) #Patch Set 4 : Makes vertical alignment of location bar bubbles same (updated tests) #
Total comments: 6
Patch Set 5 : Makes vertical alignment of location bar bubbles same (nits and test simplification) #
Total comments: 4
Patch Set 6 : Makes vertical alignment of location bar bubbles same (code comments) #
Total comments: 6
Patch Set 7 : Makes vertical alignment of location bar bubbles same (nits) #
Messages
Total messages: 55 (21 generated)
Description was changed from ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView decendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. BUG=566115 ========== to ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) BUG=566115 ==========
The CQ bit was checked by varkha@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/1802073002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802073002/1
varkha@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@, can you please take a look? I think we should follow this up with shifting all bubbles up by 2 pixels at least with MD to connect them better to the activated icons but I wanted to equalize offsets everywhere first. Thanks!
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1802073002/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/1802073002/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:279: set_anchor_view_insets(gfx::Insets(-1, 0, -1, 0)); nit: gfx::Insets(-1, 0) can you colocate this with the part where we actually set NONE arrow? https://codereview.chromium.org/1802073002/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/1802073002/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:30: set_anchor_view_insets(gfx::Insets(5, 0, 5, 0)); gfx::Insets(5, 0) should this be different in MD/pre-MD?
https://codereview.chromium.org/1802073002/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/1802073002/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:279: set_anchor_view_insets(gfx::Insets(-1, 0, -1, 0)); On 2016/03/14 21:57:53, Evan Stade wrote: > nit: gfx::Insets(-1, 0) > > can you colocate this with the part where we actually set NONE arrow? Done. https://codereview.chromium.org/1802073002/diff/1/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/1802073002/diff/1/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:30: set_anchor_view_insets(gfx::Insets(5, 0, 5, 0)); On 2016/03/14 21:57:53, Evan Stade wrote: > gfx::Insets(5, 0) > > should this be different in MD/pre-MD? I think so but I wanted to make them all same across different bubbles first - both pre-MD and in MD.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) BUG=566115 ========== to ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) I plan to follow this up with a change to add a GetLayoutConstant and have all location bar bubbles use 7 with MD instead of 5 to better connect the bubble with the bottom edge of the activated ink drop. BUG=566115 ==========
https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:90: bookmark_bubble_->set_anchor_view_insets(gfx::Insets(-1, 0)); Do you still need to do this if you use PAINT_TRANSPARENT for the arrow type instead? I think that should position the bubble like the others but just not draw an arrow. https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:29: // Compensate for built-in vertical padding in the anchor view's image. Is this comment still correct? The various anchor images have different amounts of padding, don't they? Is this the max padding, min padding, or some average point for these images?
https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:90: bookmark_bubble_->set_anchor_view_insets(gfx::Insets(-1, 0)); On 2016/03/14 22:35:51, Peter Kasting wrote: > Do you still need to do this if you use PAINT_TRANSPARENT for the arrow type > instead? I think that should position the bubble like the others but just not > draw an arrow. That would indeed be great and I wanted to do that. However with the arrow code as it is the transparent arrow is not transparent to clicks so it will be a noticeable regression in how easy it is to click the star when a bubble is open (assuming the same visible vertical offset). I will look into whether it is possible to update hit testing for the bubble to account for a transparent arrow but prefer to do that separately. https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:29: // Compensate for built-in vertical padding in the anchor view's image. On 2016/03/14 22:35:51, Peter Kasting wrote: > Is this comment still correct? The various anchor images have different amounts > of padding, don't they? Is this the max padding, min padding, or some average > point for these images? This same comment appears in all dozen or so call sites for this compensation. I am planning on touching them to add MD layout constant and can refresh the comment then (and maybe reduce the number of call sites). Is that OK?
https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:90: bookmark_bubble_->set_anchor_view_insets(gfx::Insets(-1, 0)); On 2016/03/15 00:11:42, varkha wrote: > On 2016/03/14 22:35:51, Peter Kasting wrote: > > Do you still need to do this if you use PAINT_TRANSPARENT for the arrow type > > instead? I think that should position the bubble like the others but just not > > draw an arrow. > > That would indeed be great and I wanted to do that. However with the arrow code > as it is the transparent arrow is not transparent to clicks so it will be a > noticeable regression in how easy it is to click the star when a bubble is open > (assuming the same visible vertical offset). > I will look into whether it is possible to update hit testing for the bubble to > account for a transparent arrow but prefer to do that separately. I think it should be possible and it's probably a decent idea to look into that before landing this patch since it has the potential to affect this patch.
https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:90: bookmark_bubble_->set_anchor_view_insets(gfx::Insets(-1, 0)); On 2016/03/15 00:13:27, Evan Stade wrote: > On 2016/03/15 00:11:42, varkha wrote: > > On 2016/03/14 22:35:51, Peter Kasting wrote: > > > Do you still need to do this if you use PAINT_TRANSPARENT for the arrow type > > > instead? I think that should position the bubble like the others but just > not > > > draw an arrow. > > > > That would indeed be great and I wanted to do that. However with the arrow > code > > as it is the transparent arrow is not transparent to clicks so it will be a > > noticeable regression in how easy it is to click the star when a bubble is > open > > (assuming the same visible vertical offset). > > I will look into whether it is possible to update hit testing for the bubble > to > > account for a transparent arrow but prefer to do that separately. > > I think it should be possible and it's probably a decent idea to look into that > before landing this patch since it has the potential to affect this patch. Yeah, I'd like to see if this is easy enough to do as well. Doing it separately is fine but I would go ahead and look at that first. https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc (right): https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc:29: // Compensate for built-in vertical padding in the anchor view's image. On 2016/03/15 00:11:42, varkha wrote: > On 2016/03/14 22:35:51, Peter Kasting wrote: > > Is this comment still correct? The various anchor images have different > amounts > > of padding, don't they? Is this the max padding, min padding, or some average > > point for these images? > > This same comment appears in all dozen or so call sites for this compensation. I > am planning on touching them to add MD layout constant and can refresh the > comment then (and maybe reduce the number of call sites). Is that OK? OK.
pkasting@, estade@, have a question about event dispatch in bubbles. https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/1802073002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:90: bookmark_bubble_->set_anchor_view_insets(gfx::Insets(-1, 0)); I have tried to pursue this. I can make the BubbleFrameView transparent to events by overriding DoesIntersectRect (similar to [1]) but that doesn't solve the problem because the bubble has its own native widget and so the mouse events never reach the browser frame's root view. Am I missing something easy? [1] - https://codereview.chromium.org/1767363002/diff/80001/chrome/browser/ui/views...
I am thinking maybe changing BubbleBorder::GetBounds [1] to account for the transparent arrow. Instead of reserving space for the transparent arrow we could shift the bubble bounds while making it same size as with ARROW_NONE. Do you see problems with this approach? [1] https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/bubble/bu...
I think that sounds more promising than trying to have a larger bubble with a transparent area where the arrow is. You can work around the event dispatch problems you're seeing, but it's annoying. Probably easier and safer to go this other route.
PTAL. Manipulating the bubble border bounds and insets allows transparent and none to be visually in the same place as before this change but because I now shrink the bubble size with TRANSPARENT arrow paint type (in addition to NONE) the clicks just above the visible bubble go to underlying widget / view (in this case to the anchor icon in location bar).
The CQ bit was checked by varkha@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/1802073002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802073002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@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/1802073002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802073002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a suggestion to simplify the new test code. https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:171: // this shift the bounds by |arrow_interior_thickness|; Nit: ; -> . But maybe better, just remove the last sentence. It just restates the code. https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:264: arrow_paint_type_ == PAINT_TRANSPARENT || !has_arrow(arrow_)) { Nit: Simpler, avoids {}: if (arrow_paint_type_ != PAINT_NORMAL || !has_arrow(arrow_)) (2 places) https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border_unittest.cc (right): https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_border_unittest.cc:350: const int kArrowShift = kImages->arrow_interior_thickness + We have a lot of code below this point that explicitly computes all the shift values and specifies them for every case. I think we could ditch all of this, and return to something like the original code, and simply calculate these on the fly within the loop. Basically, we know what the delta is between a shift for a visible arrow and an invisible arrow; in the loop we would look at the arrow position and then add this delta to the appropriate value from the with-arrow amount. Etc.
The CQ bit was checked by varkha@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/1802073002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802073002/120001
varkha@chromium.org changed reviewers: + msw@chromium.org
msw@, can you please take a look for OWNERS in ui/views/bubble/? https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:171: // this shift the bounds by |arrow_interior_thickness|; On 2016/03/17 06:16:15, Peter Kasting wrote: > Nit: ; -> . > > But maybe better, just remove the last sentence. It just restates the code. Done. https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:264: arrow_paint_type_ == PAINT_TRANSPARENT || !has_arrow(arrow_)) { On 2016/03/17 06:16:15, Peter Kasting wrote: > Nit: Simpler, avoids {}: > > if (arrow_paint_type_ != PAINT_NORMAL || !has_arrow(arrow_)) > > (2 places) Done. https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border_unittest.cc (right): https://codereview.chromium.org/1802073002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_border_unittest.cc:350: const int kArrowShift = kImages->arrow_interior_thickness + On 2016/03/17 06:16:15, Peter Kasting wrote: > We have a lot of code below this point that explicitly computes all the shift > values and specifies them for every case. > > I think we could ditch all of this, and return to something like the original > code, and simply calculate these on the fly within the loop. Basically, we know > what the delta is between a shift for a visible arrow and an invisible arrow; in > the loop we would look at the arrow position and then add this delta to the > appropriate value from the with-arrow amount. Etc. Yes, this is indeed shorter and made it easier to reason. Thanks!
https://codereview.chromium.org/1802073002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1802073002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:168: images_->arrow_interior_thickness + kStroke - images_->arrow_thickness; took me a good while to figure out what this is. It's actually the size of the shadow I guess? Any way we can use variable names to make that more clear? Is the += just below equivalent to arrow_shift = images_->arrow_thickness; ?
I have a similar question/confusion as Evan... https://codereview.chromium.org/1802073002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1802073002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:172: arrow_shift += images_->arrow_interior_thickness; Hmm, I don't understand why adding the interior thickness again works as intended, shouldn't this instead only subtract images_->arrow_thickness above if arrow_paint_type_ != PAINT_TRANSPARENT? Perhaps manually test this change (in views_examples?) with each of the BubbleBorder::Shadow types. I wonder if the bubble placements with bottom/right arrows aren't correct.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
I have added code comments, hope they make it a bit less confusing but I think (and I've checked the visuals with different arrow types) that the math was right. https://codereview.chromium.org/1802073002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1802073002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:168: images_->arrow_interior_thickness + kStroke - images_->arrow_thickness; On 2016/03/18 01:49:21, Evan Stade wrote: > took me a good while to figure out what this is. It's actually the size of the > shadow I guess? Any way we can use variable names to make that more clear? > The image we use for the arrow (I am taking the arrow used with the content bubbles that is used with TOP_RIGHT arrow as an example here) contains 3 elements: - Top blank space (shadow) - upward pointing arrow tip (|arrow_interior_thickness| tall, think of it as 6 pixels) - horizontal line ("stroke" - |kStroke| thick line, 1 pixel) The whole image is |arrow_thickness| tall (think of it as 10 pixels). For the TOP_RIGHT arrow type we want to align the tip of the arrow with the anchor bottom which means we need to shift the BubbleBorder bounds up by the shadow thickness. Shadow thickness is |arrow_thickness - kStroke - arrow_internal_thickness|. The shift is negative that which is the calculation in line 168. The previous name |arrow_size| was totally confusing, I have changed it to arrow_shift, I have added a comment but I prefer "shift" to referring to image elements because the shift can be positive or negative depending on arrow paint type. I hope the comment makes it a bit less confusing. > Is the += just below equivalent to > arrow_shift = images_->arrow_thickness; > ? I don't think so. With the numbers as in my example above (which happen to be the numbers I see in the debug traces) I get arrow_shift = (6 + 1 - 10) + 6 = -3 + 6 = 3. With your suggestion I would get arrow_shift = 10 which would place it in a wrong place. https://codereview.chromium.org/1802073002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:172: arrow_shift += images_->arrow_interior_thickness; On 2016/03/18 02:02:52, msw wrote: > Hmm, I don't understand why adding the interior thickness again works as > intended, shouldn't this instead only subtract images_->arrow_thickness above if > arrow_paint_type_ != PAINT_TRANSPARENT? Perhaps manually test this change (in > views_examples?) with each of the BubbleBorder::Shadow types. I wonder if the > bubble placements with bottom/right arrows aren't correct. No. I am not logically adding it again (although the math works this way), but rather shifting down the bounds by the visible height of the visible arrow tip which is |arrow_internal_thickness|. Please see the comment above for what I think those mean. Consider that the height of the bubble with TRANSPARENT arrow (with this CL) is same as the height with NONE arrow - instead of reserving space for the arrow and not drawing it I now simply don't reserve the space. This meant that to maintain the same visible bounds for the TRANSPARENT case I had to shift the bounds down by |arrow_internal_thickness|. Effectively the change in this file is pixel-neutral but allows clicks just above the bubble to reach the elements under the click. For simplicity all my comments refer to bubble placement below the anchor but of course I have checked the bottom/right placements as well as top and left and they seem right to me (and I've double checked the math here). I have also added the unit test and it checks that the bubble is where I expect it to be with all permutations of arrow type and alignment. That test should be easy to reason about.
lgtm https://codereview.chromium.org/1802073002/diff/140001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1802073002/diff/140001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:169: // and the shadow thickness can be calculated as total arrow image height nit: I'd leave off everything after the second line https://codereview.chromium.org/1802073002/diff/140001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:175: // to be positioned at the same bounds as when the arrow is shown. Shift nit: I'd leave off everything past the first sentence. https://codereview.chromium.org/1802073002/diff/140001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:178: arrow_shift += images_->arrow_interior_thickness; just to double check --- you don't need a + kStroke here?
Thanks! https://codereview.chromium.org/1802073002/diff/140001/ui/views/bubble/bubble... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/1802073002/diff/140001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:169: // and the shadow thickness can be calculated as total arrow image height On 2016/03/18 04:53:32, Evan Stade wrote: > nit: I'd leave off everything after the second line Done. https://codereview.chromium.org/1802073002/diff/140001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:175: // to be positioned at the same bounds as when the arrow is shown. Shift On 2016/03/18 04:53:32, Evan Stade wrote: > nit: I'd leave off everything past the first sentence. Done. https://codereview.chromium.org/1802073002/diff/140001/ui/views/bubble/bubble... ui/views/bubble/bubble_border.cc:178: arrow_shift += images_->arrow_interior_thickness; On 2016/03/18 04:53:32, Evan Stade wrote: > just to double check --- you don't need a + kStroke here? Not if I want the top or bottom horizontal lines of a bubble with TRANSPARENT arrow to be exactly where the horizontal lines are for a bubble with NORMAL arrow. You can compare the current magnifying glass bubble placement with keyboard accelerator (arrow) vs. the same bubble when it is opened with a mouse click. I think this is because the window_bubble_shadow_*.png images are shorter than window_bubble_shadow_spike_*.png by |kStroke|. One of the tests checks that the bubble size shrinks by the same amount as the |arrow_interior_thickness| when moving from NORMAL to TRANSPARENT or NONE.
Description was changed from ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) I plan to follow this up with a change to add a GetLayoutConstant and have all location bar bubbles use 7 with MD instead of 5 to better connect the bubble with the bottom edge of the activated ink drop. BUG=566115 ========== to ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) I plan to follow this up with a change to add a GetLayoutConstant and have all location bar bubbles use 7 with MD instead of 5 to better connect the bubble with the bottom edge of the activated ink drop. BUG=566115 TEST=Open zoom bubble by pressing [Ctrl-+] or [Ctrl--]. Note the vertical position of the bubble (ignoring the arrow). Open the same bubble by clicking the magnifying glass icon in location bar. Verify that the bubble itself appears at the same exact position - when shown with or without the arrow. ==========
The CQ bit was checked by varkha@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/1802073002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802073002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Okay, lgtm. But I idly wonder if anchor inset adjustments + PAINT_NONE would achieve the same thing in a more straightforward manner (and we could eliminate PAINT_TRANSPARENT).
On 2016/03/18 17:12:24, msw wrote: > Okay, lgtm. But I idly wonder if anchor inset adjustments + PAINT_NONE would > achieve the same thing in a more straightforward manner (and we could eliminate > PAINT_TRANSPARENT). We can obviously position the TRANSPARENT and NONE bubbles at exactly same point by adjusting the external offset, this would however need tweaking the insets externally. What TRANSPARENT achieves now is that the bubble can be offset externally by the same amount and will be visually well aligned in both TRANSPARENT and NORMAL modes which we use to distinguish input event-driven (no visible arrow but shadow on the anchor) vs. non-user-driven (visible arrow in a bubble) scenarios. So this simplifies the code at call sites.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1802073002/#ps160001 (title: "Makes vertical alignment of location bar bubbles same (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1802073002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1802073002/160001
Message was sent while issue was closed.
Description was changed from ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) I plan to follow this up with a change to add a GetLayoutConstant and have all location bar bubbles use 7 with MD instead of 5 to better connect the bubble with the bottom edge of the activated ink drop. BUG=566115 TEST=Open zoom bubble by pressing [Ctrl-+] or [Ctrl--]. Note the vertical position of the bubble (ignoring the arrow). Open the same bubble by clicking the magnifying glass icon in location bar. Verify that the bubble itself appears at the same exact position - when shown with or without the arrow. ========== to ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) I plan to follow this up with a change to add a GetLayoutConstant and have all location bar bubbles use 7 with MD instead of 5 to better connect the bubble with the bottom edge of the activated ink drop. BUG=566115 TEST=Open zoom bubble by pressing [Ctrl-+] or [Ctrl--]. Note the vertical position of the bubble (ignoring the arrow). Open the same bubble by clicking the magnifying glass icon in location bar. Verify that the bubble itself appears at the same exact position - when shown with or without the arrow. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) I plan to follow this up with a change to add a GetLayoutConstant and have all location bar bubbles use 7 with MD instead of 5 to better connect the bubble with the bottom edge of the activated ink drop. BUG=566115 TEST=Open zoom bubble by pressing [Ctrl-+] or [Ctrl--]. Note the vertical position of the bubble (ignoring the arrow). Open the same bubble by clicking the magnifying glass icon in location bar. Verify that the bubble itself appears at the same exact position - when shown with or without the arrow. ========== to ========== Makes vertical alignment of location bar bubbles same All LocationBarBubbleDelegateView descendants should use the same offset. BookmarkBubbleView offset is updated to match the rest of the bubbles. This CL changes the visible offset for the following bubbles: BookmarkBubbleView (bookmark star - down by 3 pixels) SaveCardBubbleViews (save credit card - up by 5 pixels) TranslateBubbleView (Google translate suggest - up by 5 pixels) I plan to follow this up with a change to add a GetLayoutConstant and have all location bar bubbles use 7 with MD instead of 5 to better connect the bubble with the bottom edge of the activated ink drop. BUG=566115 TEST=Open zoom bubble by pressing [Ctrl-+] or [Ctrl--]. Note the vertical position of the bubble (ignoring the arrow). Open the same bubble by clicking the magnifying glass icon in location bar. Verify that the bubble itself appears at the same exact position - when shown with or without the arrow. Committed: https://crrev.com/e2d602b62dd9bf59125dbcee534f95a2f63cbe80 Cr-Commit-Position: refs/heads/master@{#382010} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e2d602b62dd9bf59125dbcee534f95a2f63cbe80 Cr-Commit-Position: refs/heads/master@{#382010}
Message was sent while issue was closed.
On 2016/03/18 17:52:40, commit-bot: I haz the power wrote: > Patchset 7 (id:??) landed as > https://crrev.com/e2d602b62dd9bf59125dbcee534f95a2f63cbe80 > Cr-Commit-Position: refs/heads/master@{#382010} afterthought: did you check how this affects SystemTrayBubble, the only other place PAINT_TRANSPARENT seems to be used?
Message was sent while issue was closed.
On 2016/03/18 19:05:53, Evan Stade wrote: > On 2016/03/18 17:52:40, commit-bot: I haz the power wrote: > > Patchset 7 (id:??) landed as > > https://crrev.com/e2d602b62dd9bf59125dbcee534f95a2f63cbe80 > > Cr-Commit-Position: refs/heads/master@{#382010} > > afterthought: did you check how this affects SystemTrayBubble, the only other > place PAINT_TRANSPARENT seems to be used? Yes, I've tried it. I can show volume bubble (pressing F9 / F10 opens the volume bubble without an arrow) and it shows up in the correct spot (same as before this change). I've also tried it with different shelf alignment values and made sure that the shift is indeed capable of changing what I see on scree so this was a legitimate concern. Thanks for reminding to check that! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
