Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2409563002:
views: use darker stroke for non-prominent buttons (Closed)
|
Created:
4 years, 2 months ago by Elly Fong-Jones Modified:
4 years, 2 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionviews: use darker stroke for non-prominent buttons
This change:
1) Introduces kColorId_{Non,}ProminentButtonBorderColor
2) Uses it for the stroke instead of an alpha blend of the text color
in MdTextButton
BUG=654015
Committed: https://crrev.com/dcfe5a45b59aeb5ccc16a74fc5312aa7a95ac74e
Cr-Commit-Position: refs/heads/master@{#424805}
Patch Set 1 #
Total comments: 1
Patch Set 2 : use magic alpha value #
Total comments: 2
Patch Set 3 : add an explanatory comment #Messages
Total messages: 21 (7 generated)
Description was changed from
==========
views: use darker stroke for non-prominent buttons
This change:
1) Introduces kColorId_{Non,}ProminentButtonBorderColor
2) Uses it for the stroke instead of an alpha blend of the text color
in MdTextButton
BUG=654015
==========
to
==========
views: use darker stroke for non-prominent buttons
This change:
1) Introduces kColorId_{Non,}ProminentButtonBorderColor
2) Uses it for the stroke instead of an alpha blend of the text color
in MdTextButton
BUG=654015
==========
ellyjones@chromium.org changed reviewers: + estade@chromium.org
estade: ptal?
https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... File ui/views/controls/button/md_text_button.cc (right): https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... ui/views/controls/button/md_text_button.cc:274: : ui::NativeTheme::kColorId_NonProminentButtonBorderColor); this doesn't work well for buttons that use custom colors (like the Show All button on the DL shelf). I think we are supposed to be using kPrimaryTextColor for kColorId_ButtonEnabledColor in common_theme.cc. That should fix both the text color and the stroke.
On 2016/10/10 17:13:49, Evan Stade wrote: > https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... > File ui/views/controls/button/md_text_button.cc (right): > > https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... > ui/views/controls/button/md_text_button.cc:274: : > ui::NativeTheme::kColorId_NonProminentButtonBorderColor); > this doesn't work well for buttons that use custom colors (like the Show All > button on the DL shelf). > > I think we are supposed to be using kPrimaryTextColor for > kColorId_ButtonEnabledColor in common_theme.cc. That should fix both the text > color and the stroke. At the moment, the spec has the text color as #5a5a5a and the stroke as #000000 @ 0.2a, so I think we can't use the same color for both. shrike@ is syncing up with UX about it; it's possible these are intended to be the same color.
On 2016/10/11 11:12:04, Elly Fong-Jones wrote: > On 2016/10/10 17:13:49, Evan Stade wrote: > > > https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... > > File ui/views/controls/button/md_text_button.cc (right): > > > > > https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... > > ui/views/controls/button/md_text_button.cc:274: : > > ui::NativeTheme::kColorId_NonProminentButtonBorderColor); > > this doesn't work well for buttons that use custom colors (like the Show All > > button on the DL shelf). > > > > I think we are supposed to be using kPrimaryTextColor for > > kColorId_ButtonEnabledColor in common_theme.cc. That should fix both the text > > color and the stroke. > > At the moment, the spec has the text color as #5a5a5a and the stroke as #000000 > @ 0.2a, so I think we can't use the same color for both. shrike@ is syncing up > with UX about it; it's possible these are intended to be the same color. I don't think they're supposed to be the same color, but the stroke should still be the text color with some special alpha applied. For example, if we wanted 5a5a5a text and 000 * 0.2a stroke, that would be essentially equivalent to a stroke of 5a5a5a * 0.56a. If we want #000 text that certainly makes it easier.
On 2016/10/11 21:17:04, Evan Stade wrote: > On 2016/10/11 11:12:04, Elly Fong-Jones wrote: > > On 2016/10/10 17:13:49, Evan Stade wrote: > > > > > > https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... > > > File ui/views/controls/button/md_text_button.cc (right): > > > > > > > > > https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... > > > ui/views/controls/button/md_text_button.cc:274: : > > > ui::NativeTheme::kColorId_NonProminentButtonBorderColor); > > > this doesn't work well for buttons that use custom colors (like the Show All > > > button on the DL shelf). > > > > > > I think we are supposed to be using kPrimaryTextColor for > > > kColorId_ButtonEnabledColor in common_theme.cc. That should fix both the > text > > > color and the stroke. > > > > At the moment, the spec has the text color as #5a5a5a and the stroke as > #000000 > > @ 0.2a, so I think we can't use the same color for both. shrike@ is syncing up > > with UX about it; it's possible these are intended to be the same color. > > I don't think they're supposed to be the same color, but the stroke should still > be the text color with some special alpha applied. For example, if we wanted > 5a5a5a text and 000 * 0.2a stroke, that would be essentially equivalent to a > stroke of 5a5a5a * 0.56a. If we want #000 text that certainly makes it easier. actually, i think 0.31a rather than 0.56a. But the bottom line is there's some alpha value that can describe the relationship between text and stroke.
On 2016/10/11 21:21:32, Evan Stade (ooo till 10-20) wrote: > On 2016/10/11 21:17:04, Evan Stade wrote: > > On 2016/10/11 11:12:04, Elly Fong-Jones wrote: > > > On 2016/10/10 17:13:49, Evan Stade wrote: > > > > > > > > > > https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... > > > > File ui/views/controls/button/md_text_button.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2409563002/diff/1/ui/views/controls/button/md... > > > > ui/views/controls/button/md_text_button.cc:274: : > > > > ui::NativeTheme::kColorId_NonProminentButtonBorderColor); > > > > this doesn't work well for buttons that use custom colors (like the Show > All > > > > button on the DL shelf). > > > > > > > > I think we are supposed to be using kPrimaryTextColor for > > > > kColorId_ButtonEnabledColor in common_theme.cc. That should fix both the > > text > > > > color and the stroke. > > > > > > At the moment, the spec has the text color as #5a5a5a and the stroke as > > #000000 > > > @ 0.2a, so I think we can't use the same color for both. shrike@ is syncing > up > > > with UX about it; it's possible these are intended to be the same color. > > > > I don't think they're supposed to be the same color, but the stroke should > still > > be the text color with some special alpha applied. For example, if we wanted > > 5a5a5a text and 000 * 0.2a stroke, that would be essentially equivalent to a > > stroke of 5a5a5a * 0.56a. If we want #000 text that certainly makes it easier. > > actually, i think 0.31a rather than 0.56a. But the bottom line is there's some > alpha value that can describe the relationship between text and stroke. 0.31a is close: AlphaBlend(5a5a5a@1, ffffff@1, 0.31) yields cbcbcb@1, but AlphaBlend(000000@1, ffffff@1, 0.2) yields cccccc@1. The ideal value is something close to 0.308, but 8 bits isn't wide enough to represent it exactly. The closest 8-bit alpha value would be 4e, I think, which yields cccccc@1. I'll change this over to use that alpha value, but I'm uneasy about linking these two constants like this; in the spec they're given as separate manifest constants and it doesn't seem like there's any reason they should always be nicely related like this.
estade: ptal? :)
ellyjones@chromium.org changed reviewers: + sky@chromium.org
hm, estade@'s OOO until 20161020. sky@, can you take a look? :)
lgtm > The closest 8-bit alpha value would be 4e, I think, which yields cccccc@1. I'll > change this over to use that alpha value, but I'm uneasy about linking these two > constants like this; in the spec they're given as separate manifest constants > and it doesn't seem like there's any reason they should always be nicely related > like this. The spec sometimes requires some extrapolation. How else do we derive a stroke color for themed buttons? Custom browser themes define a color for text on the toolbar but not a stroke. The only other option I see, which I don't think is particularly attractive, is that we could add a constant in the native theme and use it in the normal case, but instead calculate the stroke if there's a bg color override. That would cover the download shelf case. But then we have to update both the GTK native theme and incognito native theme to specify a stroke color. If we just calculate the stroke from the text, it covers all native themes and custom browser themes. https://codereview.chromium.org/2409563002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/md_text_button.cc (right): https://codereview.chromium.org/2409563002/diff/20001/ui/views/controls/butto... ui/views/controls/button/md_text_button.cc:274: is_prominent_ ? SK_ColorTRANSPARENT : SkColorSetA(text_color, 0x4e); perhaps a note about how this was calculated or what it means? Something like: // The spec specifies black with 0x33 alpha for the default stroke. Here we derive it from the text color: 0x4e with ChromeIconGrey is very close.
LGTM
https://codereview.chromium.org/2409563002/diff/20001/ui/views/controls/butto... File ui/views/controls/button/md_text_button.cc (right): https://codereview.chromium.org/2409563002/diff/20001/ui/views/controls/butto... ui/views/controls/button/md_text_button.cc:274: is_prominent_ ? SK_ColorTRANSPARENT : SkColorSetA(text_color, 0x4e); On 2016/10/12 15:28:35, Evan Stade (ooo till 10-20) wrote: > perhaps a note about how this was calculated or what it means? Something like: > > // The spec specifies black with 0x33 alpha for the default stroke. Here we > derive it from the text color: 0x4e with ChromeIconGrey is very close. Done.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2409563002/#ps40001 (title: "add an explanatory comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
views: use darker stroke for non-prominent buttons
This change:
1) Introduces kColorId_{Non,}ProminentButtonBorderColor
2) Uses it for the stroke instead of an alpha blend of the text color
in MdTextButton
BUG=654015
==========
to
==========
views: use darker stroke for non-prominent buttons
This change:
1) Introduces kColorId_{Non,}ProminentButtonBorderColor
2) Uses it for the stroke instead of an alpha blend of the text color
in MdTextButton
BUG=654015
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
views: use darker stroke for non-prominent buttons
This change:
1) Introduces kColorId_{Non,}ProminentButtonBorderColor
2) Uses it for the stroke instead of an alpha blend of the text color
in MdTextButton
BUG=654015
==========
to
==========
views: use darker stroke for non-prominent buttons
This change:
1) Introduces kColorId_{Non,}ProminentButtonBorderColor
2) Uses it for the stroke instead of an alpha blend of the text color
in MdTextButton
BUG=654015
Committed: https://crrev.com/dcfe5a45b59aeb5ccc16a74fc5312aa7a95ac74e
Cr-Commit-Position: refs/heads/master@{#424805}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dcfe5a45b59aeb5ccc16a74fc5312aa7a95ac74e Cr-Commit-Position: refs/heads/master@{#424805} |
