|
|
DescriptionModified OverviewMode's LabelButton bounds to cover the entire item.
This way, we don't have to worry about custom targeting. A minor change
had to be done to the LabelButton view code to avoid printing text on
a view border.
BUG=453441
Committed: https://crrev.com/8cdfe63203195b79686813d7cbd3029411ceba79
Cr-Commit-Position: refs/heads/master@{#315596}
Patch Set 1 #Patch Set 2 : Removed wrong comment #Patch Set 3 : Removed unnecessary changes from unittests #
Total comments: 8
Patch Set 4 : Changed how we modify label opacity to old, working way #Patch Set 5 : Removed ignore_border_overlap from label_button as per msw suggestion. #
Total comments: 12
Patch Set 6 : Reworked LabelButton changes. #
Total comments: 4
Patch Set 7 : Fixed nits #
Total comments: 1
Messages
Total messages: 32 (10 generated)
nsatragno@chromium.org changed reviewers: + flackr@chromium.org
nsatragno@chromium.org changed required reviewers: + flackr@chromium.org
Hi Rob, Can you give this patch a first look? Thanks!
ash/wm lgtm, I'm curious about why we need to ignore the vertical border by default though. https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... ui/views/controls/button/label_button.cc:327: ignore_vertical_overlap_ ? 0 : child_area.y()); Out of curiosity, can you tell from the git history if ignoring the border was added on purpose? I wonder if it was just overlooked, or if it's only required for a particular view perhaps we could respect the border overlap by default and only ignore it for that particular case.
nsatragno@chromium.org changed reviewers: + sadrul@chromium.org
nsatragno@chromium.org changed required reviewers: + sadrul@chromium.org
Hi Sadrul, Can you please give /ui/* a look on this patch? Rob, please see my reply on the comment. Thanks! https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... ui/views/controls/button/label_button.cc:327: ignore_vertical_overlap_ ? 0 : child_area.y()); On 2015/01/30 15:34:04, flackr wrote: > Out of curiosity, can you tell from the git history if ignoring the border was > added on purpose? I wonder if it was just overlooked, or if it's only required > for a particular view perhaps we could respect the border overlap by default and > only ignore it for that particular case. It's required by the bookmark bar: https://codereview.chromium.org/424663008 https://code.google.com/p/chromium/issues/detail?id=397995#c3 So the original behaviour was respecting the border and it was changed later. Wouldn't ignoring it for a particular case require repetition of most code on Layout()?
tdanderson@chromium.org changed reviewers: + sky@chromium.org, tdanderson@chromium.org
tdanderson@chromium.org changed required reviewers: - sadrul@chromium.org
Hi Scott, would you mind taking a look at the changes in ui/views?
sky@chromium.org changed reviewers: + msw@chromium.org
+msw https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... ui/views/controls/button/label_button.cc:327: ignore_vertical_overlap_ ? 0 : child_area.y()); On 2015/01/30 15:44:32, Nicolás wrote: > On 2015/01/30 15:34:04, flackr wrote: > > Out of curiosity, can you tell from the git history if ignoring the border was > > added on purpose? I wonder if it was just overlooked, or if it's only required > > for a particular view perhaps we could respect the border overlap by default > and > > only ignore it for that particular case. > > It's required by the bookmark bar: > https://codereview.chromium.org/424663008 > https://code.google.com/p/chromium/issues/detail?id=397995#c3 > So the original behaviour was respecting the border and it was changed later. > Wouldn't ignoring it for a particular case require repetition of most code on > Layout()? This seems suspicious to me. If BookmarkBar wants custom behavior it should be tweaking something, not everyone else.
We can always make |ignore_vertical_overlap_| default to false and only set in to true for the bookmark bar. This way we would satisfy every condition, right?
On 2015/02/03 22:11:59, Nicolás wrote: > We can always make |ignore_vertical_overlap_| default to false and only set in > to true for the bookmark bar. This way we would satisfy every condition, right? Would it be possible to update BookmarkButton to do the right thing (e.g. override GetChildAreaBounds()), instead of having LabelButton explicitly do this?
https://codereview.chromium.org/872113004/diff/40001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/872113004/diff/40001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:288: window_label_button_view_->set_ignore_vertical_overlap(false); Would you mind posting a screenshot of what this looks like with false versus true? I'm curious to see why clipping text is preferable to allowing overlap with the border. https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... ui/views/controls/button/label_button.cc:327: ignore_vertical_overlap_ ? 0 : child_area.y()); On 2015/02/03 21:40:02, sky wrote: > On 2015/01/30 15:44:32, Nicolás wrote: > > On 2015/01/30 15:34:04, flackr wrote: > > > Out of curiosity, can you tell from the git history if ignoring the border > was > > > added on purpose? I wonder if it was just overlooked, or if it's only > required > > > for a particular view perhaps we could respect the border overlap by default > > and > > > only ignore it for that particular case. > > > > It's required by the bookmark bar: > > https://codereview.chromium.org/424663008 > > https://code.google.com/p/chromium/issues/detail?id=397995#c3 > > So the original behaviour was respecting the border and it was changed later. > > Wouldn't ignoring it for a particular case require repetition of most code on > > Layout()? > > This seems suspicious to me. If BookmarkBar wants custom behavior it should be > tweaking something, not everyone else. Ignoring the vertical insets for text rendering was done to match the legacy behavior of view::TextButton, it wasn't just for bookmark bar buttons. The bookmark bar was just a prominent case where this change was needed (because it clamps buttons to a fixed short height). Keeping the default as true for |ignore_vertical_overlap_| seems okay to me; otherwise we'll need to audit all the LabelButton instances to ensure that this wouldn't cut off tall text (for example when fallback font heights exceed the system font height, which seems to often be the case for Hindi text on Windows 7, etc.).
Please see my comments below, thanks. https://codereview.chromium.org/872113004/diff/40001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/872113004/diff/40001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:288: window_label_button_view_->set_ignore_vertical_overlap(false); On 2015/02/03 23:50:19, msw wrote: > Would you mind posting a screenshot of what this looks like with false versus > true? I'm curious to see why clipping text is preferable to allowing overlap > with the border. Please see screenshots attached to https://crbug.com/453441. We are using the border as a clickable transparent margin on top of the label. https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... ui/views/controls/button/label_button.cc:327: ignore_vertical_overlap_ ? 0 : child_area.y()); On 2015/02/03 23:50:19, msw wrote: > On 2015/02/03 21:40:02, sky wrote: > > On 2015/01/30 15:44:32, Nicolás wrote: > > > On 2015/01/30 15:34:04, flackr wrote: > > > > Out of curiosity, can you tell from the git history if ignoring the border > > was > > > > added on purpose? I wonder if it was just overlooked, or if it's only > > required > > > > for a particular view perhaps we could respect the border overlap by > default > > > and > > > > only ignore it for that particular case. > > > > > > It's required by the bookmark bar: > > > https://codereview.chromium.org/424663008 > > > https://code.google.com/p/chromium/issues/detail?id=397995#c3 > > > So the original behaviour was respecting the border and it was changed > later. > > > Wouldn't ignoring it for a particular case require repetition of most code > on > > > Layout()? > > > > This seems suspicious to me. If BookmarkBar wants custom behavior it should be > > tweaking something, not everyone else. > > Ignoring the vertical insets for text rendering was done to match the legacy > behavior of view::TextButton, it wasn't just for bookmark bar buttons. The > bookmark bar was just a prominent case where this change was needed (because it > clamps buttons to a fixed short height). Keeping the default as true for > |ignore_vertical_overlap_| seems okay to me; otherwise we'll need to audit all > the LabelButton instances to ensure that this wouldn't cut off tall text (for > example when fallback font heights exceed the system font height, which seems to > often be the case for Hindi text on Windows 7, etc.). Acknowledged.
https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/872113004/diff/40001/ui/views/controls/button... ui/views/controls/button/label_button.cc:327: ignore_vertical_overlap_ ? 0 : child_area.y()); On 2015/02/04 16:20:11, Nicolás wrote: > On 2015/02/03 23:50:19, msw wrote: > > On 2015/02/03 21:40:02, sky wrote: > > > On 2015/01/30 15:44:32, Nicolás wrote: > > > > On 2015/01/30 15:34:04, flackr wrote: > > > > > Out of curiosity, can you tell from the git history if ignoring the > border > > > was > > > > > added on purpose? I wonder if it was just overlooked, or if it's only > > > required > > > > > for a particular view perhaps we could respect the border overlap by > > default > > > > and > > > > > only ignore it for that particular case. > > > > > > > > It's required by the bookmark bar: > > > > https://codereview.chromium.org/424663008 > > > > https://code.google.com/p/chromium/issues/detail?id=397995#c3 > > > > So the original behaviour was respecting the border and it was changed > > later. > > > > Wouldn't ignoring it for a particular case require repetition of most code > > on > > > > Layout()? > > > > > > This seems suspicious to me. If BookmarkBar wants custom behavior it should > be > > > tweaking something, not everyone else. > > > > Ignoring the vertical insets for text rendering was done to match the legacy > > behavior of view::TextButton, it wasn't just for bookmark bar buttons. The > > bookmark bar was just a prominent case where this change was needed (because > it > > clamps buttons to a fixed short height). Keeping the default as true for > > |ignore_vertical_overlap_| seems okay to me; otherwise we'll need to audit all > > the LabelButton instances to ensure that this wouldn't cut off tall text (for > > example when fallback font heights exceed the system font height, which seems > to > > often be the case for Hindi text on Windows 7, etc.). > > Acknowledged. So it sounds like if we ignore borders for the purpose of clipping (i.e. label_size) but respect them for positioning we could do away with ignore_vertical_overlap_ right?
I pulled your CL and was overlaying the swipe-to-close feature work and I noticed that the window labels disappear in overview mode when using the text input filter. Can you double check that this isn't a regression caused by this patch.
You might be able to solve this by having the Label bounds respect GetChildAreaBounds, but not use GetInsets(), then have your button override GetChildAreaBounds. You could probably also disable clipping by plumbing a views::Label setting through to RenderText::set_clip_to_display_rect(false)... ( that will be easier after views::Label is rewritten to cache RenderText objects - https://codereview.chromium.org/867003002 )
On 2015/02/04 16:56:43, bruthig wrote: > I pulled your CL and was overlaying the swipe-to-close feature work and I > noticed that the window labels disappear in overview mode when using the text > input filter. Can you double check that this isn't a regression caused by this > patch. Thanks for the report. I changed the way we set the opacity to how we did it before (directly on the layer) and now it works well.
msw@, please review label_button.[h|cc]. I modified the Layout code to use GetChildItemBounds(). flackr@, can you give a quick look at the changes on window_selector_item.cc? Let me know if you have any questions. Thanks, Nico.
I *really* recommend just using two buttons (one for the image at the top, and one for the text at the bottom), to avoid the complexity of customizing LabelButton layout for this unusual vertical case. Using two buttons here ought to be quite easy, since they won't appear disjoint if they just use an empty border. LabelButton wasn't written with vertical layout in mind, and while supporting that correctly might be a reasonable request, this CL isn't the best workaround to support your use case. This might work as a temporary hack, but it won't support vertical layouts in a general and correct way. https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:75: bounds.Inset(GetInsets()); This will double-apply border insets, instead, here you should do: gfx::Rect bounds = GetLocalBounds(); bounds.Inset(<height of the top image and its padding>, 0, 0, 0); I think the right way to do this is to have a custom button view with the unique vertical layout, or following my general advice of just using two buttons... https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:295: window_label_button_view_->SetTextColor(views::LabelButton::STATE_HOVERED, I don't think you need to explicitly set the text colors for hovered and pressed, they should inherit from the normal state's color. https://codereview.chromium.org/872113004/diff/80001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/872113004/diff/80001/ui/views/controls/button... ui/views/controls/button/label_button.cc:290: // By default, GetChildAreaBounds() ignores the top and bottom border, but we Flip around some of this code: GetChildAreaBounds shouldn't apply any border insets (MenuButton and CredentialsItemView already rely on that behavior). Instead have this code do something like: gfx::Rect child_area(GetChildAreaBounds()); gfx::Rect label_area(child_area); gfx::Insets insets(GetInsets()); child_area.Inset(insets); // Labels can paint over the vertical component of the border insets. label_area.Inset(insets.left(), 0, insets.right(), 0); gfx::Size image_size(image_->GetPreferredSize()); image_size.SetToMin(child_area.size()); // The label takes any remaining width after sizing the image, unless both // views are centered. In that case, using the tighter preferred label width // avoids wasted space within the label that would look like awkward padding. <REMOVE THE LAST LINE OF THE COMMENT...> gfx::Size label_size(label_area.size()); https://codereview.chromium.org/872113004/diff/80001/ui/views/controls/button... ui/views/controls/button/label_button.cc:327: gfx::Point label_origin(child_area.origin()); Then change this to: gfx::Point label_origin(label_area.origin()); https://codereview.chromium.org/872113004/diff/80001/ui/views/controls/button... ui/views/controls/button/label_button.cc:352: gfx::Rect bounds = GetLocalBounds(); Revert this to just return GetLocalBounds().
msw@, please see my attached comments and changes. I decided to refactor the code following your new suggestions, while still using only one button to play well with the a11y framework. https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:75: bounds.Inset(GetInsets()); On 2015/02/06 21:24:26, msw wrote: > This will double-apply border insets, instead, here you should do: > gfx::Rect bounds = GetLocalBounds(); > bounds.Inset(<height of the top image and its padding>, 0, 0, 0); Done. > I think the right way to do this is to have a custom button view with the unique > vertical layout, But then we would have to repeat much of the existing logic, right? Especially in light of crbug.com/361182 which can benefit from the existing ability to add an image to the label. > or following my general advice of just using two buttons... This would not work as the a11y framework would give the user confusing feedback, there shouldn't be two buttons for the same action. https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:295: window_label_button_view_->SetTextColor(views::LabelButton::STATE_HOVERED, On 2015/02/06 21:24:26, msw wrote: > I don't think you need to explicitly set the text colors for hovered and > pressed, they should inherit from the normal state's color. It seems they don't - removing the lines makes the button change colour when it is hovered. https://codereview.chromium.org/872113004/diff/80001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/872113004/diff/80001/ui/views/controls/button... ui/views/controls/button/label_button.cc:290: // By default, GetChildAreaBounds() ignores the top and bottom border, but we On 2015/02/06 21:24:26, msw wrote: > Flip around some of this code: GetChildAreaBounds shouldn't apply any border > insets (MenuButton and CredentialsItemView already rely on that behavior). > > Instead have this code do something like: > gfx::Rect child_area(GetChildAreaBounds()); > gfx::Rect label_area(child_area); > > gfx::Insets insets(GetInsets()); > child_area.Inset(insets); > // Labels can paint over the vertical component of the border insets. > label_area.Inset(insets.left(), 0, insets.right(), 0); > > gfx::Size image_size(image_->GetPreferredSize()); > image_size.SetToMin(child_area.size()); > > // The label takes any remaining width after sizing the image, unless both > // views are centered. In that case, using the tighter preferred label width > // avoids wasted space within the label that would look like awkward padding. > <REMOVE THE LAST LINE OF THE COMMENT...> > gfx::Size label_size(label_area.size()); Done. The idea behind the way I had originally written it was to avoid having the LabelButton subclass know the detail that its parent class only ignores the vertical button padding. https://codereview.chromium.org/872113004/diff/80001/ui/views/controls/button... ui/views/controls/button/label_button.cc:327: gfx::Point label_origin(child_area.origin()); On 2015/02/06 21:24:26, msw wrote: > Then change this to: gfx::Point label_origin(label_area.origin()); Done. https://codereview.chromium.org/872113004/diff/80001/ui/views/controls/button... ui/views/controls/button/label_button.cc:352: gfx::Rect bounds = GetLocalBounds(); On 2015/02/06 21:24:26, msw wrote: > Revert this to just return GetLocalBounds(). Done.
lgtm with nits, sorry for the runaround here... Feel free to add proper vertical layout support to LabelButton in a future CL, I'll gladly review. https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:75: bounds.Inset(GetInsets()); On 2015/02/09 16:35:09, Nicolás wrote: > On 2015/02/06 21:24:26, msw wrote: > > This will double-apply border insets, instead, here you should do: > > gfx::Rect bounds = GetLocalBounds(); > > bounds.Inset(<height of the top image and its padding>, 0, 0, 0); > > Done. > > > I think the right way to do this is to have a custom button view with the > unique > > vertical layout, > > But then we would have to repeat much of the existing logic, right? Especially > in light of crbug.com/361182 which can benefit from the existing ability to add > an image to the label. > > > or following my general advice of just using two buttons... > > This would not work as the a11y framework would give the user confusing > feedback, there shouldn't be two buttons for the same action. Acknowledged. https://codereview.chromium.org/872113004/diff/80001/ash/wm/overview/window_s... ash/wm/overview/window_selector_item.cc:295: window_label_button_view_->SetTextColor(views::LabelButton::STATE_HOVERED, On 2015/02/09 16:35:09, Nicolás wrote: > On 2015/02/06 21:24:26, msw wrote: > > I don't think you need to explicitly set the text colors for hovered and > > pressed, they should inherit from the normal state's color. > > It seems they don't - removing the lines makes the button change colour when it > is hovered. Acknowledged. Sorry, it probably should behave as I suggested... https://codereview.chromium.org/872113004/diff/100001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/872113004/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:289: views::Border::CreateEmptyBorder(0, 0, 0, 0)); nit: I think you can use views::Border::NullBorder() here. https://codereview.chromium.org/872113004/diff/100001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/872113004/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:39: ~OverviewLabelButton() override {} nit: don't define this virtual dtor inline in the header. https://codereview.chromium.org/872113004/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:44: gfx::Rect GetChildAreaBounds() override; nit: add the overrides section comment: "// views::LabelButton:" https://codereview.chromium.org/872113004/diff/100001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.h:49: }; nit: DISALLOW_COPY_AND_ASSIGN(OverviewLabelButton);
New patchsets have been uploaded after l-g-t-m from msw@chromium.org
LGTM
The CQ bit was checked by nsatragno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/872113004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8cdfe63203195b79686813d7cbd3029411ceba79 Cr-Commit-Position: refs/heads/master@{#315596}
Message was sent while issue was closed.
oshima@chromium.org changed reviewers: + oshima@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/872113004/diff/120001/ash/wm/overview/window_... File ash/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/872113004/diff/120001/ash/wm/overview/window_... ash/wm/overview/window_selector_item.cc:104: : LabelButton(listener, text) { you need to initialize top_padding_ here. |