Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810403002:
Views: Don't add insets for views::Link focus rings under MD. (Closed)
DescriptionViews: Don't add insets for views::Link focus rings under MD.
Insets for dashed focus rings are throwing off the text alignment on
dialogs. Currently they are added to any Link and any StyledLabel that
contains a Link.
Dashed focus rings have already been phased out from most places in
Chrome. On a views::Link, they are still needed when the link is
permanently underlined since that's the only way to indicate focus.
To contain the insanity, this CL makes assumptions (both with and
without MD).
- only views::Link ever wants a focus ring - not vanilla views::Labels
(effectively, a vanilla Label is never focusable).
- If a views::Link consumer calls SetUnderline(true) then it wants a
focus ring (even under MD). This happens (sometimes) for the
BookmarkBarInstructionsView and nothing else.
- If SetUnderline is not called it wants the "default"
- Nothing overrides Label::GetInsets to add things that are not focus
rings (StyledLabel was already making this assumption).
- Under MD only, views::StyledLabel consumers never want underlines on
links (this is currently the case).
- Under Non-MD only, calling SetUnderline(false) removes the underline
but does not change the focus style.
This allows a lot of the "focus ring" logic to move down into
views::Link where it will be easier to delete later. Calculating the
bounds stays in views::Label since it inspects Label's private
RenderText instances.
BUG=711161
Review-Url: https://codereview.chromium.org/2810403002
Cr-Commit-Position: refs/heads/master@{#466211}
Committed: https://chromium.googlesource.com/chromium/src/+/480f457f6939eade256dc987fd740452ef5912de
Patch Set 1 #Patch Set 2 : Self review #Patch Set 3 : Tests passing #
Total comments: 7
Patch Set 4 : respond to comments #Patch Set 5 : Update tests, but I need to rebase to add the Harmony one #Patch Set 6 : rebase #Patch Set 7 : add harmony test from crrev/2834603002 #Patch Set 8 : zap width check: concrete_link is not added to a Widget so can differ #Patch Set 9 : self nits #
Total comments: 2
Patch Set 10 : Add missing // namespace comments #
Messages
Total messages: 60 (50 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't add StyledLabel insets under MD cl format Golly cl format progress - bugs the test cl format Stash BUG= ========== to ========== Don't add StyledLabel insets under MD To contain the insanity, this makes a few assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" BUG=711161 ==========
Description was changed from ========== Don't add StyledLabel insets under MD To contain the insanity, this makes a few assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" BUG=711161 ========== to ========== Don't add StyledLabel insets under MD To contain the insanity, this makes a few assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" BUG=711161 ==========
Description was changed from ========== Don't add StyledLabel insets under MD To contain the insanity, this makes a few assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this makes a few assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" BUG=711161 ==========
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this makes a few assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this CL makes assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" BUG=711161 ==========
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this CL makes assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this CL makes assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). BUG=711161 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this CL makes assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this CL makes assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - only views::Link ever wants to inset for a focus ring - not vanilla views::Labels. BUG=711161 ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this CL makes assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If a SetUnderline(false) is called, then it wants no focus ring - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - only views::Link ever wants to inset for a focus ring - not vanilla views::Labels. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this CL makes assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then it wants no focus ring (underline to indicate focus) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - only views::Link ever wants to inset for a focus ring - not vanilla views::Labels. BUG=711161 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. To contain the insanity, this CL makes assumptions: - Under MD, views::StyledLabel consumers never want underlines on links. and, under both MD and non-MD: - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then it wants no focus ring (underline to indicate focus) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - only views::Link ever wants to inset for a focus ring - not vanilla views::Labels. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions: - only views::Link ever wants to a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then it wants no focus ring (underline to indicate focus) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. BUG=711161 ==========
tapted@chromium.org changed reviewers: + sky@chromium.org
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions: - only views::Link ever wants to a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then it wants no focus ring (underline to indicate focus) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants to a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays since that inspects Label's private RenderText instances. BUG=711161 ==========
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants to a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays since that inspects Label's private RenderText instances. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants to a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays since that inspects Label's private RenderText instances. BUG=711161 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants to a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays since that inspects Label's private RenderText instances. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants to a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays since that inspects Label's private RenderText instances. BUG=711161 ==========
Hi sky (cc pkasting/estade) I was hoping to get some feedback on this approach. I still need to add tests for the codepaths when --secondary-ui-md is flipped, but I think the code is correct. I'm wondering about your thoughts about the list of tradeoffs in the CL description, or if you have ideas for better ways to do this. E.g. - Is it safe to assume that any views::Label that needs a focus ring will be a views::Link? or (more extreme) - How bad would it be to scrap the focus ring everywhere, immediately (_before_ flipping --secondary-ui-md), and just indicate focus with an underline system wide? (the CL currently does the first, but not the second). Thanks in advance. (there's a red linux bot - but I think it's just a font kerning thing) https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (left): https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label... ui/views/controls/label.cc:503: // non-focusable labels with selection, which are given focus explicitly in views::Link are never selectable (Link::IsSelectionSupported() overrides to return false), so OnMousePressed will never RequestFocus and Label no longer has focus rings so this should no longer be an issue. https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/style... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/style... ui/views/controls/styled_label.cc:93: result.font_style = gfx::Font::UNDERLINE; Of course.. this will add underlines to some StyledLabel instances that currently don't have underlines -- even before --secondary-ui-md is flipped. I don't think we want that. Plan for this is to add a `bool` (or FontStyle) argument to CreateForLink() since the majority are setting one, and some do really gross things like check for MaterialDesignController::IsSecondaryUiMaterial() whenever calling CreateForLink() -- e.g. https://cs.chromium.org/chromium/src/chrome/browser/ui/views/page_info/page_i... (and line 264). This would blow out the CL diff, so if this sounds good, I'll change the CreateForLink() signature in a precursor CL to this.
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants to a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays since that inspects Label's private RenderText instances. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays in views::Label since it inspects Label's private RenderText instances. BUG=711161 ==========
I agree with all the cl bullet point except maybe this one: - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) It seems like focus rings should always be used for non-MD, regardless of underline state. In MD we'd almost* always not have an underline and in that case use underline to indicate focus. *the one exception I can think of is that when you have an empty bookmark bar and a browser theme, the import bookmark link text color has to match the non-link text color because we don't have any better color to choose. Since the text color is the same the underline is the only thing that indicates linkiness.
On 2017/04/19 12:28:45, tapted wrote: > Hi sky (cc pkasting/estade) > > I was hoping to get some feedback on this approach. I still need to add tests > for the codepaths when --secondary-ui-md is flipped, but I think the code is > correct. I'm wondering about your thoughts about the list of tradeoffs in the CL > description, or if you have ideas for better ways to do this. > > E.g. > - Is it safe to assume that any views::Label that needs a focus ring will be a > views::Link? or (more extreme) I *think* so. I looked at calls to SetFocusBehavior in ash and chrome and didn't see any to Labels, but I didn't investigate all calls. > - How bad would it be to scrap the focus ring everywhere, immediately (_before_ > flipping --secondary-ui-md), and just indicate focus with an underline system > wide? I think we should wait for md before doing that. > > (the CL currently does the first, but not the second). > > Thanks in advance. > > (there's a red linux bot - but I think it's just a font kerning thing) > > https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label.cc > File ui/views/controls/label.cc (left): > > https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label... > ui/views/controls/label.cc:503: // non-focusable labels with selection, which > are given focus explicitly in > views::Link are never selectable (Link::IsSelectionSupported() overrides to > return false), so OnMousePressed will never RequestFocus and Label no longer has > focus rings so this should no longer be an issue. > > https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/style... > File ui/views/controls/styled_label.cc (right): > > https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/style... > ui/views/controls/styled_label.cc:93: result.font_style = gfx::Font::UNDERLINE; > Of course.. this will add underlines to some StyledLabel instances that > currently don't have underlines -- even before --secondary-ui-md is flipped. I > don't think we want that. Plan for this is to add a `bool` (or FontStyle) > argument to CreateForLink() since the majority are setting one, and some do > really gross things like check for > MaterialDesignController::IsSecondaryUiMaterial() whenever calling > CreateForLink() -- e.g. > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/page_info/page_i... > (and line 264). > > This would blow out the CL diff, so if this sounds good, I'll change the > CreateForLink() signature in a precursor CL to this.
https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label... ui/views/controls/label.cc:853: rect.Inset(GetInsets() - View::GetInsets()); This is very subtle and worth a comment. In fact I tend to think it's a bug that View::GetContentsBounds() doesn't call GetInsets(). https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label... ui/views/controls/label.h:229: virtual void MaybePaintFocusRing(gfx::Canvas* canvas) const; We generally don't use 'maybe' for painting calls that might do nothing. How about PaintFocusRing?
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring - If SetUnderline(false) is called, then the consumer wants no focus ring (underline to indicate focus instead) - If neither is called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays in views::Label since it inspects Label's private RenderText instances. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring (even under MD). This happens (sometimes) for the BookmarkBarInstructionsView and nothing else. - If SetUnderline is not called it wants the "default" - Links in StyledLabel never want underlines under MD (this is currently the case). - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. - Under Non-MD only, calling SetUnderline(false) removes the underline but does not change the focus style. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays in views::Label since it inspects Label's private RenderText instances. BUG=711161 ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Patchset #4 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky, PTAL - I think this is ready for review. On 2017/04/19 17:27:39, sky wrote: > On 2017/04/19 12:28:45, tapted wrote: > > E.g. > > - Is it safe to assume that any views::Label that needs a focus ring will be > a > > views::Link? or (more extreme) > > I *think* so. I looked at calls to SetFocusBehavior in ash and chrome and didn't > see any to Labels, but I didn't investigate all calls. Yeah I couldn't find anything either. On 2017/04/19 13:29:56, Evan Stade wrote: > I agree with all the cl bullet point except maybe this one: > > - If SetUnderline(false) is called, then the consumer wants no focus > ring (underline to indicate focus instead) > > It seems like focus rings should always be used for non-MD, regardless of > underline state. In MD we'd almost* always not have an underline and in that > case use underline to indicate focus. I've modified Link::GetFocusStyle() to account for this - it now just returns GetDefaultFocusStyle() unless that's UNDERLINE and the link always has an underline. > > *the one exception I can think of is that when you have an empty bookmark bar > and a browser theme, the import bookmark link text color has to match the > non-link text color because we don't have any better color to choose. Since the > text color is the same the underline is the only thing that indicates linkiness. Yup - confirmed this with my brute-force refactoring. So it looks like there's exactly one case that will still need focus rings after MD :/ https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label... ui/views/controls/label.cc:853: rect.Inset(GetInsets() - View::GetInsets()); On 2017/04/19 17:27:46, sky wrote: > This is very subtle and worth a comment. In fact I tend to think it's a bug that > View::GetContentsBounds() doesn't call GetInsets(). Done. https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/label... ui/views/controls/label.h:229: virtual void MaybePaintFocusRing(gfx::Canvas* canvas) const; On 2017/04/19 17:27:46, sky wrote: > We generally don't use 'maybe' for painting calls that might do nothing. How > about PaintFocusRing? Done. https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/style... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/2810403002/diff/40001/ui/views/controls/style... ui/views/controls/styled_label.cc:93: result.font_style = gfx::Font::UNDERLINE; On 2017/04/19 12:28:45, tapted wrote: > Of course.. this will add underlines to some StyledLabel instances that > currently don't have underlines -- even before --secondary-ui-md is flipped. I > don't think we want that. Plan for this is to add a `bool` (or FontStyle) > argument to CreateForLink() since the majority are setting one, and some do > really gross things like check for > MaterialDesignController::IsSecondaryUiMaterial() whenever calling > CreateForLink() -- e.g. > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/page_info/page_i... > (and line 264). > > This would blow out the CL diff, so if this sounds good, I'll change the > CreateForLink() signature in a precursor CL to this. After my brute-force refactoring in crrev.com/2833563003 it turns out there is exactly one consumer that wants an underlined Link in StyledLabel (page info bubble), and then only under MD. So it's straightforward to revert this part and update CreateLabelRange to check for things unexpectedly adding underlines in MD.
LGTM https://codereview.chromium.org/2810403002/diff/180001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2810403002/diff/180001/ui/views/controls/labe... ui/views/controls/label.cc:45: } // namespace
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring (even under MD). This happens (sometimes) for the BookmarkBarInstructionsView and nothing else. - If SetUnderline is not called it wants the "default" - Links in StyledLabel never want underlines under MD (this is currently the case). - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links. - Under Non-MD only, calling SetUnderline(false) removes the underline but does not change the focus style. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays in views::Label since it inspects Label's private RenderText instances. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring (even under MD). This happens (sometimes) for the BookmarkBarInstructionsView and nothing else. - If SetUnderline is not called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links (this is currently the case). - Under Non-MD only, calling SetUnderline(false) removes the underline but does not change the focus style. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays in views::Label since it inspects Label's private RenderText instances. BUG=711161 ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2810403002/diff/180001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2810403002/diff/180001/ui/views/controls/labe... ui/views/controls/label.cc:45: } On 2017/04/20 17:38:29, sky wrote: > // namespace Done.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2810403002/#ps200001 (title: "Add missing // namespace comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492737566703400,
"parent_rev": "190b7dd412a2f782a2b7c0b671778e1c00a48563", "commit_rev":
"480f457f6939eade256dc987fd740452ef5912de"}
Message was sent while issue was closed.
Description was changed from ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring (even under MD). This happens (sometimes) for the BookmarkBarInstructionsView and nothing else. - If SetUnderline is not called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links (this is currently the case). - Under Non-MD only, calling SetUnderline(false) removes the underline but does not change the focus style. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays in views::Label since it inspects Label's private RenderText instances. BUG=711161 ========== to ========== Views: Don't add insets for views::Link focus rings under MD. Insets for dashed focus rings are throwing off the text alignment on dialogs. Currently they are added to any Link and any StyledLabel that contains a Link. Dashed focus rings have already been phased out from most places in Chrome. On a views::Link, they are still needed when the link is permanently underlined since that's the only way to indicate focus. To contain the insanity, this CL makes assumptions (both with and without MD). - only views::Link ever wants a focus ring - not vanilla views::Labels (effectively, a vanilla Label is never focusable). - If a views::Link consumer calls SetUnderline(true) then it wants a focus ring (even under MD). This happens (sometimes) for the BookmarkBarInstructionsView and nothing else. - If SetUnderline is not called it wants the "default" - Nothing overrides Label::GetInsets to add things that are not focus rings (StyledLabel was already making this assumption). - Under MD only, views::StyledLabel consumers never want underlines on links (this is currently the case). - Under Non-MD only, calling SetUnderline(false) removes the underline but does not change the focus style. This allows a lot of the "focus ring" logic to move down into views::Link where it will be easier to delete later. Calculating the bounds stays in views::Label since it inspects Label's private RenderText instances. BUG=711161 Review-Url: https://codereview.chromium.org/2810403002 Cr-Commit-Position: refs/heads/master@{#466211} Committed: https://chromium.googlesource.com/chromium/src/+/480f457f6939eade256dc987fd74... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/chromium/src/+/480f457f6939eade256dc987fd74...
Message was sent while issue was closed.
On 2017/04/20 11:50:18, tapted wrote: > Yup - confirmed this with my brute-force refactoring. So it looks like there's > exactly one case that will still need focus rings after MD :/ > I wonder if a designer could be convinced to turn that link into a button or something else that's not a link. I think the Show All button on the download shelf was once a link. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
