Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17756003:
Colors in views::StyledLabel. (Closed)
|
Created:
7 years, 6 months ago by fdoray Modified:
7 years, 5 months ago Reviewers:
bcwhite, noms, Roger Tawa OOO till Jul 10th, Evan Stade, Peter Kasting, achuithb, Ben Goodger (Google) CC:
chromium-reviews, tfarina, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, sky Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionColors in views::StyledLabel.
Add the possibility to specify a custom foreground color when defining the style of a text range in a views::StyledLabel. Also support auto color readability in views::StyledLabel, to automatically adjust the foreground colors when they are not readable over the background color.
Necessary to comply with the mock of the bookmark sync promo (crbug.com/244279).
TEST=No user visible change introduced by this CL. Custom colors will be used in the bookmark sync promo (crbug.com/244279).
BUG=244630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211471
Patch Set 1 #
Total comments: 6
Patch Set 2 : Color readability + Tests #Patch Set 3 : Static function to get the default color of links #Patch Set 4 : Style fix #
Total comments: 2
Patch Set 5 : Colors from SkColor.h #
Total comments: 6
Patch Set 6 : Remove unused method SetAutoColorReadabilityEnabled #Patch Set 7 : Rename SetBackgroundColor -> SetDisplayedOnBackgroundColor #
Total comments: 2
Patch Set 8 : Static variable in GetDefaultEnabledColor #Patch Set 9 : No static variables #Patch Set 10 : Rebase #
Messages
Total messages: 43 (0 generated)
Could you review this CL? Thanks.
https://codereview.chromium.org/17756003/diff/1/chrome/browser/chromeos/ui/ec... File chrome/browser/chromeos/ui/echo_dialog_view.cc (right): https://codereview.chromium.org/17756003/diff/1/chrome/browser/chromeos/ui/ec... chrome/browser/chromeos/ui/echo_dialog_view.cc:62: views::StyledLabel::RangeStyleInfo::CreateForLink(); This changes |disable_line_wrapping| from false to true. Is that OK? https://codereview.chromium.org/17756003/diff/1/ui/views/controls/styled_labe... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/17756003/diff/1/ui/views/controls/styled_labe... ui/views/controls/styled_label.cc:80: result.color = SkColorSetRGB(0, 51, 153); When will this get fixed? When you finish the GTK version of the bookmark sync promo?
Answered Roger's comments. https://codereview.chromium.org/17756003/diff/1/chrome/browser/chromeos/ui/ec... File chrome/browser/chromeos/ui/echo_dialog_view.cc (right): https://codereview.chromium.org/17756003/diff/1/chrome/browser/chromeos/ui/ec... chrome/browser/chromeos/ui/echo_dialog_view.cc:62: views::StyledLabel::RangeStyleInfo::CreateForLink(); On 2013/06/26 15:16:29, Roger Tawa wrote: > This changes |disable_line_wrapping| from false to true. Is that OK? I think it's OK. We don't want a line wrap in the middle of a "Learn More" link. https://codereview.chromium.org/17756003/diff/1/ui/views/controls/styled_labe... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/17756003/diff/1/ui/views/controls/styled_labe... ui/views/controls/styled_label.cc:80: result.color = SkColorSetRGB(0, 51, 153); This is how the default link color is obtained for views::Link (link.cc (177)). I didn't want to make this color available from theme provider since it's already a TODO for beng. Should I write TODO(beng) here? Do you think there is a better way to do this (for example, adding a public static function in views::Link to avoid duplicating the code to get the color)? On 2013/06/26 15:16:29, Roger Tawa wrote: > When will this get fixed? When you finish the GTK version of the bookmark sync > promo?
https://codereview.chromium.org/17756003/diff/1/ui/views/controls/styled_labe... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/17756003/diff/1/ui/views/controls/styled_labe... ui/views/controls/styled_label.cc:80: result.color = SkColorSetRGB(0, 51, 153); On 2013/06/26 17:06:38, fdoray wrote: > This is how the default link color is obtained for views::Link (link.cc (177)). > I didn't want to make this color available from theme provider since it's > already a TODO for beng. Should I write TODO(beng) here? Do you think there is a > better way to do this (for example, adding a public static function in > views::Link to avoid duplicating the code to get the color)? > > On 2013/06/26 15:16:29, Roger Tawa wrote: > > When will this get fixed? When you finish the GTK version of the bookmark > sync > > promo? > Might want to ask Ben what his plans are. Adding a static method in views::Link to avoid duplication sounds good to me in the mean time.
On 2013/06/27 13:34:32, Roger Tawa wrote: > Might want to ask Ben what his plans are. I can't see Ben having an answer to this. Those TODOs are at least five years old. I would probably stick the TODO in here and be done with it. Incidentally, there's an outstanding bug on color support for StyledLabel: http://crbug.com/244630 . You might want to search the codebase for references to that, since I think there's at least two places that want color support already checked in.
On 2013/06/27 17:52:18, Peter Kasting wrote: > On 2013/06/27 13:34:32, Roger Tawa wrote: > > Might want to ask Ben what his plans are. > > I can't see Ben having an answer to this. Those TODOs are at least five years > old. > > I would probably stick the TODO in here and be done with it. > > Incidentally, there's an outstanding bug on color support for StyledLabel: > http://crbug.com/244630 . You might want to search the codebase for references > to that, since I think there's at least two places that want color support > already checked in. Peter is correct.
This new version fixes bug 244630. I realized that it could be useful for the bookmark sync promo to have the auto color readability feature since text will be drawn on a grey background.
Style fix
lgtm https://codereview.chromium.org/17756003/diff/17001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/17756003/diff/17001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:16: const SkColor kBlack = SkColorSetRGB(0, 0, 0); The "SkColor.h" file has a number of standard color definitions you can use rather than defining new constants.
lgtm
+achuith@chromium.org owner for echo_dialog_view.cc +estade@chromium.org owner for autofill_dialog_views.cc
lgtm https://codereview.chromium.org/17756003/diff/1/chrome/browser/chromeos/ui/ec... File chrome/browser/chromeos/ui/echo_dialog_view.cc (right): https://codereview.chromium.org/17756003/diff/1/chrome/browser/chromeos/ui/ec... chrome/browser/chromeos/ui/echo_dialog_view.cc:62: views::StyledLabel::RangeStyleInfo::CreateForLink(); On 2013/06/26 17:06:38, fdoray wrote: > On 2013/06/26 15:16:29, Roger Tawa wrote: > > This changes |disable_line_wrapping| from false to true. Is that OK? > > I think it's OK. We don't want a line wrap in the middle of a "Learn More" link. OK. This seems more generic than just the "learn more" link so that's why I was asking.
https://codereview.chromium.org/17756003/diff/17001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/17756003/diff/17001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:16: const SkColor kBlack = SkColorSetRGB(0, 0, 0); On 2013/07/02 14:22:13, bcwhite wrote: > The "SkColor.h" file has a number of standard color definitions you can use > rather than defining new constants. Done.
lgtm for chromeos/ui
https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.h:71: void SetAutoColorReadabilityEnabled(bool enabled); is this used somewhere?
You have Ben as a reviewer on this one. Do you really need both of us?
On 2013/07/08 20:41:47, sky wrote: > You have Ben as a reviewer on this one. Do you really need both of us? -sky@chromium.org I need either Ben or Scott. If Ben can review this soon, it's good!
https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.h:75: void SetBackgroundColor(SkColor color); This name implies the background will be drawn though... so I'd recommend coming up with a different name.
I answered the comments from Ben and Evan. https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.h:71: void SetAutoColorReadabilityEnabled(bool enabled); On 2013/07/08 18:14:04, Evan Stade wrote: > is this used somewhere? No. But a method with the same name in views::Label is used. I provided this method so that both controls have a similar interface. The bug crbug.com/244630 suggests this. Do you prefer that I remove this unused method? Auto-color-readability would always be enabled for views::StyledLabel. https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.h:75: void SetBackgroundColor(SkColor color); On 2013/07/09 17:36:15, Ben Goodger (Google) wrote: > This name implies the background will be drawn though... so I'd recommend coming > up with a different name. I chose this name because it's used in views::Label, but it's true that it's confusing. What about SetAutoReadabilityBackgroundColor?
https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.h:71: void SetAutoColorReadabilityEnabled(bool enabled); On 2013/07/09 18:13:47, fdoray wrote: > On 2013/07/08 18:14:04, Evan Stade wrote: > > is this used somewhere? > > No. But a method with the same name in views::Label is used. I provided this > method so that both controls have a similar interface. The bug crbug.com/244630 > suggests this. > > Do you prefer that I remove this unused method? Auto-color-readability would > always be enabled for views::StyledLabel. yes, remove dead code.
SetTextContrastColor? -Ben On Tue, Jul 9, 2013 at 11:13 AM, <fdoray@chromium.org> wrote: > I answered the comments from Ben and Evan. > > > > https://codereview.chromium.**org/17756003/diff/26001/ui/** > views/controls/styled_label.h<https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_label.h> > File ui/views/controls/styled_**label.h (right): > > https://codereview.chromium.**org/17756003/diff/26001/ui/** > views/controls/styled_label.h#**newcode71<https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_label.h#newcode71> > ui/views/controls/styled_**label.h:71: void > SetAutoColorReadabilityEnabled**(bool enabled); > On 2013/07/08 18:14:04, Evan Stade wrote: > >> is this used somewhere? >> > > No. But a method with the same name in views::Label is used. I provided > this method so that both controls have a similar interface. The bug > crbug.com/244630 suggests this. > > Do you prefer that I remove this unused method? Auto-color-readability > would always be enabled for views::StyledLabel. > > > https://codereview.chromium.**org/17756003/diff/26001/ui/** > views/controls/styled_label.h#**newcode75<https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_label.h#newcode75> > ui/views/controls/styled_**label.h:75: void SetBackgroundColor(SkColor > color); > On 2013/07/09 17:36:15, Ben Goodger (Google) wrote: > >> This name implies the background will be drawn though... so I'd >> > recommend coming > >> up with a different name. >> > > I chose this name because it's used in views::Label, but it's true that > it's confusing. What about SetAutoReadabilityBackgroundCo**lor? > > https://codereview.chromium.**org/17756003/<https://codereview.chromium.org/1... >
On 2013/07/09 19:34:58, Ben Goodger (Google) wrote: > SetTextContrastColor? I don't like this because it can sound as if you're specifying the alternate text color that auto-readability might switch to.
On 2013/07/09 19:40:41, Peter Kasting wrote: > On 2013/07/09 19:34:58, Ben Goodger (Google) wrote: > > SetTextContrastColor? > > I don't like this because it can sound as if you're specifying the alternate > text color that auto-readability might switch to. Which one is best? - SetBackgroundColorForAutoReadability - SetAutoReadabilityBackgroundColor - SetTextContrastColor - SetBackgroundContrastColor - other suggestions?
I removed SetAutoColorReadabilityEnabled as requested by estade@. https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/17756003/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.h:71: void SetAutoColorReadabilityEnabled(bool enabled); On 2013/07/09 18:35:18, Evan Stade wrote: > On 2013/07/09 18:13:47, fdoray wrote: > > On 2013/07/08 18:14:04, Evan Stade wrote: > > > is this used somewhere? > > > > No. But a method with the same name in views::Label is used. I provided this > > method so that both controls have a similar interface. The bug > crbug.com/244630 > > suggests this. > > > > Do you prefer that I remove this unused method? Auto-color-readability would > > always be enabled for views::StyledLabel. > > yes, remove dead code. Done.
On 2013/07/09 20:04:40, fdoray wrote: > On 2013/07/09 19:40:41, Peter Kasting wrote: > > On 2013/07/09 19:34:58, Ben Goodger (Google) wrote: > > > SetTextContrastColor? > > > > I don't like this because it can sound as if you're specifying the alternate > > text color that auto-readability might switch to. > > Which one is best? > - SetBackgroundColorForAutoReadability > - SetAutoReadabilityBackgroundColor > - SetTextContrastColor > - SetBackgroundContrastColor > - other suggestions? Honestly, nothing seems to be a big enough improvement over the original that I'd bother changing it. I see Ben's point, but I can't think of a clear improvement, and besides, we kind of _want_ people to see/think of this function if they're trying to set background colors, because we want to ensure they always call it in that case.
On 2013/07/09 20:58:00, Peter Kasting wrote: > On 2013/07/09 20:04:40, fdoray wrote: > > On 2013/07/09 19:40:41, Peter Kasting wrote: > > > On 2013/07/09 19:34:58, Ben Goodger (Google) wrote: > > > > SetTextContrastColor? > > > > > > I don't like this because it can sound as if you're specifying the alternate > > > text color that auto-readability might switch to. > > > > Which one is best? > > - SetBackgroundColorForAutoReadability > > - SetAutoReadabilityBackgroundColor > > - SetTextContrastColor > > - SetBackgroundContrastColor > > - other suggestions? > > Honestly, nothing seems to be a big enough improvement over the original that > I'd bother changing it. I see Ben's point, but I can't think of a clear > improvement, and besides, we kind of _want_ people to see/think of this function > if they're trying to set background colors, because we want to ensure they > always call it in that case. So we keep SetBackgroundColor?
I agree with Ben that it's very confusing. How about SetHasBackgroundColor?
On 2013/07/10 23:50:13, Evan Stade wrote: > I agree with Ben that it's very confusing. How about SetHasBackgroundColor? Wouldn't you expect that to take a bool?
On 2013/07/10 23:53:57, Peter Kasting wrote: > On 2013/07/10 23:50:13, Evan Stade wrote: > > I agree with Ben that it's very confusing. How about SetHasBackgroundColor? > > Wouldn't you expect that to take a bool? SetBackgroundColor(BLUE) is confusing SetHasBackgroundColor(BLUE) is less confusing
On 2013/07/11 00:08:41, Evan Stade wrote: > On 2013/07/10 23:53:57, Peter Kasting wrote: > > On 2013/07/10 23:50:13, Evan Stade wrote: > > > I agree with Ben that it's very confusing. How about SetHasBackgroundColor? > > > > Wouldn't you expect that to take a bool? > > SetBackgroundColor(BLUE) is confusing > SetHasBackgroundColor(BLUE) is less confusing 1 SetBackgroundColor 2 SetHasBackgroundColor 3 SetBackgroundColorForAutoReadability 4 SetAutoReadabilityBackgroundColor 5 SetTextContrastColor 6 SetBackgroundContrastColor 7 SetDisplayedOnBackgroundColor I like 1, 3, 5 and 7. Which one do you prefer?
On 2013/07/11 13:25:04, fdoray wrote: > On 2013/07/11 00:08:41, Evan Stade wrote: > > On 2013/07/10 23:53:57, Peter Kasting wrote: > > > On 2013/07/10 23:50:13, Evan Stade wrote: > > > > I agree with Ben that it's very confusing. How about > SetHasBackgroundColor? > > > > > > Wouldn't you expect that to take a bool? > > > > SetBackgroundColor(BLUE) is confusing > > SetHasBackgroundColor(BLUE) is less confusing > > 1 SetBackgroundColor > 2 SetHasBackgroundColor > 3 SetBackgroundColorForAutoReadability > 4 SetAutoReadabilityBackgroundColor > 5 SetTextContrastColor > 6 SetBackgroundContrastColor > 7 SetDisplayedOnBackgroundColor > > I like 1, 3, 5 and 7. Which one do you prefer? 7
+1 On Thu, Jul 11, 2013 at 9:26 AM, <estade@chromium.org> wrote: > On 2013/07/11 13:25:04, fdoray wrote: > >> On 2013/07/11 00:08:41, Evan Stade wrote: >> > On 2013/07/10 23:53:57, Peter Kasting wrote: >> > > On 2013/07/10 23:50:13, Evan Stade wrote: >> > > > I agree with Ben that it's very confusing. How about >> SetHasBackgroundColor? >> > > >> > > Wouldn't you expect that to take a bool? >> > >> > SetBackgroundColor(BLUE) is confusing >> > SetHasBackgroundColor(BLUE) is less confusing >> > > 1 SetBackgroundColor >> 2 SetHasBackgroundColor >> 3 SetBackgroundColorForAutoReada**bility >> 4 SetAutoReadabilityBackgroundCo**lor >> 5 SetTextContrastColor >> 6 SetBackgroundContrastColor >> 7 SetDisplayedOnBackgroundColor >> > > I like 1, 3, 5 and 7. Which one do you prefer? >> > > 7 > > https://codereview.chromium.**org/17756003/<https://codereview.chromium.org/1... >
I changed the method name to SetDisplayedOnBackgroundColor. The CL can be reviewed again. On 2013/07/11 16:44:22, Ben Goodger (Google) wrote: > +1 > > > On Thu, Jul 11, 2013 at 9:26 AM, <mailto:estade@chromium.org> wrote: > > > On 2013/07/11 13:25:04, fdoray wrote: > > > >> On 2013/07/11 00:08:41, Evan Stade wrote: > >> > On 2013/07/10 23:53:57, Peter Kasting wrote: > >> > > On 2013/07/10 23:50:13, Evan Stade wrote: > >> > > > I agree with Ben that it's very confusing. How about > >> SetHasBackgroundColor? > >> > > > >> > > Wouldn't you expect that to take a bool? > >> > > >> > SetBackgroundColor(BLUE) is confusing > >> > SetHasBackgroundColor(BLUE) is less confusing > >> > > > > 1 SetBackgroundColor > >> 2 SetHasBackgroundColor > >> 3 SetBackgroundColorForAutoReada**bility > >> 4 SetAutoReadabilityBackgroundCo**lor > >> 5 SetTextContrastColor > >> 6 SetBackgroundContrastColor > >> 7 SetDisplayedOnBackgroundColor > >> > > > > I like 1, 3, 5 and 7. Which one do you prefer? > >> > > > > 7 > > > > > https://codereview.chromium.**org/17756003/%3Chttps://codereview.chromium.org...> > >
https://codereview.chromium.org/17756003/diff/57001/ui/views/controls/link.cc File ui/views/controls/link.cc (right): https://codereview.chromium.org/17756003/diff/57001/ui/views/controls/link.cc... ui/views/controls/link.cc:39: return color_utils::GetSysSkColor(COLOR_HOTLIGHT); I dont' know if this is an expensive operation, but previously it was optimized to only be called once. Now it may be called more than once.
I uploaded the change suggested by Evan. https://codereview.chromium.org/17756003/diff/57001/ui/views/controls/link.cc File ui/views/controls/link.cc (right): https://codereview.chromium.org/17756003/diff/57001/ui/views/controls/link.cc... ui/views/controls/link.cc:39: return color_utils::GetSysSkColor(COLOR_HOTLIGHT); It makes a call to a Windows function (GetSysColor). I now use a static variable to make sure it's called only once, like before. However, I'm not sure why a static variable was used. http://src.chromium.org/viewvc/chrome/trunk/src/views/controls/link.cc?r1=271... shows that a static variable was used even we no call to a Windows function was made. The variable only stored the result of SkColorSetRGB(...). On 2013/07/11 17:54:39, Evan Stade wrote: > I dont' know if this is an expensive operation, but previously it was optimized > to only be called once. Now it may be called more than once.
lgtm
It's actually wrong to use a static variable to store the result of GetSys[Sk]Color() result. This leads to bugs when the user changes themes. Don't do it. GetSysColor() is cheap anyway.
I've removed the static variables to store colors. Is it better this way?
LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fdoray@chromium.org/17756003/85001
Message was sent while issue was closed.
Change committed as 211471 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
