Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2957263002:
Update dialog button text color for Harmony per the latest button spec (Closed)
|
Created:
3 years, 5 months ago by tapted Modified:
3 years, 5 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate dialog button text color for Harmony per the latest button spec
Only use the new colors when using HarmonyTypographyProvider.
Diverge from NativeTheme for this since there's too much indirection.
`kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor
is used for lots of other things.
Note we still use NativeTheme when the Harmony spec is ignored (e.g. OS
themes with inverted or high-constrast themes). This CL refactors the
code in HarmonyTypographyProvider to be more explicit about when the
Harmony spec needs to be ignored.
BUG=691891, 738292
Review-Url: https://codereview.chromium.org/2957263002
Cr-Commit-Position: refs/heads/master@{#483954}
Committed: https://chromium.googlesource.com/chromium/src/+/d007b3e7c64c2fdaf91c70ed2bc89205fcdfcb41
Patch Set 1 #Patch Set 2 : stray edit #Patch Set 3 : corner case? #Patch Set 4 : Explicitly handle cases where Harmony is ignored #Patch Set 5 : fix default buttons #
Total comments: 10
Patch Set 6 : color_utils::IsInvertedColorScheme(), fewer Win constants #
Messages
Total messages: 43 (25 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 ========== Update button text color for Harmony cl format First pass? BUG=691891 ========== to ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. BUG=691891 ==========
tapted@chromium.org changed reviewers: + pkasting@chromium.org
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...
Hi Peter, please take a look. Eventually... we will have only MdTextButton + Button::STYLE_TEXTBUTTON and things will be simpler, but I couldn't find any threads to pull right now - they hinge on Button::STYLE_BUTTON being killed off.
I think the right thing to do is to go through the theme provider, but to split the colors of pushbuttons off from radio/checkboxes instead of having them linked as they are today. I don't think we should have a theme provider that provides a "button enabled/disabled color" but then have buttons that don't pay attention to it.
On 2017/06/28 07:25:04, Peter Kasting wrote: > I think the right thing to do is to go through the theme provider, but to split > the colors of pushbuttons off from radio/checkboxes instead of having them > linked as they are today. > > I don't think we should have a theme provider that provides a "button > enabled/disabled color" but then have buttons that don't pay attention to it. But is the button enabled/disabled color that the theme provider should provide for bookmarks toolbar buttons or for dialog buttons? Harmony wants them to be different, as I understand it. So unless I've misinterpreted we would need new NativeTheme::ColorId constants, which no theme provider currently provides anyway.
On 2017/06/28 07:28:07, tapted wrote:
> On 2017/06/28 07:25:04, Peter Kasting wrote:
> > I think the right thing to do is to go through the theme provider, but to
> split
> > the colors of pushbuttons off from radio/checkboxes instead of having them
> > linked as they are today.
> >
> > I don't think we should have a theme provider that provides a "button
> > enabled/disabled color" but then have buttons that don't pay attention to
it.
>
> But is the button enabled/disabled color that the theme provider should
provide
> for bookmarks toolbar buttons or for dialog buttons? Harmony wants them to be
> different, as I understand it. So unless I've misinterpreted we would need new
> NativeTheme::ColorId constants, which no theme provider currently provides
> anyway.
Ah, actually bookmarks bar buttons use kColorId_LabelEnabledColor not
kColorId_ButtonEnabledColor, so that's not a good example.
I guess I'm trying to be a bit more explicit about where themes kick in, since I
get lost every time I need to change a color.
E.g. the current list
kColorId_LabelEnabledColor,
kColorId_LabelDisabledColor,
kColorId_ButtonEnabledColor,
kColorId_ButtonDisabledColor,
kColorId_ButtonHoverColor,
kColorId_ButtonPressedShade,
kColorId_BlueButtonEnabledColor,
kColorId_BlueButtonDisabledColor,
kColorId_BlueButtonPressedColor,
kColorId_BlueButtonHoverColor,
kColorId_BlueButtonShadowColor,
kColorId_ProminentButtonColor,
kColorId_TextOnProminentButtonColor,
I guess the current struggle is that we don't have a HarmonyNativeTheme, just
Light and Dark themes. and everything is getting tangled up. I can't map
kColorId_ButtonEnabledColor to the new colours without yet more constants,
feeding more knowledge about --secondary-ui-md into NativeTheme, or changing
colours elsewhere (i.e. without --secondary-ui-md). I hate NativeTheme :|. It
makes simple spec updates into a giant yak shave every time.
</rant>
Anyway.. I guess I'll poke at this some more tomorrow. Really I'd rather
NativeTheme to become less complex -- moving appropriate things into the
TypographyProvider -- rather than make it more complex.
I think you understand this already, but so I'm sure we're on the same page as each other, the reason for all the theme system complexity is that we'd like to have a fallback: Custom theme colors come first Then system theme colors, at least if they've been explicitly specified (and maybe not; I'm always pressing designers on this front, since Chrome is never respectful enough of the OS colors) Then Chrome default colors This also gets crossed up with the fact that in incognito, a lot of the bottom layer, and some of the top, can change. So the system is complex, but that's because it's reflecting behavioral complexity. If we move things out to the typography provider, for example, we basically nuke this whole stack and just force a hardcoded color. I think we still want a "light" and "dark" default theme, but we want the theme provider itself to detect if harmony is on and do slightly different things. I don't think we want a "harmony light" and "harmony dark" theme because most colors shouldn't change in harmony. Also, really, we should be able to dynamically compute a lot of "light" and "dark" things without actually having true separate base themes for them. Some of the MD implementation was a bit lazy in this regard, but we could do better. Anyway, yes, both "change colors elsewhere" and "plumb more Harmony into the theme system" seem reasonable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Yep, we are on the same page. I toyed around with OS themes some more. It was actually a lot easier to get an inverted color scheme on Windows than I thought it was :/. Although I really struggled on Linux, where I thought it would be easy (gtk-chtheme offered one theme -- xfce-dusk -- but it was unusable in GTK apps, and I could only alter text on menus when using gtk-theme-config) Chrome Stable looks kinda rubbish on Windows with inverted/high-contrast colors, but I'll be testing that out more now I know how to do it. We seem to use button colors in a lot of places we should be using text colors, and patches of blue and black bleed into bits of the UI they shouldn't. Anyway, I _was_ concerned by the added complexity we have to support users whose text color isn't black. After experimenting, I'm now less bothered by that -- I agree we need good support for it, but it would be nice to have some UMA on it. It does still bother me that we make a lot of arbitrary decisions about how to map color IDs to concepts in OS themes. I think NativeTheme should only offer a few constants that have clear mappings, and some kind of `ColorProvider` should provide mappings to those constants for given contexts/styles, and know when it needs to ignore the Harmony spec. HarmonyTypographyProvider is headed a bit in that direction already, but just for text. Anyway, those yaks aren't going to shave themselves. And they don't all disappear if we get rid of NativeTheme. So I put them in a line and got out my shears: - https://codereview.chromium.org/2964453002/ - https://codereview.chromium.org/2963083003/ - https://codereview.chromium.org/2960393002/ - https://codereview.chromium.org/2961223002/
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 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 ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. BUG=691891 ========== to ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. BUG=691891, 738292 ==========
Description was changed from ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. BUG=691891, 738292 ========== to ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. Note we still use NativeTheme when the Harmony is ignored (e.g. OS themes with inverted or high-constrast themes). This CL refactors the code in HarmonyTypographyProvider to be more explicit about that. BUG=691891, 738292 ==========
PTAL - this is the CL where I had a bit of a rant. Patchset 5 is basically the same as what I published out in patchset 3, but it's now more explicit about the cases where we ignore the harmony spec. The original CL would still consult the NativeTheme, but I think it wasn't quite clear enough about when that would happen. So I've fixed that. You said earlier that we should go through NativeTheme, but I don't think that's going to work. We can't use kColorId_ButtonEnabledColor both for NativeTheme and for Harmony. Harmony's kColorId_ButtonEnabledColor is gray and bad for high contrast themes. But NativeThemeWin should *always* provide the kColorId_ButtonEnabledColor that a user requested in Control Panel. And -- even for Harmony buttons -- that's the color we should be using if the user has a high contrast theme. It's only when the user has a "default" Windows theme where we should start tinkering with button colors. I've linked up http://crbug.com/738292 which has a bunch of screengrabs. There's a ton of comments in the code with more rationale.. The other places using kColorId_ButtonEnabledColor when they probably shouldn't are still "bugs" but this CL is no longer blocked on those since I don't think we can use kColorId_ButtonEnabledColor for a Harmony color.
My handwavey answer to this is that at the lowest level, NativeThemeWin should indeed always provide the OS colors, but very few people should be calling NativeThemeWin. I would think the ThemeProvider is the place to expose things like the control colors, and it could do logic like "if (we want to override the user's defaults in some way) return kHardcodedThing; else return native_theme->color;". Then this becomes the only real place that calls NativeTheme, except if we have code (like skare is trying to add for inactive user toasts right now) that wants to "always look native". Then in turn other people ask the theme provider what to do, including the typography provider, whose job is now not "implement harmony-specific colors" but more "convert from a context and style into the correct theme color enum to ask the theme provider for". Ignoring the question of how we get there from here, do you think that would be a sane world to end up in? Or am I wrong to want this "decide how to override the system" logic to like in the theme provider? If that would make sense as a long-term world, then what's the path towards that that's actually doable now, since I suspect we don't want to block Harmony phase 1 on "rewrite theme layering"? Perhaps a change like this at least doesn't take us further away, since it moves away from using the native colors directly, which we'd need to do regardless? It doesn't seem like it gets us much closer, but I'm not sure I see a whole lot of things that do, that give us meaningful win immediately.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/30 06:12:01, Peter Kasting wrote: > My handwavey answer to this is that at the lowest level, NativeThemeWin should > indeed always provide the OS colors, but very few people should be calling > NativeThemeWin. > > I would think the ThemeProvider is the place to expose things like the control > colors, and it could do logic like "if (we want to override the user's defaults > in some way) return kHardcodedThing; else return native_theme->color;". Then > this becomes the only real place that calls NativeTheme, except if we have code > (like skare is trying to add for inactive user toasts right now) that wants to > "always look native". > > Then in turn other people ask the theme provider what to do, including the > typography provider, whose job is now not "implement harmony-specific colors" > but more "convert from a context and style into the correct theme color enum to > ask the theme provider for". > > Ignoring the question of how we get there from here, do you think that would be > a sane world to end up in? Or am I wrong to want this "decide how to override > the system" logic to like in the theme provider? Oh boy - you actually meant ThemeService. ThemeProperties is missing a ton of IDs we'd need, and apart from a weird thing on Mac, BrowserThemeProvider is the only ThemeProvider, and there's no precedent for it being used for secondary UI. I haven't toyed with Chrome themes much, so I dove into the code and looked around. I think there's too much that ThemeService/ThemeProivder has which we don't want for secondary UI - trying to shoehorn it will add complexity to secondary UI where it's not needed. ThemeService is also missing a lot. So I think we'd be making both systems more complex.The question should be "what extra use cases are satisfied by the more complex solution". I guess the main one would be that secondary UI could match the browser theme. I don't know if we want that from a product perspective.. From an engineering perspective, it's already hard to make decisions how to map secondary UI concepts to native themes - doing that for Chrome themes as well frightens me. I think ThemeService querying the typography provider -- for text -- when it needs a default is the right layering, if we want to start combining the two. > If that would make sense as a long-term world, then what's the path towards that > that's actually doable now, since I suspect we don't want to block Harmony phase > 1 on "rewrite theme layering"? Perhaps a change like this at least doesn't take > us further away, since it moves away from using the native colors directly, > which we'd need to do regardless? It doesn't seem like it gets us much closer, > but I'm not sure I see a whole lot of things that do, that give us meaningful > win immediately. If we want to support new use cases, I think it would be straightforward to add a `ThemedTypographyProvider` which knows how to merge concepts from native themes, the harmony spec, and a Chrome theme. But I think it would be best to isolate that complexity so that it only comes into play when Chrome is running with a theme. That is, we should always have a provider which is completely unaware of Chrome themes. I think the way ThemeService/ThemeProvider/ThemeSupplier is constructed, that's not possible -- everything seems twisted up in the extension system. How would that actually look? Well, Views only knows about ui::NativeTheme right now -- a Widget can already inherit a ui::NativeTheme from the browser that created it. We could attach a ui::ThemeProvider* to it which can also be consulted by the ThemedTypographyProvider when it's asked to provide a text color. ThemedTypographyProvider can then map either to a ThemeProperties enum value or a NativeTheme enum depending on what's available and what it was asked for.
On 2017/06/30 07:12:51, tapted wrote: > On 2017/06/30 06:12:01, Peter Kasting wrote: > > Ignoring the question of how we get there from here, do you think that would > be > > a sane world to end up in? Or am I wrong to want this "decide how to override > > the system" logic to like in the theme provider? > > Oh boy - you actually meant ThemeService. ThemeProperties is missing a ton of > IDs we'd need, and apart from a weird thing on Mac, BrowserThemeProvider is the > only ThemeProvider, and there's no precedent for it being used for secondary UI. I'm pretty sure I meant ThemeProvider; ThemeService is mainly the way you get at the ThemeProvider, and ThemeProvider actually implements things like GetColor(). > I think there's too much that ThemeService/ThemeProivder has which we don't want > for secondary UI - trying to shoehorn it will add complexity to secondary UI > where it's not needed. I'm confused what this means. Calling GetColor() on the theme provider isn't hard. What sort of complexity were you envisioning this introducing? > ThemeService is also missing a lot. So I think we'd be > making both systems more complex. We'd definitely need to add more color IDs, and the code to understand when to return "built in" colors versus "underlying system" colors. That seems like a sizeable, but relatively isolated, chunk of work. > The question should be "what extra use cases > are satisfied by the more complex solution". The main thing is code clarity. Every place in the code that wants a color goes to the theme provider. The theme provider becomes the single place for all Chrome default colors. The underlying native theme becomes the place that returns all system colors. No one else has any colors. Right now we have colors in a lot of places and people are accessing various levels directly. This makes it hard to fix something like the bug you filed earlier on how we get all kinds of things wrong in high contrast. > I guess the main one would be that > secondary UI could match the browser theme. That would be a nice added bonus. It's not my main motivation. I do think we want that from a product perspective though. If you install a dark theme but all your menus are still white, that feels half-baked. > From an engineering perspective, it's already hard to make > decisions how to map secondary UI concepts to native themes - doing that for > Chrome themes as well frightens me. My goal is to actually make this much easier. The various providers and controls don't need any custom logic or thought anymore. This becomes entirely the job of the ThemeProvider. > I think ThemeService querying the typography provider -- for text -- when it > needs a default is the right layering, if we want to start combining the two. I don't think this is right. That would mean no one can query the typography provider for colors directly, because the theme provider can theoretically override it, and we need to be consistent. Or if we ban the theme provider from overriding the typography system, it means the typography provider has to deal with logic like "do we want to override the system colors", which ultimate we have to add to the theme provider anyway, so we get duplicated complicated code. > > If that would make sense as a long-term world, then what's the path towards > that > > that's actually doable now, since I suspect we don't want to block Harmony > phase > > 1 on "rewrite theme layering"? Perhaps a change like this at least doesn't > take > > us further away, since it moves away from using the native colors directly, > > which we'd need to do regardless? It doesn't seem like it gets us much > closer, > > but I'm not sure I see a whole lot of things that do, that give us meaningful > > win immediately. > > If we want to support new use cases, I think it would be straightforward to add > a `ThemedTypographyProvider` which knows how to merge concepts from native > themes, the harmony spec, and a Chrome theme. But I think it would be best to > isolate that complexity so that it only comes into play when Chrome is running > with a theme. That is, we should always have a provider which is completely > unaware of Chrome themes. I think the way > ThemeService/ThemeProvider/ThemeSupplier is constructed, that's not possible -- > everything seems twisted up in the extension system. To me this answer is a reflection of the fact that making TypographyProvider understand anything at all about colors, let alone the complexity of how native/default/themed colors interact, is an inverted layering. Your answer is making me feel more confident of my answer rather than less. That was not your intent, I think :)
On 2017/06/30 07:31:20, Peter Kasting wrote: > On 2017/06/30 07:12:51, tapted wrote: > > On 2017/06/30 06:12:01, Peter Kasting wrote: > > > Ignoring the question of how we get there from here, do you think that would > > be > > > a sane world to end up in? Or am I wrong to want this "decide how to > override > > > the system" logic to like in the theme provider? > > > > Oh boy - you actually meant ThemeService. ThemeProperties is missing a ton of > > IDs we'd need, and apart from a weird thing on Mac, BrowserThemeProvider is > the > > only ThemeProvider, and there's no precedent for it being used for secondary > UI. > > I'm pretty sure I meant ThemeProvider; ThemeService is mainly the way you get at > the ThemeProvider, and ThemeProvider actually implements things like GetColor(). Sorry - lots of new concepts. I first thought you meant to get Harmony info out of *NativeTheme*, and I started on a journey to make kColorId_ButtonEnabledColor work for Harmony. ThemeProvider was a curveball I didn't expect. > > > I think there's too much that ThemeService/ThemeProivder has which we don't > want > > for secondary UI - trying to shoehorn it will add complexity to secondary UI > > where it's not needed. > > I'm confused what this means. Calling GetColor() on the theme provider isn't > hard. What sort of complexity were you envisioning this introducing? Some examples: We need an ID. If we're re-using ThemeProviders, they need the same ID namespace. that involves exposing all the browser-window theme IDs to ThemedTypographyProvider and having to know which one to use. views::Combobox also needs how to draw itself -- it can't know about the same IDs, so we'll need multiple lists. Maybe there are ways around these, but these are my concerns. > > ThemeService is also missing a lot. So I think we'd be > > making both systems more complex. > > We'd definitely need to add more color IDs, and the code to understand when to > return "built in" colors versus "underlying system" colors. That seems like a > sizeable, but relatively isolated, chunk of work. If this means we're also making all existing Theme[Suppliers?] responsible for understanding the new IDs, or having the potential to refer to concepts like "Body1 Body2" then I am not in favour of this. > > The question should be "what extra use cases > > are satisfied by the more complex solution". > > The main thing is code clarity. Every place in the code that wants a color goes > to the theme provider. The theme provider becomes the single place for all > Chrome default colors. The underlying native theme becomes the place that > returns all system colors. No one else has any colors. > > Right now we have colors in a lot of places and people are accessing various > levels directly. This makes it hard to fix something like the bug you filed > earlier on how we get all kinds of things wrong in high contrast. Currently I'm trying to solve this for text colors. TypographyProvider is text. If you want line spacing, typeface, boldness, size, or color for text, you ask TypographyProvider. TypographyProvider has a clear job to map from a spec from designers into something that's used in code. ThemeProvider has a load of other baggage. Some of the text properties are linked, e.g. boldness and color for text is linked, updates to the spec shouldn't have to modify code in two places. Again, more complexity could be added to Themes to support boldness and other font properties, but this is getting further from the goal of having a single class that encapsulates "The Spec" from designers. I don't think "The Spec" should be mixed up into themes just so we can have it as a single source of colours, since it doesn't reflect reality. Reality is: NativeTheme, The Harmony Spec, Browser Themes - they are all "ground truths". > > I guess the main one would be that > > secondary UI could match the browser theme. > > That would be a nice added bonus. It's not my main motivation. > > I do think we want that from a product perspective though. If you install a > dark theme but all your menus are still white, that feels half-baked. I agree on this point. But long term. Harmony is too shackled in feature creep already. > > From an engineering perspective, it's already hard to make > > decisions how to map secondary UI concepts to native themes - doing that for > > Chrome themes as well frightens me. > > My goal is to actually make this much easier. The various providers and > controls don't need any custom logic or thought anymore. This becomes entirely > the job of the ThemeProvider. > > > I think ThemeService querying the typography provider -- for text -- when it > > needs a default is the right layering, if we want to start combining the two. > > I don't think this is right. That would mean no one can query the typography > provider for colors directly, because the theme provider can theoretically > override it, and we need to be consistent. Or if we ban the theme provider from > overriding the typography system, it means the typography provider has to deal > with logic like "do we want to override the system colors", which ultimate we > have to add to the theme provider anyway, so we get duplicated complicated code. I would like to backtrack on this statement :). I think ThemedTypographyProvider I described can avoid the concerns you raise here. > > > > If that would make sense as a long-term world, then what's the path towards > > that > > > that's actually doable now, since I suspect we don't want to block Harmony > > phase > > > 1 on "rewrite theme layering"? Perhaps a change like this at least doesn't > > take > > > us further away, since it moves away from using the native colors directly, > > > which we'd need to do regardless? It doesn't seem like it gets us much > > closer, > > > but I'm not sure I see a whole lot of things that do, that give us > meaningful > > > win immediately. > > > > If we want to support new use cases, I think it would be straightforward to > add > > a `ThemedTypographyProvider` which knows how to merge concepts from native > > themes, the harmony spec, and a Chrome theme. But I think it would be best to > > isolate that complexity so that it only comes into play when Chrome is running > > with a theme. That is, we should always have a provider which is completely > > unaware of Chrome themes. I think the way > > ThemeService/ThemeProvider/ThemeSupplier is constructed, that's not possible > -- > > everything seems twisted up in the extension system. > > To me this answer is a reflection of the fact that making TypographyProvider > understand anything at all about colors, let alone the complexity of how > native/default/themed colors interact, is an inverted layering. > > Your answer is making me feel more confident of my answer rather than less. > That was not your intent, I think :) I see HarmonyTypographyProvider as an encapsulation of the Harmony spec. If you change it, you're changing the Harmony spec. Mixing in more concepts, or mixing it with other classes loses that. In reality, there are tradeoffs: The NativeTheme fallbacks for high-contrast mode, since designers don't care about it right now, but we need something. This Windows 10 native stuff. The very start of http://crbug.com/691891 was about mapping the harmony spec to code. If we head in the direction you propose, it sounds like we will be throwing all that out so we can mix in the harmony spec with a bunch of other concepts.
On 2017/06/30 08:06:22, tapted wrote: > On 2017/06/30 07:31:20, Peter Kasting wrote: > > On 2017/06/30 07:12:51, tapted wrote: > > > I think there's too much that ThemeService/ThemeProivder has which we don't > > want > > > for secondary UI - trying to shoehorn it will add complexity to secondary UI > > > where it's not needed. > > > > I'm confused what this means. Calling GetColor() on the theme provider isn't > > hard. What sort of complexity were you envisioning this introducing? > > Some examples: > > We need an ID. If we're re-using ThemeProviders, they need the same ID > namespace. that involves exposing all the browser-window theme IDs to > ThemedTypographyProvider and having to know which one to use. By "an ID", you mean "add things to the lists in theme_properties.h", right? I don't understand what "exposing all the browser-window theme IDs to ThemedTypographyProvider" means. You mean, that it could pull in this .h and look at these enums? It can already do that. Also, I still object to the concept of a ThemedTypographyProvider :). TypographyProvider should just be getting all colors for everything from the ThemeProvider at all times, there's no NotThemedTypographyProvider. > views::Combobox also needs how to draw itself -- it can't know about the same > IDs, so we'll need multiple lists. This is basically the "views vs. chrome" distinction, right? The issue here is presumably that theme_properties.h itself is in chrome/ right now but some IDs need to be specified in a ui/ dir so controls in views can ask for them. That's basically the same issue we've solved a couple of times now with the layout and typography providers, so doing this doesn't seem problematic. > > > ThemeService is also missing a lot. So I think we'd be > > > making both systems more complex. > > > > We'd definitely need to add more color IDs, and the code to understand when to > > return "built in" colors versus "underlying system" colors. That seems like a > > sizeable, but relatively isolated, chunk of work. > > If this means we're also making all existing Theme[Suppliers?] responsible for > understanding the new IDs, or having the potential to refer to concepts like > "Body1 Body2" then I am not in favour of this. No, things like "body 1" are concepts that the typography provider uses. When it talks to the theme provider it maps these to color IDs like "enabled button color" and the like. I don't know what a ThemeSupplier is. Are you referring to "making theme authors have to know about things"? If so, no, I'm not. Or to "having to implement support for new color IDs in ThemeProvider implementations"? Yes, I am on that. > > > The question should be "what extra use cases > > > are satisfied by the more complex solution". > > > > The main thing is code clarity. Every place in the code that wants a color > goes > > to the theme provider. The theme provider becomes the single place for all > > Chrome default colors. The underlying native theme becomes the place that > > returns all system colors. No one else has any colors. > > > > Right now we have colors in a lot of places and people are accessing various > > levels directly. This makes it hard to fix something like the bug you filed > > earlier on how we get all kinds of things wrong in high contrast. > > Currently I'm trying to solve this for text colors. TypographyProvider is text. > If you want line spacing, typeface, boldness, size, or color for text, you ask > TypographyProvider. TypographyProvider has a clear job to map from a spec from > designers into something that's used in code. Yes, and that's not changing. I'm saying that in the case of color, the mapping it would do is partial: it maps from context and style to specific color IDs, but the theme provider tells you the actual color value for those color IDs, just as it would tell everything else everywhere in the code the correct color if they ask directly for a particular ID. > ThemeProvider has a load of other > baggage. Some of the text properties are linked, e.g. boldness and color for > text is linked, updates to the spec shouldn't have to modify code in two places. > > Again, more complexity could be added to Themes to support boldness and other > font properties, but this is getting further from the goal of having a single > class that encapsulates "The Spec" from designers. I don't want Themes to support boldness and other font properties. Just colors. I don't think "change the boldness and color of a font in a button" requiring us to change values in two source files (for boldness and color) is a problem. > Reality is: NativeTheme, The Harmony > Spec, Browser Themes - they are all "ground truths". No, they're not. They're layered atop each other. The spec for Harmony is written like a standalone spec because the designers have failed to sufficiently consider native themes and a11y, and on asking them repeatedly how this works, they've basically punted the question. And maybe we don't answer it perfectly right now, but some day we will have to, and we will need something layered to do so. > > > I guess the main one would be that > > > secondary UI could match the browser theme. > > > > That would be a nice added bonus. It's not my main motivation. > > > > I do think we want that from a product perspective though. If you install a > > dark theme but all your menus are still white, that feels half-baked. > > I agree on this point. But long term. Harmony is too shackled in feature creep > already. Yes, I'm not trying to get this in now. > > To me this answer is a reflection of the fact that making TypographyProvider > > understand anything at all about colors, let alone the complexity of how > > native/default/themed colors interact, is an inverted layering. > > > > Your answer is making me feel more confident of my answer rather than less. > > That was not your intent, I think :) > > I see HarmonyTypographyProvider as an encapsulation of the Harmony spec. If you > change it, you're changing the Harmony spec. Mixing in more concepts, or mixing > it with other classes loses that. A spec exists to be implemented, not the other way around. The behavior is the ultimate guide, not the spec of the behavior. I am saying that, as a long-term vision, the behavior we need is a layered one. There is no inherent value in trying to write classes that capture "the Harmony spec" with any particular granularity or encapsulation, if they do not actually map to the behavior we want. If they do, great! The behavior we want in the long run is that all parts of the UI respect system themes, and the colors of, say, the text on buttons and the arrows on buttons both decide to use the system colors vs. the default ones at the same time. That sounds to me like a system that is best served by centralizing the decision of the actual color values in the place we've already built to serve color values, and making the TypographyProvider do the work of mapping context/style values down to specific color IDs to isolate the theme system from that. I may be wrong. But I feel like we're not yet on the same page in terms of understanding each others' proposals completely, so it's hard to tell.
BTW, note that this is basically already what you're doing in the DefaultTypographyProvider where you map to color IDs and then ask for their color. Effectively, the proposed change from the typographyprovider perspective is so do something a bit like what DefaultTypographyProvider::GetColor() is doing in https://codereview.chromium.org/2964453002 in all cases.
In the meantime while we're discussing the larger long-term vision -- this looks like a positive change overall in the short term. Some comments below that are more locally-nitpicky, but if you're convinced I'm wrong about them/they don't make sense to address, LGTM. https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right): https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:58: const bool inverted_scheme = Ideally, avoid looking for an inverted scheme and then toggling below; instead, use GetReadableColor() with something you think is a representative color for the background where you'd expect the proposed foreground to appear. (I realize you don't know what this might be, but doing something like picking the window or button background can't be any less accurate than checking for the button enabled color being white.) If for some reason this absolutely wouldn't work, I'd use utilities like IsDark() to check for "darker vs. lighter" instead of looking at "== white" directly. (A similar comment may apply in line 35, where it's not obvious to me that "label color is black" is the right test. In an ideal world, we'd probably adjust our colors color-by-color, or else punt to default/theme colors when any custom theme is enabled. But even in a "test one color" sort of world, maybe we should be looking for "default color is dark enough" rather than "black"?) https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:63: // will skip the NativeTheme entirely. Nit: Avoid Windows-specific IDs in comments here and below (and honestly maybe above), as it doesn't really give the reader needed info but it does confuse the issue on non-Win. I think it's clearer to always simply use the nativetheme IDs (abbreviated) rather than referring to the underlying Windows native names. https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:140: return SkColorSetRGB(0x9e, 0x9e, 0x9e); The spec doesn't actually seem to give button-specific colors for disabled buttons, so I think my comment about #000 w/alpha below applies here. https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:142: return SkColorSetRGB(0x75, 0x75, 0x75); Shouldn't this be #5a5a5a? Or am I misreading the spec? https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:150: return SkColorSetRGB(0x9e, 0x9e, 0x9e); While here: I read the spec as saying these are actually supposed to all be #000 with different alphas (87%, 54%, 38%) rather than opaque colors. It does provide opaque values, but those seem to be explanatory rather than normative.
Description was changed from ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. Note we still use NativeTheme when the Harmony is ignored (e.g. OS themes with inverted or high-constrast themes). This CL refactors the code in HarmonyTypographyProvider to be more explicit about that. BUG=691891, 738292 ========== to ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. Note we still use NativeTheme when the Harmony spec is ignored (e.g. OS themes with inverted or high-constrast themes). This CL refactors the code in HarmonyTypographyProvider to be more explicit about when the Harmony spec needs to be ignored. BUG=691891, 738292 ==========
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...
We should probably more the design discussion to a design doc :). I think we've got different implementations in our brains, so in the details we're debating for/against different things and it's just confusing. There may still be something fundamentally different in our perspectives, in that I'm not in favour of a multi-layered approach for this. I think layers are working badly for theme colors. Adding a layer removes context about where the color is, and why it's being chosen. At the lower layer, it makes it harder for the right color to be chosen since there is less context. At the higher layer, it makes "post-processing" like GetReadableColor compulsory since it can never trust that the color provided is sensible. Having a "stack" of layers requires multiple, giant switch statements and makes reasoning about colors very difficult. Yes, we still need layers: details, and platform inconsistency, must be abstracted away. But I think we would benefit from some kind of "Color Decider" which is a single layer with all the context. It knows how to pick from multiple sources of color, and each source of color may have its own/disjoint "color ID namespace" so that it can describe the colors it is responsible for in the way that best matches the set of colors it is able to provide. E.g. we wouldn't have a NativeTheme providing "BlueButton" colors, since that's not a concept that exists in native themes. (Neither would a NativeTheme have different "bubble"/"dialog" colors, or "Positive/Negative/High/Medium/Low" severity colors). Similarly, the cs.chromium.org/CustomThemeSupplier would _only_ offer colors that can be overridden by an installable Chrome theme, and none of ThemeProperties::NotOverwritableByUserThemeProperty So I think we should not aim to map context and style to specific color IDs. It loses context, makes a large number of IDs, and embeds some non-trivial logic for mapping from IDs to color constants at multiple layers. DefaultTypographyProvider does it, but I don't think it generalizes well to the use cases it doesn't need to support. https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right): https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:58: const bool inverted_scheme = On 2017/06/30 08:56:12, Peter Kasting wrote: > Ideally, avoid looking for an inverted scheme and then toggling below; instead, > use GetReadableColor() with something you think is a representative color for > the background where you'd expect the proposed foreground to appear. (I realize > you don't know what this might be, but doing something like picking the window > or button background can't be any less accurate than checking for the button > enabled color being white.) > > If for some reason this absolutely wouldn't work, I'd use utilities like > IsDark() to check for "darker vs. lighter" instead of looking at "== white" > directly. (A similar comment may apply in line 35, where it's not obvious to me > that "label color is black" is the right test. In an ideal world, we'd probably > adjust our colors color-by-color, or else punt to default/theme colors when any > custom theme is enabled. But even in a "test one color" sort of world, maybe we > should be looking for "default color is dark enough" rather than "black"?) I've discovered color_utils::IsInvertedColorScheme() - I think it captures what you are after from the second paragraph. I don't think GetReadableColor is right here - per the comment about matching user expectations - I think we'd actually want to match a color from the theme. High contrast themes simplify the UI - generating new colors will work against that. And views::Label already applies GetReadableColor (and permits code to override that). Also GetReadableColor says "This won't do anything but waste time if the supplied foreground color has a luma value close to the midpoint (0.5 in the HSL representation)." Disabled colors are exactly that - e.g. 0.54a. Technically.. "solid yellow" is also that (i.e. L in HSL is 0.5) but color_util's `GetLuma/GetRelativeLuminance()` is not derived from HSL :/ https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:63: // will skip the NativeTheme entirely. On 2017/06/30 08:56:12, Peter Kasting wrote: > Nit: Avoid Windows-specific IDs in comments here and below (and honestly maybe > above), as it doesn't really give the reader needed info but it does confuse the > issue on non-Win. I think it's clearer to always simply use the nativetheme IDs > (abbreviated) rather than referring to the underlying Windows native names. Done. However, I think it does give the reader needed info.. The point of the comments is to make it clear where colors come from if a future reader ever gets here and needs to debug an issue. 99% of the cases where a Chrome process will get here `in the wild` will be because of a high-contrast theme on Windows, and will be directly influenced by those constants. Burying it under layers makes that harder, IMO. For code, sure. Referring to Windows constants in code here would be unnecessary and undesirable coupling. But I don't think that applies to comments. The constants are very strongly related. https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:140: return SkColorSetRGB(0x9e, 0x9e, 0x9e); On 2017/06/30 08:56:12, Peter Kasting wrote: > The spec doesn't actually seem to give button-specific colors for disabled > buttons, so I think my comment about #000 w/alpha below applies here. There's a "disabled" column in the 2017-06-28 spec attached to http://crbug.com/691891 -- https://bugs.chromium.org/p/chromium/issues/attachment?aid=290994&inline=1 It says "Font color: #000 0.38a (#757575)" -- #000 @ 0.38a is actually #9e9e9e (the 757575 is just a typo I think, since that's what the old color was, and in the new spec it wouldn't be any different to non-disabled). https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:142: return SkColorSetRGB(0x75, 0x75, 0x75); On 2017/06/30 08:56:12, Peter Kasting wrote: > Shouldn't this be #5a5a5a? Or am I misreading the spec? That's the old color :). This change is to make it lighter per the latest, button specific spec. The original sticker sheet (which covers more than just buttons) is obsolete for buttons. https://codereview.chromium.org/2957263002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:150: return SkColorSetRGB(0x9e, 0x9e, 0x9e); On 2017/06/30 08:56:12, Peter Kasting wrote: > While here: I read the spec as saying these are actually supposed to all be #000 > with different alphas (87%, 54%, 38%) rather than opaque colors. It does > provide opaque values, but those seem to be explanatory rather than normative. I think it's the opposite - if we added an alpha component, the color would change depending on the background, but I've seen no indication we should ever do that from the spec. The spec would also need to be explicit about the compositing order: dialog background, button background, hover shade, inkdrop, and text are all layered but only the inkdrop is influenced by other colors. transparent colors are more expensive to draw and raise questions around how subpixel antialiasing should work. Unless we are actually drawing text over something like a gradient AND want the font to have different color values depending where it is on the gradient, then I think we should never have alpha values in font colors. Also methods like color_utils::GetReadableColor say "alpha values will be ignored". Basically alpha makes lots of things more complex and we don't have a use-case that requires alpha for text.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2957263002/#ps100001 (title: "color_utils::IsInvertedColorScheme(), fewer Win constants")
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": 100001, "attempt_start_ts": 1499063559133030,
"parent_rev": "92609111d08f0235ac8337a8d06d1def4d322383", "commit_rev":
"d007b3e7c64c2fdaf91c70ed2bc89205fcdfcb41"}
Message was sent while issue was closed.
Description was changed from ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. Note we still use NativeTheme when the Harmony spec is ignored (e.g. OS themes with inverted or high-constrast themes). This CL refactors the code in HarmonyTypographyProvider to be more explicit about when the Harmony spec needs to be ignored. BUG=691891, 738292 ========== to ========== Update dialog button text color for Harmony per the latest button spec Only use the new colors when using HarmonyTypographyProvider. Diverge from NativeTheme for this since there's too much indirection. `kButtonEnabledColor` is gfx::kChromeIconGrey, but kButtonEnabledColor is used for lots of other things. Note we still use NativeTheme when the Harmony spec is ignored (e.g. OS themes with inverted or high-constrast themes). This CL refactors the code in HarmonyTypographyProvider to be more explicit about when the Harmony spec needs to be ignored. BUG=691891, 738292 Review-Url: https://codereview.chromium.org/2957263002 Cr-Commit-Position: refs/heads/master@{#483954} Committed: https://chromium.googlesource.com/chromium/src/+/d007b3e7c64c2fdaf91c70ed2bc8... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d007b3e7c64c2fdaf91c70ed2bc8...
Message was sent while issue was closed.
As you note, detailed design arguments probably shouldn't be hashed out on a code review. So I'll just give you a sketch of my ultimate goal in hopes it explains where I'm coming from. A core hope of mine is that we get to the point where the design specifies no colors, sizes, font faces, etc. at all. Instead, everything would be specified in terms of how to adapt system and theme settings. For example, some particular font might become "system window text font, but with alpha 0.8 and size +2". We're nowhere close to that. Designers always want some specific look and feel, and for now we're accommodating them. But in the long run I am hoping to push us toward a unified underlying set of system + theme-provided colors, font sizes, etc. and then make the design work in those terms. It will be much better for accessibility and customization. And in my head, the way that works is that we have low-level infrastructure like the theme and font systems that give you the core primitives, and then higher-level services like typography provider that blend those primitives into specific combinations for common use. Anyway, as you note, we can try and hash out those details more later. I don't think we need to agree on either the specifics or the broad strokes for now -- the patch as landed here seems like a win. :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
