Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1300843003:
Change width of rows in material design omnibox dropdown (Closed)
|
Created:
5 years, 4 months ago by tdanderson Modified:
5 years, 4 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange width of rows in material design omnibox dropdown
For material design in browser top chrome, the
width of each row in the omnibox dropdown
should be the same as the width of the dropdown
itself, though contents (icons and text) should
remain bounded by the width of the location bar.
The net effect of this change to the user is that
the selected/hovered row colour is visible along
the entire width of the dropdown, and this entire
region is clickable/touchable.
BUG=519514
TEST=manual
Committed: https://crrev.com/fd8e2a64e6c07272209dbc1f2a57e9ef894f8b27
Cr-Commit-Position: refs/heads/master@{#345221}
Patch Set 1 #
Total comments: 12
Patch Set 2 : nits, change in PaintMatch() #
Total comments: 5
Patch Set 3 : change order of terms #Patch Set 4 : no changes to PaintMatch() #
Total comments: 4
Patch Set 5 : change left/right to start/end #
Messages
Total messages: 15 (3 generated)
tdanderson@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter, can you PTAL? Note this does not address RTL in the material design implementation. I've filed crbug.com/522134 as a follow-up to this CL.
https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:129: min_vertical_padding); Nit: This code is the same length, but might be slightly clearer as it separates out the vertical and horizontal insetting. (I also modified the comment slightly.) contents_rect.Inset( 0, views::NonClientFrameView::kClientEdgeThickness + min_vertical_padding, 0, min_vertical_padding); // In the non-material dropdown, the colored/clickable regions within the // dropdown are only as wide as the location bar. In the material version, // these are full width, and OmniboxResultView instead insets the icons/text // inside to be aligned with the location bar. if (!ui::MaterialDesignController::IsModeMaterial()) contents_rect.Inset(left_margin_, 0, right_margin_, 0); https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:440: // No text should be drawn within the right margin. This isn't the right way to do this. You need to change the code in PaintMatch() so that it computes the contents and description widths based on the correct available width, as we're making eliding and layout decisions that rely on knowing precisely how much space is available. What this code does instead is lay out as if we have the full width, but then truncate the last portion. https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:633: horizontal_padding + LeftOffset() + Nit: If we create two more temps: const int start_x = LeftOffset() + horizontal_padding; const int end_x = width() - RightOffset() - horizontal_padding; Then we can simplify the code here some: icon_bounds_.SetRect( start_x + ((icon.width() == default_icon_size_) ? 0 : trailing_padding), (GetContentLineHeight() - icon.height()) / 2, icon.width(), icon.height()); const int text_x = start_x + default_icon_size_ + horizontal_padding_; int text_width = end_x - text_x; if (match_.associated_keyword) { const int max_kw_x = end_x - keyword_icon_->width(); const int kw_x = animation_->CurrentValueBetween(max_kw_x, start_x); ... https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:810: if (ui::MaterialDesignController::IsModeMaterial()) Nit: Shorter: return ui::MaterialDesignController::IsModeMaterial() ? model_->left_margin() : 0; (2 places) https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.h:179: // location bar. Nit: How about: Returns the necessary margin, if any, at the start and end of the view. This allows us to keep the icon and text in the view aligned with the location bar constants. (This avoids going into implementation details in an API comment.) https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.h:181: int RightOffset() const; I'd use "Margin" instead of "Offset" here, as a margin is a width and an offset is a position. These values are widths, not positions. I'd also try to avoid "left" and "right" and prefer "Start" and "End" if possible, though perhaps that change only makes sense when you fix this for RTL?
Thanks for the comments. Please take a look at patch set 2. https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right): https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:129: min_vertical_padding); On 2015/08/18 19:55:37, Peter Kasting wrote: > Nit: This code is the same length, but might be slightly clearer as it separates > out the vertical and horizontal insetting. (I also modified the comment > slightly.) > > contents_rect.Inset( > 0, views::NonClientFrameView::kClientEdgeThickness + min_vertical_padding, > 0, min_vertical_padding); > // In the non-material dropdown, the colored/clickable regions within the > // dropdown are only as wide as the location bar. In the material version, > // these are full width, and OmniboxResultView instead insets the icons/text > // inside to be aligned with the location bar. > if (!ui::MaterialDesignController::IsModeMaterial()) > contents_rect.Inset(left_margin_, 0, right_margin_, 0); Agreed, that is much nicer. Changed. https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:440: // No text should be drawn within the right margin. On 2015/08/18 19:55:37, Peter Kasting wrote: > This isn't the right way to do this. You need to change the code in > PaintMatch() so that it computes the contents and description widths based on > the correct available width, as we're making eliding and layout decisions that > rely on knowing precisely how much space is available. What this code does > instead is lay out as if we have the full width, but then truncate the last > portion. OK, please take a look at what I've done in Patch Set 2, which looks fine with and without the MD flag on a local build. Though I'm a bit suspicious of the fact that I am fiddling with the call to mirroring_context_->remaining_width(x) in PaintMatch() but not the one in DrawRenderText(). https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:633: horizontal_padding + LeftOffset() + On 2015/08/18 19:55:37, Peter Kasting wrote: > Nit: If we create two more temps: > > const int start_x = LeftOffset() + horizontal_padding; > const int end_x = width() - RightOffset() - horizontal_padding; > > Then we can simplify the code here some: > > icon_bounds_.SetRect( > start_x + ((icon.width() == default_icon_size_) ? 0 : trailing_padding), > (GetContentLineHeight() - icon.height()) / 2, > icon.width(), icon.height()); > > const int text_x = start_x + default_icon_size_ + horizontal_padding_; > int text_width = end_x - text_x; > > if (match_.associated_keyword) { > const int max_kw_x = end_x - keyword_icon_->width(); > const int kw_x = animation_->CurrentValueBetween(max_kw_x, start_x); > ... Thanks, should've caught that. Done. https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:810: if (ui::MaterialDesignController::IsModeMaterial()) On 2015/08/18 19:55:37, Peter Kasting wrote: > Nit: Shorter: > > return ui::MaterialDesignController::IsModeMaterial() ? > model_->left_margin() : 0; > > (2 places) Done. https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.h:179: // location bar. On 2015/08/18 19:55:37, Peter Kasting wrote: > Nit: How about: > > Returns the necessary margin, if any, at the start and end of the view. This > allows us to keep the icon and text in the view aligned with the location bar > constants. > > (This avoids going into implementation details in an API comment.) Done. https://codereview.chromium.org/1300843003/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.h:181: int RightOffset() const; On 2015/08/18 19:55:37, Peter Kasting wrote: > I'd use "Margin" instead of "Offset" here, as a margin is a width and an offset > is a position. These values are widths, not positions. Done. > > I'd also try to avoid "left" and "right" and prefer "Start" and "End" if > possible, though perhaps that change only makes sense when you fix this for RTL? I was planning on addressing that if needed when looking at RTL. I picked "left" and "right" to be consistent with the naming of |left_margin_| and |right_margin_|.
https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:336: if (ui::MaterialDesignController::IsModeMaterial()) { On more reflection: why is this block needed at all? The mirroring context tells you how much space you have based on the values given to it in an Initialize() call. That call happens in OnPaint() and will use the [keyword_]text_bounds_ calculated in Layout(). Since you've changed the code there to reflect the margins you want, the mirroring context should be correctly clamping to the space you want. I think you ought to be able to entirely revert the changes to PaintMatch(), and still get the behavior you want. If not, then I'll need to have a better understanding of what's going wrong to know how to fix. https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:643: const int text_x = start_x + horizontal_padding + default_icon_size_; Nit: Put |default_icon_size_| before |horizontal_padding| so it's clear that's the padding after the icon, not an erroneous duplication of the padding before it.
https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:336: if (ui::MaterialDesignController::IsModeMaterial()) { On 2015/08/19 20:37:34, Peter Kasting wrote: > On more reflection: why is this block needed at all? > > The mirroring context tells you how much space you have based on the values > given to it in an Initialize() call. That call happens in OnPaint() and will > use the [keyword_]text_bounds_ calculated in Layout(). Since you've changed the > code there to reflect the margins you want, the mirroring context should be > correctly clamping to the space you want. I think you ought to be able to > entirely revert the changes to PaintMatch(), and still get the behavior you > want. > > If not, then I'll need to have a better understanding of what's going wrong to > know how to fix. Without this block, the "Search <site> for <query>" text shows up in the right margin when normally it would be hidden until the user actually presses tab. I'll attach a screenshot to crbug.com/519514. https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:643: const int text_x = start_x + horizontal_padding + default_icon_size_; On 2015/08/19 20:37:34, Peter Kasting wrote: > Nit: Put |default_icon_size_| before |horizontal_padding| so it's clear that's > the padding after the icon, not an erroneous duplication of the padding before > it. Done.
Sorry it took me a while to look at this. https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:336: if (ui::MaterialDesignController::IsModeMaterial()) { On 2015/08/19 22:32:57, tdanderson wrote: > On 2015/08/19 20:37:34, Peter Kasting wrote: > > On more reflection: why is this block needed at all? > > > > The mirroring context tells you how much space you have based on the values > > given to it in an Initialize() call. That call happens in OnPaint() and will > > use the [keyword_]text_bounds_ calculated in Layout(). Since you've changed > the > > code there to reflect the margins you want, the mirroring context should be > > correctly clamping to the space you want. I think you ought to be able to > > entirely revert the changes to PaintMatch(), and still get the behavior you > > want. > > > > If not, then I'll need to have a better understanding of what's going wrong to > > know how to fix. > > Without this block, the "Search <site> for <query>" text shows up in the right > margin when normally it would be hidden until the user actually presses tab. > I'll attach a screenshot to crbug.com/519514. This is because the change to Layout() is incomplete. You need to change line 652 to: keyword_text_bounds_.SetRect(kw_text_x, 0, std::max(end_x - kw_text_x, 0), height());
On 2015/08/21 22:13:06, Peter Kasting wrote: > Sorry it took me a while to look at this. > > https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): > > https://codereview.chromium.org/1300843003/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/omnibox/omnibox_result_view.cc:336: if > (ui::MaterialDesignController::IsModeMaterial()) { > On 2015/08/19 22:32:57, tdanderson wrote: > > On 2015/08/19 20:37:34, Peter Kasting wrote: > > > On more reflection: why is this block needed at all? > > > > > > The mirroring context tells you how much space you have based on the values > > > given to it in an Initialize() call. That call happens in OnPaint() and > will > > > use the [keyword_]text_bounds_ calculated in Layout(). Since you've changed > > the > > > code there to reflect the margins you want, the mirroring context should be > > > correctly clamping to the space you want. I think you ought to be able to > > > entirely revert the changes to PaintMatch(), and still get the behavior you > > > want. > > > > > > If not, then I'll need to have a better understanding of what's going wrong > to > > > know how to fix. > > > > Without this block, the "Search <site> for <query>" text shows up in the right > > margin when normally it would be hidden until the user actually presses tab. > > I'll attach a screenshot to crbug.com/519514. > > This is because the change to Layout() is incomplete. > > You need to change line 652 to: > > keyword_text_bounds_.SetRect(kw_text_x, 0, std::max(end_x - kw_text_x, 0), > height()); Thanks. In patch set 4 there are no longer any changes to PaintMatch().
LGTM https://codereview.chromium.org/1300843003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h (right): https://codereview.chromium.org/1300843003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h:73: } Nit: I'd probably just do all these getters in a block like: int max_match_contents_width() const { return max_match_contents_width_; } int left_margin() const { return left_margin_; } int right_margin() const { return right_margin_; } (I think that's what clang-format would do too?) https://codereview.chromium.org/1300843003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1300843003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:179: int RightMargin() const; I suspect the code you now have works for RTL as well as LTR. If so, consider going ahead with naming these Start/EndMargin() or the like. If you want to stay consistent with OmniboxPopupContentsView, its accessors and members could both be renamed similarly.
https://codereview.chromium.org/1300843003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h (right): https://codereview.chromium.org/1300843003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h:73: } On 2015/08/24 18:20:00, Peter Kasting wrote: > Nit: I'd probably just do all these getters in a block like: > > int max_match_contents_width() const { return max_match_contents_width_; } > int left_margin() const { return left_margin_; } > int right_margin() const { return right_margin_; } > > (I think that's what clang-format would do too?) Done. https://codereview.chromium.org/1300843003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/omnibox/omnibox_result_view.h (right): https://codereview.chromium.org/1300843003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/omnibox/omnibox_result_view.h:179: int RightMargin() const; On 2015/08/24 18:20:00, Peter Kasting wrote: > I suspect the code you now have works for RTL as well as LTR. If so, consider > going ahead with naming these Start/EndMargin() or the like. If you want to > stay consistent with OmniboxPopupContentsView, its accessors and members could > both be renamed similarly. You're right, it seems to look fine for RTL. Re-named everything accordingly.
The CQ bit was checked by tdanderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1300843003/#ps80001 (title: "change left/right to start/end")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1300843003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1300843003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fd8e2a64e6c07272209dbc1f2a57e9ef894f8b27 Cr-Commit-Position: refs/heads/master@{#345221} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
