Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629293002:
Try to improve toolbar separator coloration for themed browser windows. (Closed)
|
Created:
3 years, 11 months ago by Evan Stade Modified:
3 years, 6 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTry to improve toolbar separator coloration for themed browser windows.
Particularly noticeable for dark themes.
There are two colors here: the separator below the toolbar/attached
bookmark bar, and the separator below the detached bookmark bar. The
latter is intentionally lighter than the former.
There were a couple problems to fix. The luminance calculation didn't
make much sense when the base color was not opaque, so that is changed
to retain complete opacity. That's also changed to create a less
noticeable separator, which is more in line with the value of the
default theme's hardcoded colors relative to the text color.
The default browser theme should be unaffected by this change. Themed
windows will get slightly less noticeable separators, which addresses
the symptoms shown in the linked bug.
BUG=628312
Patch Set 1 #
Total comments: 2
Patch Set 2 : also change gtk a little #
Total comments: 10
Patch Set 3 : tweaks #Patch Set 4 : revert #
Total comments: 2
Messages
Total messages: 11 (4 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2629293002/diff/1/chrome/browser/themes/theme... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/2629293002/diff/1/chrome/browser/themes/theme... chrome/browser/themes/theme_service.cc:499: return color_utils::BlendTowardOppositeLuma( The 50% didn't match up well with what the default theme did: 0xb6b4b6 for normal, which is about 70% white (bg) and 25% black (text), and 0x282828 for incognito, which is about 85% black (bg) and 15% white (text). So this now uses 75% bg and 25% text. https://codereview.chromium.org/2629293002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (left): https://codereview.chromium.org/2629293002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:227: tp->GetColor(ThemeProperties::COLOR_DETACHED_BOOKMARK_BAR_SEPARATOR); This code looks weird because when the code was originally written, it drew a separator at the top as well (where the luma wasn't touched). As part of MD, that was changed to be drawn by the toolbar instead. Now there are two separate color identifiers so the calculation can be done in ThemeService.
The CQ bit was checked by estade@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...
On 2017/01/13 01:16:52, Evan Stade wrote: > https://codereview.chromium.org/2629293002/diff/1/chrome/browser/themes/theme... > File chrome/browser/themes/theme_service.cc (right): > > https://codereview.chromium.org/2629293002/diff/1/chrome/browser/themes/theme... > chrome/browser/themes/theme_service.cc:499: return > color_utils::BlendTowardOppositeLuma( > The 50% didn't match up well with what the default theme did: 0xb6b4b6 for > normal, which is about 70% white (bg) and 25% black (text), and 0x282828 for > incognito, which is about 85% black (bg) and 15% white (text). So this now uses > 75% bg and 25% text. > > https://codereview.chromium.org/2629293002/diff/1/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/browser_view.cc (left): > > https://codereview.chromium.org/2629293002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:227: > tp->GetColor(ThemeProperties::COLOR_DETACHED_BOOKMARK_BAR_SEPARATOR); > This code looks weird because when the code was originally written, it drew a > separator at the top as well (where the luma wasn't touched). As part of MD, > that was changed to be drawn by the toolbar instead. Now there are two separate > color identifiers so the calculation can be done in ThemeService. ping
https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:481: // Same as the normal toolbar separator, but with increased luminance. It seems kind of random that we increase the luminance here as a way of making this separator less noticeable. I assume that's because the original author here was thinking of a white NTP background. Shouldn't we instead blend towards whatever the NTP background color actually is? https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:500: GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT, incognito), 0xBF); Doesn't blending toward the opposite luma here assume the bookmark bar background is (whatever the opposite luma is)? That is, it seems like maybe you instead want 25% bookmark bar text, 75% bookmark bar background? (In which case you might have to raise the fraction back up from 25% since the difference won't be so large)
https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:481: // Same as the normal toolbar separator, but with increased luminance. On 2017/01/17 20:51:25, Peter Kasting wrote: > It seems kind of random that we increase the luminance here as a way of making > this separator less noticeable. I assume that's because the original author > here was thinking of a white NTP background. Shouldn't we instead blend towards > whatever the NTP background color actually is? In the dark case (e.g. incognito) it is actually more noticeable. I think for incognito if we blended towards the NTP background it would effectively be invisible. https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:500: GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT, incognito), 0xBF); On 2017/01/17 20:51:25, Peter Kasting wrote: > Doesn't blending toward the opposite luma here assume the bookmark bar > background is (whatever the opposite luma is)? I think it's a safe assumption that blending towards the opposite luma will make the separator a less dramatic version of the text color. This would only not be true if the bookmark text was something unreadable like black on black, right? Whether we want to tinge the separator color with the bookmark bar bg color is a matter of judgment I suppose, but the text doesn't do that so I don't know why the separator should.
https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:481: // Same as the normal toolbar separator, but with increased luminance. On 2017/01/18 00:16:59, Evan Stade wrote: > On 2017/01/17 20:51:25, Peter Kasting wrote: > > It seems kind of random that we increase the luminance here as a way of making > > this separator less noticeable. I assume that's because the original author > > here was thinking of a white NTP background. Shouldn't we instead blend > towards > > whatever the NTP background color actually is? > > In the dark case (e.g. incognito) it is actually more noticeable. Which seems like a bug. Why would we want a stronger separation between the detached bar bottom and the area below it than between the toolbar and the detached bar? And why would we want it to be less noticeable in regular, but more in incognito? What is the computed toolbar separator color in incognito with your changes? This would help answer the question of how visible it would be if we blend toward the NTP background color. https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:500: GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT, incognito), 0xBF); On 2017/01/18 00:16:59, Evan Stade wrote: > On 2017/01/17 20:51:25, Peter Kasting wrote: > > Doesn't blending toward the opposite luma here assume the bookmark bar > > background is (whatever the opposite luma is)? > > I think it's a safe assumption that blending towards the opposite luma will make > the separator a less dramatic version of the text color. This would only not be > true if the bookmark text was something unreadable like black on black, right? > Whether we want to tinge the separator color with the bookmark bar bg color is a > matter of judgment I suppose, but the text doesn't do that so I don't know why > the separator should. The effect of the previous alpha-blend was, basically, to blend the text color toward the background color. You said you were trying to make this fully opaque; I'm looking for a way to duplicate the old effect more closely in that world. My worry about blending toward opposite luma is that the level of contrast you get is much less predictable than with "blend toward background color".
Description was changed from ========== Try to improve toolbar separator coloration for themed browser windows. Particularly noticeable for dark themes. There are two colors here: the separator below the toolbar/attached bookmark bar, and the separator below the detached bookmark bar. The latter is intentionally lighter than the former. There were a couple problems to fix. The luminance calculation didn't make much sense when the base color was not opaque, so that is changed to retain complete opacity. That's also changed to create a less noticeable separator, which is more in line with the value of the default theme's hardcoded colors relative to the text color. The default browser theme should be unaffected by this change. Themed windows will get slightly less noticeable separators, which addresses the symptoms shown in the linked bug. BUG=628312 ========== to ========== Try to improve toolbar separator coloration for themed browser windows. Particularly noticeable for dark themes. There are two colors here: the separator below the toolbar/attached bookmark bar, and the separator below the detached bookmark bar. The latter is intentionally lighter than the former. There were a couple problems to fix. The luminance calculation didn't make much sense when the base color was not opaque, so that is changed to retain complete opacity. That's also changed to create a less noticeable separator, which is more in line with the value of the default theme's hardcoded colors relative to the text color. The default browser theme should be unaffected by this change. Themed windows will get slightly less noticeable separators, which addresses the symptoms shown in the linked bug. BUG=628312 ==========
https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:481: // Same as the normal toolbar separator, but with increased luminance. On 2017/01/18 00:40:16, Peter Kasting wrote: > On 2017/01/18 00:16:59, Evan Stade wrote: > > On 2017/01/17 20:51:25, Peter Kasting wrote: > > > It seems kind of random that we increase the luminance here as a way of > making > > > this separator less noticeable. I assume that's because the original author > > > here was thinking of a white NTP background. Shouldn't we instead blend > > towards > > > whatever the NTP background color actually is? > > > > In the dark case (e.g. incognito) it is actually more noticeable. > > Which seems like a bug. Why would we want a stronger separation between the > detached bar bottom and the area below it than between the toolbar and the > detached bar? And why would we want it to be less noticeable in regular, but > more in incognito? In practice, it doesn't look like a bug. You can test it easily enough because this is what Chrome already does. > > What is the computed toolbar separator color in incognito with your changes? > This would help answer the question of how visible it would be if we blend > toward the NTP background color. The bottom separator is essentially invisible in incognito as it is because the contrast to the ntp bg is very low. This is per Sebastien's design. If we blend towards the NTP bg color it is even lower in contrast and the floating bookmark bar will essentially not have a separator. If we're talking about the default theme here, this patch intentionally leaves the current behavior intact. I don't think there's a bug here. https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:500: GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT, incognito), 0xBF); On 2017/01/18 00:40:16, Peter Kasting wrote: > On 2017/01/18 00:16:59, Evan Stade wrote: > > On 2017/01/17 20:51:25, Peter Kasting wrote: > > > Doesn't blending toward the opposite luma here assume the bookmark bar > > > background is (whatever the opposite luma is)? > > > > I think it's a safe assumption that blending towards the opposite luma will > make > > the separator a less dramatic version of the text color. This would only not > be > > true if the bookmark text was something unreadable like black on black, right? > > Whether we want to tinge the separator color with the bookmark bar bg color is > a > > matter of judgment I suppose, but the text doesn't do that so I don't know why > > the separator should. > > The effect of the previous alpha-blend was, basically, to blend the text color > toward the background color. You said you were trying to make this fully > opaque; I'm looking for a way to duplicate the old effect more closely in that > world. My worry about blending toward opposite luma is that the level of > contrast you get is much less predictable than with "blend toward background > color". I'm not sure I follow because there's no way to predict the contrast of two colors a theme author has chosen, but I can change this to use AlphaBlend. When I do that, it looks better across the handful of tested themes if I lower the foreground percent even further. https://codereview.chromium.org/2629293002/diff/60001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/2629293002/diff/60001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:494: return GetColor(ThemeProperties::COLOR_TOOLBAR, incognito); tangent: I noticed that on many themes this is better if it's COLOR_NTP_BACKGROUND, but if we made that change the text might not be readable on it (for example, toolbar is dark text on a light bg and ntp is light text on a dark bg)
https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:481: // Same as the normal toolbar separator, but with increased luminance. On 2017/01/24 00:46:08, Evan Stade wrote: > On 2017/01/18 00:40:16, Peter Kasting wrote: > > On 2017/01/18 00:16:59, Evan Stade wrote: > > > On 2017/01/17 20:51:25, Peter Kasting wrote: > > > > It seems kind of random that we increase the luminance here as a way of > > making > > > > this separator less noticeable. I assume that's because the original > author > > > > here was thinking of a white NTP background. Shouldn't we instead blend > > > towards > > > > whatever the NTP background color actually is? > > > > > > In the dark case (e.g. incognito) it is actually more noticeable. > > > > Which seems like a bug. Why would we want a stronger separation between the > > detached bar bottom and the area below it than between the toolbar and the > > detached bar? And why would we want it to be less noticeable in regular, but > > more in incognito? > > In practice, it doesn't look like a bug. You can test it easily enough because > this is what Chrome already does. > > > > > What is the computed toolbar separator color in incognito with your changes? > > This would help answer the question of how visible it would be if we blend > > toward the NTP background color. > > The bottom separator is essentially invisible in incognito as it is because the > contrast to the ntp bg is very low. This is per Sebastien's design. If we blend > towards the NTP bg color it is even lower in contrast and the floating bookmark > bar will essentially not have a separator. > > If we're talking about the default theme here, this patch intentionally leaves > the current behavior intact. I don't think there's a bug here. Staring at the current behavior more closely, I think blending the toolbar separator lighter happens to work for normal and incognito windows happens to work, but only coincidentally; and my suggestion of blending towards the NTP background color is wrong too. The problem is that both of these start from the toolbar separator color, but that isn't guaranteed to have any relationship to what we want. In the default theme, the detached bookmark bar and the NTP background are always both the same color: white for normal windows, dark grey for incognito. What we want in a separator is something that will contrast with both of those. Today, lightening the toolbar separator color produces a medium grey, which is darker than the default window's white and lighter than the incognito window's dark grey, and thus is visible in both cases. But there's nothing guaranteeing this will still be true with arbitrary theme colors. This is basically the same problem we had to solve with ThemeService::GetSeparatorColor(): given two arbitrary colors, find a third that contrasts with both. I wonder if we could just call GetSeparatorColor(), or maybe tweak the kAlpha value it uses (i.e. pass that as a param). https://codereview.chromium.org/2629293002/diff/20001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:500: GetColor(ThemeProperties::COLOR_BOOKMARK_TEXT, incognito), 0xBF); On 2017/01/24 00:46:08, Evan Stade wrote: > On 2017/01/18 00:40:16, Peter Kasting wrote: > > On 2017/01/18 00:16:59, Evan Stade wrote: > > > On 2017/01/17 20:51:25, Peter Kasting wrote: > > > > Doesn't blending toward the opposite luma here assume the bookmark bar > > > > background is (whatever the opposite luma is)? > > > > > > I think it's a safe assumption that blending towards the opposite luma will > > make > > > the separator a less dramatic version of the text color. This would only not > > be > > > true if the bookmark text was something unreadable like black on black, > right? > > > Whether we want to tinge the separator color with the bookmark bar bg color > is > > a > > > matter of judgment I suppose, but the text doesn't do that so I don't know > why > > > the separator should. > > > > The effect of the previous alpha-blend was, basically, to blend the text color > > toward the background color. You said you were trying to make this fully > > opaque; I'm looking for a way to duplicate the old effect more closely in that > > world. My worry about blending toward opposite luma is that the level of > > contrast you get is much less predictable than with "blend toward background > > color". > > I'm not sure I follow because there's no way to predict the contrast of two > colors a theme author has chosen, I just meant, if we're trying to guarantee the separator contrasts less than the bookmark text, then blending toward background will achieve that, but blending toward opposite luma could contrast more. > but I can change this to use AlphaBlend. When > I do that, it looks better across the handful of tested themes if I lower the > foreground percent even further. I wonder if this, too, should be using GetSeparatorColor() somehow. This would likely change the behavior for the default incognito theme (which I'd be OK with since the black separator used there today seems invisible to me). https://codereview.chromium.org/2629293002/diff/60001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/2629293002/diff/60001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:494: return GetColor(ThemeProperties::COLOR_TOOLBAR, incognito); On 2017/01/24 00:46:08, Evan Stade wrote: > tangent: I noticed that on many themes this is better if it's > COLOR_NTP_BACKGROUND I agree; see https://bugs.chromium.org/p/chromium/issues/detail?id=618335 , especially comment 9 and comment 12. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
