Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
516943003:
Add textfield internal padding from FocusableBorder. (Closed)
|
Created:
6 years, 3 months ago by msw Modified:
6 years, 3 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, tfarina, penghuang+watch_chromium.org, nona+watch_chromium.org, James Su, benwells, calamity, jennyz Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd textfield internal padding from FocusableBorder.
Add 3px of internal padding for textfield/combobox content.
Remove corresponding unused padding from FocusableBorder.
This better accommodates excessively tall fallback font text.
Update Omnibox, Find Bar, App List, and CookieInfoView.
See before/after pics at http://crbug.com/404999#c51
BUG=404999
TEST=Bookmark bubble/dialog, find bar, etc. don't clip most tall fallback text.
R=sky@chromium.org,pkasting@chromium.org
Committed: https://crrev.com/a259c9b832bcbc4a641f1696071df39e028a5c2c
Cr-Commit-Position: refs/heads/master@{#294016}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Reduce FocusableBorder height; update Textfield and Combobox. #Patch Set 3 : Avoid SetBorder overrides with a default border member. #Patch Set 4 : Shrink FocusableBorder; add Textfield and Combobox padding. #
Total comments: 10
Patch Set 5 : Sync and rebase. #Patch Set 6 : Address comments; fix find bar offset. #
Total comments: 8
Patch Set 7 : Address comments; retain find bar size and layout. #Patch Set 8 : Refine FindBarView and CookieInfoView textfield layout. #Patch Set 9 : Update AppList's SearchBoxView and FolderHeaderView; git cl format. #
Total comments: 14
Patch Set 10 : Address comments. #
Total comments: 8
Patch Set 11 : Address Peter's request... #
Total comments: 7
Patch Set 12 : Restore Insets const ref. #Messages
Total messages: 36 (2 generated)
msw@chromium.org changed reviewers: + sky@chromium.org
Hey Scott, please take a look; thanks!
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... ui/views/controls/textfield/textfield.cc:281: // Use most of the FocusableBorder's internal padding for text rendering. Why not just make FocusableBorder thinner?
https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... ui/views/controls/textfield/textfield.cc:281: // Use most of the FocusableBorder's internal padding for text rendering. On 2014/08/28 21:15:37, Peter Kasting wrote: > Why not just make FocusableBorder thinner? That would be a good next step to give Combobox and DecoratedTextfield (the other FocusableBorder users) more vertical space for their content, but I think incremental fixes would be better for merging to M-38. FYI, DecoratedTextfield does not have the same defect (it uses a taller custom height for design reasons), but Combobox does. Other controls have similar defects, including the find bar textfield, and menu items, and various labels.
On 2014/08/28 21:29:14, msw wrote: > https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... > File ui/views/controls/textfield/textfield.cc (right): > > https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... > ui/views/controls/textfield/textfield.cc:281: // Use most of the > FocusableBorder's internal padding for text rendering. > On 2014/08/28 21:15:37, Peter Kasting wrote: > > Why not just make FocusableBorder thinner? > > That would be a good next step to give Combobox and DecoratedTextfield (the > other FocusableBorder users) more vertical space for their content, but I think > incremental fixes would be better for merging to M-38. Do we really need to fix this in M38? It seems like a minor enough glitch that we could just do the right thing on trunk and then eventually merge if it ends up looking safe and we decide we really need it. I'd be more OK with the current change if it didn't seem like just changing the FocusableBorder insets would be even simpler from a code perspective, and avoid needing to make any Textfield code change (I think?).
On 2014/08/28 21:31:56, Peter Kasting wrote: > On 2014/08/28 21:29:14, msw wrote: > > > https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... > > File ui/views/controls/textfield/textfield.cc (right): > > > > > https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... > > ui/views/controls/textfield/textfield.cc:281: // Use most of the > > FocusableBorder's internal padding for text rendering. > > On 2014/08/28 21:15:37, Peter Kasting wrote: > > > Why not just make FocusableBorder thinner? > > > > That would be a good next step to give Combobox and DecoratedTextfield (the > > other FocusableBorder users) more vertical space for their content, but I > think > > incremental fixes would be better for merging to M-38. > > Do we really need to fix this in M38? It seems like a minor enough glitch that > we could just do the right thing on trunk and then eventually merge if it ends > up looking safe and we decide we really need it. > > I'd be more OK with the current change if it didn't seem like just changing the > FocusableBorder insets would be even simpler from a code perspective, and avoid > needing to make any Textfield code change (I think?). It is an M-38 regression, and this early in M-38 I would recommend a merge. Even if I change FocusableBorder insets, I'll still need to change Textfield, Combobox, and DecoratedTextfield [instances] to retain their preferred height values and avoid collapsing our UI. I could try to do all of that for M-38 as well, I suppose. (I'm hoping that I'll also fix the find bar and perhaps more for M-38 anyway).
On 2014/08/28 21:55:41, msw wrote: > On 2014/08/28 21:31:56, Peter Kasting wrote: > > On 2014/08/28 21:29:14, msw wrote: > > > > > > https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... > > > File ui/views/controls/textfield/textfield.cc (right): > > > > > > > > > https://codereview.chromium.org/516943003/diff/1/ui/views/controls/textfield/... > > > ui/views/controls/textfield/textfield.cc:281: // Use most of the > > > FocusableBorder's internal padding for text rendering. > > > On 2014/08/28 21:15:37, Peter Kasting wrote: > > > > Why not just make FocusableBorder thinner? > > > > > > That would be a good next step to give Combobox and DecoratedTextfield (the > > > other FocusableBorder users) more vertical space for their content, but I > > think > > > incremental fixes would be better for merging to M-38. > > > > Do we really need to fix this in M38? It seems like a minor enough glitch > that > > we could just do the right thing on trunk and then eventually merge if it ends > > up looking safe and we decide we really need it. > > > > I'd be more OK with the current change if it didn't seem like just changing > the > > FocusableBorder insets would be even simpler from a code perspective, and > avoid > > needing to make any Textfield code change (I think?). > > It is an M-38 regression, and this early in M-38 I would recommend a merge. Even > if I change FocusableBorder insets, I'll still need to change Textfield, > Combobox, and DecoratedTextfield [instances] to retain their preferred height > values and avoid collapsing our UI. It would be nice to see what that change would look like, at least (since I'm not sure offhand what will be necessary). But if at that point you really want to land this change first, I don't object.
We could do something like Patch Set #2 or #3; PTAL.
Peter had offline advice; I'm going to try something else.
Peter, please take a look, thanks. I audited textfield subclasses and instantiations on codesearch, and it seemed like the omnibox was the only one that needed specific changes. I scrapped my fixes for password_generation_bubble_view.cc, because gcasto said that file is deprecated and should be deleted. DecoratedTextfield uses a larger custom preferred size and isn't affected. AppList's folder_header_view.cc, search_box_view.cc, and Ash's window_selector.cc use custom borders and will have increased preferred size, but they should be okay as-is.
Looks better! https://codereview.chromium.org/516943003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:667: leading_width += GetEditLeadingInternalSpace(); Hmm, I worry about this implicitly assuming that kItemPadding == Textfield::kTextPadding. I think these sorts of places should be doing "kItemPadding - Textfield::kTextPadding" instead. https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/combob... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/combob... ui/views/controls/combobox/combobox.cc:634: border->SetInsets(5, 10, 5, 10); Should we be doing something like "8 - Textfield::kTextPadding" here? Or do we always want 5/10 px of additional inset if we change the textfield padding amount in the future? https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/focusa... File ui/views/controls/focusable_border.cc (right): https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/focusa... ui/views/controls/focusable_border.cc:51: paint.setStrokeWidth(SkIntToScalar(2 * kInsetSize)); This probably deserves a comment on why the stroke width is larger than the inset size. https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/textfi... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/textfi... ui/views/controls/textfield/textfield.cc:924: // Allow the text to paint over vertical padding, but not horizontal padding. Nit: Maybe fill this comment out more? Something about why we have the padding we do by default, what cases might want to paint over the vertical padding, and why this is better than other solutions?
Addressed comments and fixed the find bar, there may be other regressions. I'd honestly rather just be painting over borders and calling it a day... https://codereview.chromium.org/516943003/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/location_bar/location_bar_view.cc:667: leading_width += GetEditLeadingInternalSpace(); On 2014/08/29 21:09:11, Peter Kasting wrote: > Hmm, I worry about this implicitly assuming that kItemPadding == > Textfield::kTextPadding. > > I think these sorts of places should be doing "kItemPadding - > Textfield::kTextPadding" instead. Done :-/ I also fixed the inversion of GetEditLeadingInternalSpace()... https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/combob... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/combob... ui/views/controls/combobox/combobox.cc:634: border->SetInsets(5, 10, 5, 10); On 2014/08/29 21:09:11, Peter Kasting wrote: > Should we be doing something like "8 - Textfield::kTextPadding" here? Or do we > always want 5/10 px of additional inset if we change the textfield padding > amount in the future? I don't think the intent is clear, so I figured I wouldn't complicate the code. (Combobox should eventually be a MenuButton or match LabelButton height.) https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/focusa... File ui/views/controls/focusable_border.cc (right): https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/focusa... ui/views/controls/focusable_border.cc:51: paint.setStrokeWidth(SkIntToScalar(2 * kInsetSize)); On 2014/08/29 21:09:11, Peter Kasting wrote: > This probably deserves a comment on why the stroke width is larger than the > inset size. I reverted this; it's just drawing a rect at the local bounds with a 2px stroke. (afaict, 1px is being painted outside the view bounds but it's clipped) https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/textfi... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/textfi... ui/views/controls/textfield/textfield.cc:924: // Allow the text to paint over vertical padding, but not horizontal padding. On 2014/08/29 21:09:11, Peter Kasting wrote: > Nit: Maybe fill this comment out more? Something about why we have the padding > we do by default, what cases might want to paint over the vertical padding, and > why this is better than other solutions? Done.
https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/focusa... File ui/views/controls/focusable_border.cc (right): https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/focusa... ui/views/controls/focusable_border.cc:51: paint.setStrokeWidth(SkIntToScalar(2 * kInsetSize)); On 2014/08/30 00:17:48, msw wrote: > On 2014/08/29 21:09:11, Peter Kasting wrote: > > This probably deserves a comment on why the stroke width is larger than the > > inset size. > > I reverted this; it's just drawing a rect at the local bounds with a 2px stroke. > (afaict, 1px is being painted outside the view bounds but it's clipped) Maybe then we should just draw a kInsetSize-px line centered kInsetSize/2 (floating point) pixels inset from the bounds? That seems clearer... https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:328: int left_margin = kMarginLeftOfFindTextfield - views::Textfield::kTextPadding; The changes here and below look like they only account for the textfield padding at the left edge. What about the padding at the right? Shouldn't that be taken out of kMarginLeftOfMatchCountLabel? https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:344: gfx::Size FindBarView::GetPreferredSize() const { It surprises me that this function doesn't seem to handle the match count label at all. https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:687: const int edge_padding = kItemPadding - views::Textfield::kTextPadding; Nit: Good idea on the constant, why not do it in GetPreferredSize() too? https://codereview.chromium.org/516943003/diff/100001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/100001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:927: // This is better than clipping text, shrinking text or painting over borders. Hmm, I'd really rather not talk about "previously", as over time it becomes more confusing than meaningful. How about this comment: The default kTextPadding value is chosen so there's a reasonable amount of whitespace on all sides of the user's default font. Because some fallback fonts may be taller than the default font, we allow fonts to paint over the vertical part of this whitespace area. There are a number of possible alternate behaviors, such as shrinking fallback fonts more, or dynamically enlarging the control, but this allows the most reasonable combination of "default font looks good", "fallback fonts are large enough to be readable", and "controls don't change size dynamically (surprising users)".
Please take another look; I'll continue auditing textfield users for sizing/layout changes, but will surely miss some. https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/focusa... File ui/views/controls/focusable_border.cc (right): https://codereview.chromium.org/516943003/diff/60001/ui/views/controls/focusa... ui/views/controls/focusable_border.cc:51: paint.setStrokeWidth(SkIntToScalar(2 * kInsetSize)); On 2014/08/30 00:46:42, Peter Kasting wrote: > On 2014/08/30 00:17:48, msw wrote: > > On 2014/08/29 21:09:11, Peter Kasting wrote: > > > This probably deserves a comment on why the stroke width is larger than the > > > inset size. > > > > I reverted this; it's just drawing a rect at the local bounds with a 2px > stroke. > > (afaict, 1px is being painted outside the view bounds but it's clipped) > > Maybe then we should just draw a kInsetSize-px line centered kInsetSize/2 > (floating point) pixels inset from the bounds? That seems clearer... Feel free to rewrite this separately. https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:328: int left_margin = kMarginLeftOfFindTextfield - views::Textfield::kTextPadding; On 2014/08/30 00:46:42, Peter Kasting wrote: > The changes here and below look like they only account for the textfield padding > at the left edge. What about the padding at the right? Shouldn't that be taken > out of kMarginLeftOfMatchCountLabel? Done, but that necessitates increasing kMarginLeftOfMatchCountLabel from 2px to 3px, otherwise the textfield and match count label bounds overlap. (as we discussed, this seems preferable to ensuring a painting z-order, and we apparently want ~3px of padding around textfield text anyway). https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:344: gfx::Size FindBarView::GetPreferredSize() const { On 2014/08/30 00:46:42, Peter Kasting wrote: > It surprises me that this function doesn't seem to handle the match count label > at all. The match count label is initially not shown and its preferred size changes with the length of the "XXX of YYY" text length. The whole find bar probably shouldn't change size when the label appears/disappears or when its contents change. You said offline that this should add some minimum match count label width, and reduce the textfield's preferred width for a fairly unchanged overall find bar preferred width, but I think that can be addressed in a followup patch. For now, I subtract 2*kTextPadding from the overall preferred width, because that's now inherently part of views::Textfield::GetPreferredSize (this leaves the overall find bar preferred size unchanged). https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:687: const int edge_padding = kItemPadding - views::Textfield::kTextPadding; On 2014/08/30 00:46:43, Peter Kasting wrote: > Nit: Good idea on the constant, why not do it in GetPreferredSize() too? Done. https://codereview.chromium.org/516943003/diff/100001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/100001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:927: // This is better than clipping text, shrinking text or painting over borders. On 2014/08/30 00:46:43, Peter Kasting wrote: > Hmm, I'd really rather not talk about "previously", as over time it becomes more > confusing than meaningful. How about this comment: > > The default kTextPadding value is chosen so there's a reasonable amount of > whitespace on all sides of the user's default font. Because some fallback fonts > may be taller than the default font, we allow fonts to paint over the vertical > part of this whitespace area. There are a number of possible alternate > behaviors, such as shrinking fallback fonts more, or dynamically enlarging the > control, but this allows the most reasonable combination of "default font looks > good", "fallback fonts are large enough to be readable", and "controls don't > change size dynamically (surprising users)". I revised the comment.
Ping! Peter, please take a look when you can. The bug is an M-38 stable blocker regression.
LGTM https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:41: const int kMarginLeftOfCloseButton = 3; Nit: Should we collapse this and kMarginLeftOfMatchCountLabel together, under the theory that it's really "kMarginRightOfTextfield"? https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:46: const int kMarginAboveFindTextfield = 6; Nit: Maybe this and the next constant should just be one? (I can't think of why we'd want to make the vertical spacing uneven.) https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:323: int find_text_y = kMarginAboveFindTextfield; Interesting, is it no longer possible to use the preferred height to compute this? https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:331: int margin_b = kMarginLeftOfMatchCountLabel - views::Textfield::kTextPadding; Nit: I'm not in love with "margin_a" and "margin_b" as names... maybe "left_margin" and roll the other temp into the width calculation below? https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:658: IncrementalMinimumWidth(translate_icon_view_) + Nit: I think the old indenting was more correct https://codereview.chromium.org/516943003/diff/160001/ui/app_list/views/folde... File ui/app_list/views/folder_header_view.cc (right): https://codereview.chromium.org/516943003/diff/160001/ui/app_list/views/folde... ui/app_list/views/folder_header_view.cc:171: 2 * views::Textfield::kTextPadding; Can this be calculated by setting |folder_name_view_|'s text and then using GetPreferredSize(), or something? https://codereview.chromium.org/516943003/diff/160001/ui/views/controls/focus... File ui/views/controls/focusable_border.cc (right): https://codereview.chromium.org/516943003/diff/160001/ui/views/controls/focus... ui/views/controls/focusable_border.cc:51: paint.setStrokeWidth(SkIntToScalar(2)); Can you file a bug against me to synchronize this code with kInsetSize as I suggested earlier?
Comments addressed. Scott, please take a look; thanks! [CC'ing some ui/app_list OWNERS and authors for heads up] https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:41: const int kMarginLeftOfCloseButton = 3; On 2014/09/05 22:56:48, Peter Kasting wrote: > Nit: Should we collapse this and kMarginLeftOfMatchCountLabel together, under > the theory that it's really "kMarginRightOfTextfield"? That might be reasonable for follow-up, but I'm not sure that kMarginLeftOfCloseButton is immediately right of the textfield if the match count label and the find next/prev buttons are all between the textfield and the close button, so I'll leave it as-is for now. https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:46: const int kMarginAboveFindTextfield = 6; On 2014/09/05 22:56:48, Peter Kasting wrote: > Nit: Maybe this and the next constant should just be one? (I can't think of why > we'd want to make the vertical spacing uneven.) Done. I'm not entirely sure that these values are optimal, since the IDR_TEXTFIELD assets have some nearly transparent pixels at the top and bottom of the interior space, but they match the current layout. https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:323: int find_text_y = kMarginAboveFindTextfield; On 2014/09/05 22:56:49, Peter Kasting wrote: > Interesting, is it no longer possible to use the preferred height to compute > this? This approach is preferable to the current code for several reasons: (1) The textfield is given the entire vertical space available to it, which may be needed if the fallback text exceeds the additional 6px that this change provides. (unlikely, but possible) (2) This lets the textfield center via the baseline technique, rather than combining that with some additional naive control centering logic within the vertical space. (3) The find bar textfield currentlt exceeds the available height with excessively tall system fonts (eg. Segoe UI 15pt), which causes the textfield control's white background to spill over the find bar assets. This change fixes that defect (but obviously we'll need to expand the control or clamp the font height [further] in the long term). (4) Avoiding potentially expensice GetPreferredSize calls is always good when the preferred size isn't actually needed. https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:331: int margin_b = kMarginLeftOfMatchCountLabel - views::Textfield::kTextPadding; On 2014/09/05 22:56:48, Peter Kasting wrote: > Nit: I'm not in love with "margin_a" and "margin_b" as names... maybe > "left_margin" and roll the other temp into the width calculation below? Done. https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): https://codereview.chromium.org/516943003/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_bar_view.cc:658: IncrementalMinimumWidth(translate_icon_view_) + On 2014/09/05 22:56:49, Peter Kasting wrote: > Nit: I think the old indenting was more correct This was done by git cl format, not me, but I've reverted it. https://codereview.chromium.org/516943003/diff/160001/ui/app_list/views/folde... File ui/app_list/views/folder_header_view.cc (right): https://codereview.chromium.org/516943003/diff/160001/ui/app_list/views/folde... ui/app_list/views/folder_header_view.cc:171: 2 * views::Textfield::kTextPadding; On 2014/09/05 22:56:49, Peter Kasting wrote: > Can this be calculated by setting |folder_name_view_|'s text and then using > GetPreferredSize(), or something? The preferred size of a textfield does not depend on its content, and surprisingly enough to me, textfield does not offer a way to get the size of the text content. Really, the right way to fix this is to use a textfield with centered horizontal alignment and static bounds. I tried that, but the placeholder text and cursor are not properly placed for centered textfields (which aren't common). I'll keep that work in mind for the future (we want to clobber gfx::Canvas::GetStringWidth where possible anyway). https://codereview.chromium.org/516943003/diff/160001/ui/views/controls/focus... File ui/views/controls/focusable_border.cc (right): https://codereview.chromium.org/516943003/diff/160001/ui/views/controls/focus... ui/views/controls/focusable_border.cc:51: paint.setStrokeWidth(SkIntToScalar(2)); On 2014/09/05 22:56:49, Peter Kasting wrote: > Can you file a bug against me to synchronize this code with kInsetSize as I > suggested earlier? Done. 411527: Cleanup FocusableBorder insets and painting code.
My last passing thought is that, if there are places currently referring directly to the textfield padding constant that could instead ask the textfield for its insets, that might be more future-proof.
calamity@chromium.org changed reviewers: + calamity@chromium.org
https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:101: kPadding - views::Textfield::kTextPadding); This will change the horizontal spacing of everything in this view. I don't think that's right. Since the search box had a NullBorder(), the original issue with the FocusableBorder doesn't apply? I think it's best to leave this as it was. The only visual difference will be that the text shifts 3px to the right (but read comment below first). https://codereview.chromium.org/516943003/diff/180001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/180001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:560: insets += gfx::Insets(kTextPadding, kTextPadding, kTextPadding, kTextPadding); Is there any reason that horizontal padding needs to be added here? This will change the x position of text for textfields that have custom borders (such as the ones in the app list). Can you have FocusableBorder do the side borders while this does the top and bottom padding? If you do this, the app list will be visually unchanged and you can remove all the changes from folder_header_view and search_box_view.
https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:101: kPadding - views::Textfield::kTextPadding); On 2014/09/08 03:37:50, calamity wrote: > This will change the horizontal spacing of everything in this view. I don't > think that's right. Practically-speaking, what's the problematic effect here? Reducing the padding between the menu button and the speech button? > Since the search box had a NullBorder(), the original issue with the > FocusableBorder doesn't apply? The bug wasn't in FocusableBorder, it was that the Textfield wasn't previously enforcing padding on all sides of its content for all callers. See my reply to your other comment. https://codereview.chromium.org/516943003/diff/180001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/180001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:560: insets += gfx::Insets(kTextPadding, kTextPadding, kTextPadding, kTextPadding); On 2014/09/08 03:37:50, calamity wrote: > Is there any reason that horizontal padding needs to be added here? This will > change the x position of text for textfields that have custom borders (such as > the ones in the app list). > > Can you have FocusableBorder do the side borders while this does the top and > bottom padding? > > If you do this, the app list will be visually unchanged and you can remove all > the changes from folder_header_view and search_box_view. I vetoed this route, because the side padding isn't actually a property of focusable borders, it's a property of textfields. This change basically says that we always want padding around the textfield contents, and any callers that don't apply that padding are actually wrong as-is, so their behavior needs to change. In other words, it's intentional that this change might not preserve existing behavior.
On 2014/09/06 00:00:22, Peter Kasting wrote: > My last passing thought is that, if there are places currently referring > directly to the textfield padding constant that could instead ask the textfield > for its insets, that might be more future-proof. Do you suggest that I override Textfield::GetInsets (ditto for Combobox?) to include the new horizontal padding, then only add the vertical padding in GetPreferredSize? That would allow LocationBarView, SearchBoxView, and FolderHeaderView to use the textfield insets, instead of using the constant directly.
On 2014/09/08 19:36:13, msw wrote: > On 2014/09/06 00:00:22, Peter Kasting wrote: > > My last passing thought is that, if there are places currently referring > > directly to the textfield padding constant that could instead ask the > textfield > > for its insets, that might be more future-proof. > > Do you suggest that I override Textfield::GetInsets (ditto for Combobox?) to > include the new horizontal padding, then only add the vertical padding in > GetPreferredSize? That would allow LocationBarView, SearchBoxView, and > FolderHeaderView to use the textfield insets, instead of using the constant > directly. I had been thinking of including all sides in the insets, and merely having callers currently using the constant use the insets instead. I don't know how feasible that is, I didn't think deeply about the idea.
On 2014/09/08 19:38:37, Peter Kasting wrote: > On 2014/09/08 19:36:13, msw wrote: > > On 2014/09/06 00:00:22, Peter Kasting wrote: > > > My last passing thought is that, if there are places currently referring > > > directly to the textfield padding constant that could instead ask the > > textfield > > > for its insets, that might be more future-proof. > > > > Do you suggest that I override Textfield::GetInsets (ditto for Combobox?) to > > include the new horizontal padding, then only add the vertical padding in > > GetPreferredSize? That would allow LocationBarView, SearchBoxView, and > > FolderHeaderView to use the textfield insets, instead of using the constant > > directly. > > I had been thinking of including all sides in the insets, and merely having > callers currently using the constant use the insets instead. I don't know how > feasible that is, I didn't think deeply about the idea. Is patch Set 11 what you want?
https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:101: kPadding - views::Textfield::kTextPadding); On 2014/09/08 17:45:59, Peter Kasting wrote: > On 2014/09/08 03:37:50, calamity wrote: > > This will change the horizontal spacing of everything in this view. I don't > > think that's right. > > Practically-speaking, what's the problematic effect here? Reducing the padding > between the menu button and the speech button? > > > Since the search box had a NullBorder(), the original issue with the > > FocusableBorder doesn't apply? > > The bug wasn't in FocusableBorder, it was that the Textfield wasn't previously > enforcing padding on all sides of its content for all callers. See my reply to > your other comment. > I see. In that case, we should leave the code as it was. The effect of this change is that the padding between all subviews of the search box (i.e the search icon, textfield, menu and microphone) will have less padding between them. https://codereview.chromium.org/516943003/diff/180001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/180001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:560: insets += gfx::Insets(kTextPadding, kTextPadding, kTextPadding, kTextPadding); On 2014/09/08 17:45:59, Peter Kasting wrote: > On 2014/09/08 03:37:50, calamity wrote: > > Is there any reason that horizontal padding needs to be added here? This will > > change the x position of text for textfields that have custom borders (such as > > the ones in the app list). > > > > Can you have FocusableBorder do the side borders while this does the top and > > bottom padding? > > > > If you do this, the app list will be visually unchanged and you can remove all > > the changes from folder_header_view and search_box_view. > > I vetoed this route, because the side padding isn't actually a property of > focusable borders, it's a property of textfields. This change basically says > that we always want padding around the textfield contents, and any callers that > don't apply that padding are actually wrong as-is, so their behavior needs to > change. > > In other words, it's intentional that this change might not preserve existing > behavior. Ack.
Yeah, something like this looks cleaner. https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:101: kPadding - views::Textfield::kTextPadding); On 2014/09/09 00:46:38, calamity wrote: > On 2014/09/08 17:45:59, Peter Kasting wrote: > > On 2014/09/08 03:37:50, calamity wrote: > > > This will change the horizontal spacing of everything in this view. I don't > > > think that's right. > > > > Practically-speaking, what's the problematic effect here? Reducing the > padding > > between the menu button and the speech button? > > > > > Since the search box had a NullBorder(), the original issue with the > > > FocusableBorder doesn't apply? > > > > The bug wasn't in FocusableBorder, it was that the Textfield wasn't previously > > enforcing padding on all sides of its content for all callers. See my reply > to > > your other comment. > > > > I see. In that case, we should leave the code as it was. The effect of this > change is that the padding between all subviews of the search box (i.e the > search icon, textfield, menu and microphone) will have less padding between > them. But the textfield padding is increasing. Compensating for that is why this line was changed. That's why I asked specifically about the menu button to speech button distance, because anything adjacent to a Textfield is _not_ going to change its overall padding. https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:49: static const int kTextPadding; It looks like search_box_view.cc and combobox.cc are the remaining references to this. Can they be changed to use inset values as well? That'd let us move this to be .cc-file-static.
Thanks for clarifying, lgtm. https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... File ui/app_list/views/search_box_view.cc (right): https://codereview.chromium.org/516943003/diff/180001/ui/app_list/views/searc... ui/app_list/views/search_box_view.cc:101: kPadding - views::Textfield::kTextPadding); On 2014/09/09 01:33:29, Peter Kasting wrote: > On 2014/09/09 00:46:38, calamity wrote: > > On 2014/09/08 17:45:59, Peter Kasting wrote: > > > On 2014/09/08 03:37:50, calamity wrote: > > > > This will change the horizontal spacing of everything in this view. I > don't > > > > think that's right. > > > > > > Practically-speaking, what's the problematic effect here? Reducing the > > padding > > > between the menu button and the speech button? > > > > > > > Since the search box had a NullBorder(), the original issue with the > > > > FocusableBorder doesn't apply? > > > > > > The bug wasn't in FocusableBorder, it was that the Textfield wasn't > previously > > > enforcing padding on all sides of its content for all callers. See my reply > > to > > > your other comment. > > > > > > > I see. In that case, we should leave the code as it was. The effect of this > > change is that the padding between all subviews of the search box (i.e the > > search icon, textfield, menu and microphone) will have less padding between > > them. > > But the textfield padding is increasing. Compensating for that is why this line > was changed. That's why I asked specifically about the menu button to speech > button distance, because anything adjacent to a Textfield is _not_ going to > change its overall padding. Ah ok. Yeah. It will reduce that padding. At the moment, those two buttons don't show together but that could change in the future. We can cross that bridge when we get to it.
LGTM https://codereview.chromium.org/516943003/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/cookie_info_view.cc (left): https://codereview.chromium.org/516943003/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/cookie_info_view.cc:128: layout->AddPaddingRow(0, kExtraLineHeightPadding); Why did you remove this?
https://codereview.chromium.org/516943003/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/cookie_info_view.cc (left): https://codereview.chromium.org/516943003/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/cookie_info_view.cc:128: layout->AddPaddingRow(0, kExtraLineHeightPadding); On 2014/09/09 15:18:01, sky wrote: > Why did you remove this? The internal padding of the textfield now offers enough vertical buffer for this layout. Twice the old amount, in fact. If you prefer, I can keep the constant and subtract the insets for a padding row of 3-3=0 (or 3-6...), like Peter requested for the location bar. https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:565: const gfx::Insets insets = GetInsets(); MSW: NEED TO RESTORE CONST REF. https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:49: static const int kTextPadding; On 2014/09/09 01:33:29, Peter Kasting wrote: > It looks like search_box_view.cc and combobox.cc are the remaining references to > this. Can they be changed to use inset values as well? That'd let us move this > to be .cc-file-static. Not in a clearly good way. (1) search_box_view.cc is specifying the padding between BoxLayout views (on the left and the right of the textfield) in a single value, |kPadding - views::Textfield::kTextPadding|. I could use the (a) left or right inset value or (c) divide the overall insets width by two, or (d) rewrite the layout... (2) combobox.cc doesn't have a textfield instance, I could (a) construct a textfield instance to get its insets, or (b) duplicate the constant value. Neither seems great, but I'd do (2b) if we resolve 1...
https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:49: static const int kTextPadding; On 2014/09/09 17:01:06, msw wrote: > (1) search_box_view.cc is specifying the padding between BoxLayout views (on the > left and the right of the textfield) in a single value, |kPadding - > views::Textfield::kTextPadding|. I could use the (a) left or right inset value > or (c) divide the overall insets width by two, or (d) rewrite the layout... It'd probably be OK to do (a) and claim that Textfield's insets are always going to be symmetrical, but it's not so awesome that I'm going to push you to do it. > (2) combobox.cc doesn't have a textfield instance, I could (a) construct a > textfield instance to get its insets, or (b) duplicate the constant value. > Neither seems great, but I'd do (2b) if we resolve 1... Oh, so the combobox is just trying to mirror the look of the Textfield, not actually use one? Hmm. Not sure what the best answer is there. Maybe hoist the textfield padding constant to be an overall layout constant, or make a public static Textfield::GetPadding() method, or something... Nothing seems terrifically awesome, so you're fine doing whatever.
https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/516943003/diff/200001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:49: static const int kTextPadding; On 2014/09/09 18:27:17, Peter Kasting wrote: > On 2014/09/09 17:01:06, msw wrote: > > (1) search_box_view.cc is specifying the padding between BoxLayout views (on > the > > left and the right of the textfield) in a single value, |kPadding - > > views::Textfield::kTextPadding|. I could use the (a) left or right inset value > > or (c) divide the overall insets width by two, or (d) rewrite the layout... > > It'd probably be OK to do (a) and claim that Textfield's insets are always going > to be symmetrical, but it's not so awesome that I'm going to push you to do it. > > > (2) combobox.cc doesn't have a textfield instance, I could (a) construct a > > textfield instance to get its insets, or (b) duplicate the constant value. > > Neither seems great, but I'd do (2b) if we resolve 1... > > Oh, so the combobox is just trying to mirror the look of the Textfield, not > actually use one? Hmm. Not sure what the best answer is there. Maybe hoist > the textfield padding constant to be an overall layout constant, or make a > public static Textfield::GetPadding() method, or something... > > Nothing seems terrifically awesome, so you're fine doing whatever. Then I'll leave this as-is. I see lots of additional opportunities for cleanup and consolidation, but I need to transition away from my UI responsibilities.
The CQ bit was checked by msw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/516943003/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as 4602d091c873f560e048779ba9f80ff1a2617ecc
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a259c9b832bcbc4a641f1696071df39e028a5c2c Cr-Commit-Position: refs/heads/master@{#294016} |
