Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2616273002:
Native Themes: Add table header colors (Closed)
|
Created:
3 years, 11 months ago by Tom (Use chromium acct) Modified:
3 years, 11 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNative Themes: Add table header colors
This CL adds NativeTheme enum values for table header colors. This should
improve the look of dark themes.
See images in:
https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38
BUG=132847
R=estade@chromium.org,erg@chromium.org,sky@chromium.org
Review-Url: https://codereview.chromium.org/2616273002
Cr-Commit-Position: refs/heads/master@{#445176}
Committed: https://chromium.googlesource.com/chromium/src/+/af0cc1e8e62b43c6db0b857c2abf1da4f99d4ef1
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Remove gradient #Patch Set 4 : Update default colors #
Total comments: 4
Patch Set 5 : Refactor #
Total comments: 7
Patch Set 6 : Address sky@ and estade@'s comments #
Total comments: 6
Patch Set 7 : estade@'s comments #
Total comments: 2
Patch Set 8 : Add gradients back #
Messages
Total messages: 44 (18 generated)
Description was changed from ========== Native Themes: Add table header colors BUG=132847 R=estade@chromium.org,erg@chromium.org ========== to ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This improves the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org ==========
Description was changed from ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This improves the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org ========== to ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This improves the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org ==========
Description was changed from ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This improves the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org ========== to ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This should improve the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org ==========
thomasanderson@google.com changed reviewers: + sky@chromium.org
erg: chrome/browser/ui/libgtkui/native_theme_gtk2.cc chrome/browser/ui/libgtkui/native_theme_gtk3.cc sky: ui/views/controls/table/table_header.cc estade: ui/native_theme/common_theme.cc ui/native_theme/native_theme.h ui/native_theme/native_theme_dark_aura.cc
Description was changed from ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This should improve the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org ========== to ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This should improve the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org,sky@chromium.org ==========
Gradients have fallen out of favor and as we update UIs to Material Design we've been uniformly removing them. For example, infobars used to be a gradient and now they're solid (yellow or grey). I think we should take this opportunity to update table headers on all platforms. Perhaps we don't need any new theme colors at all (we can just use dialog bg, for example). But at the very least, we shouldn't need to add this many new colors. You might want to ping bettes@ to see if he already has plans for the task manager.
On 2017/01/07 20:09:52, Evan Stade wrote: > Gradients have fallen out of favor and as we update UIs to Material Design we've > been uniformly removing them. For example, infobars used to be a gradient and > now they're solid (yellow or grey). I think we should take this opportunity to > update table headers on all platforms. Perhaps we don't need any new theme > colors at all (we can just use dialog bg, for example). But at the very least, > we shouldn't need to add this many new colors. You might want to ping bettes@ to > see if he already has plans for the task manager. +bettes@ FYI Removed the gradient in the latest PS. There are now 3 colors added instead of 4: background, text color, and separator color. Can you PLMK which colors, specifically, should replace these (if any)?
I could speculate, but we really need Alan to weigh in. Rietveld isn't a typical medium for communication with designers; you might be better served sending an email and including screenshots to ground the discussion.
Patchset #4 (id:60001) has been deleted
On 2017/01/09 20:51:56, Evan Stade wrote: > I could speculate, but we really need Alan to weigh in. Rietveld isn't a typical > medium for communication with designers; you might be better served sending an > email and including screenshots to ground the discussion. reviewers PTAL. This CL now uses bettes@'s color suggestions (erg@, sky@ and estade@ were CC'ed on the email thread)
lgtm https://codereview.chromium.org/2616273002/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_header.cc (right): https://codereview.chromium.org/2616273002/diff/80001/ui/views/controls/table... ui/views/controls/table/table_header.cc:61: auto* theme = GetNativeTheme(); ui::NativeTheme is short enough that you should probably avoid auto in this case.
https://codereview.chromium.org/2616273002/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_header.cc (right): https://codereview.chromium.org/2616273002/diff/80001/ui/views/controls/table... ui/views/controls/table/table_header.cc:62: set_background(Background::CreateSolidBackground( Why do you need to set the background on every paint? TableHeader should override OnNativeThemeChanged() and set the background there.
https://codereview.chromium.org/2616273002/diff/80001/ui/views/controls/table... File ui/views/controls/table/table_header.cc (right): https://codereview.chromium.org/2616273002/diff/80001/ui/views/controls/table... ui/views/controls/table/table_header.cc:61: auto* theme = GetNativeTheme(); On 2017/01/18 00:01:55, Elliot Glaysher wrote: > ui::NativeTheme is short enough that you should probably avoid auto in this > case. Done. https://codereview.chromium.org/2616273002/diff/80001/ui/views/controls/table... ui/views/controls/table/table_header.cc:62: set_background(Background::CreateSolidBackground( On 2017/01/18 16:21:16, sky wrote: > Why do you need to set the background on every paint? TableHeader should > override OnNativeThemeChanged() and set the background there. Done.
https://codereview.chromium.org/2616273002/diff/100001/ui/views/controls/tabl... File ui/views/controls/table/table_header.cc (right): https://codereview.chromium.org/2616273002/diff/100001/ui/views/controls/tabl... ui/views/controls/table/table_header.cc:52: set_background( OnNativeThemeChanged should always be called (first time is when added to a widget). So, you shouldn't need to have an explicit call here.
https://codereview.chromium.org/2616273002/diff/100001/chrome/browser/ui/libg... File chrome/browser/ui/libgtkui/native_theme_gtk2.cc (right): https://codereview.chromium.org/2616273002/diff/100001/chrome/browser/ui/libg... chrome/browser/ui/libgtkui/native_theme_gtk2.cc:276: return ui::NativeTheme::GetInstanceForNativeUi()->GetSystemColor( why fall back to common colors? that will give us a light bg instead of a dark one on dark themes. https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... File ui/native_theme/common_theme.cc (right): https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... ui/native_theme/common_theme.cc:271: return base_theme->GetSystemColor(NativeTheme::kColorId_DialogBackground); Instead of adding new colors I'd just use the menu colors that already exist (which gives us the right thing on an Aura theme and a reasonable thing anywhere else), i.e. kEnabledMenuItemForegroundColor kMenuBackgroundColor kEnabledMenuButtonBorderColor
https://codereview.chromium.org/2616273002/diff/100001/chrome/browser/ui/libg... File chrome/browser/ui/libgtkui/native_theme_gtk2.cc (right): https://codereview.chromium.org/2616273002/diff/100001/chrome/browser/ui/libg... chrome/browser/ui/libgtkui/native_theme_gtk2.cc:276: return ui::NativeTheme::GetInstanceForNativeUi()->GetSystemColor( On 2017/01/19 00:49:22, Evan Stade wrote: > why fall back to common colors? that will give us a light bg instead of a dark > one on dark themes. Changed this to get the colors directly https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... File ui/native_theme/common_theme.cc (right): https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... ui/native_theme/common_theme.cc:271: return base_theme->GetSystemColor(NativeTheme::kColorId_DialogBackground); On 2017/01/19 00:49:22, Evan Stade wrote: > Instead of adding new colors I'd just use the menu colors that already exist > (which gives us the right thing on an Aura theme and a reasonable thing anywhere > else), i.e. > > kEnabledMenuItemForegroundColor > kMenuBackgroundColor > kEnabledMenuButtonBorderColor > Done. https://codereview.chromium.org/2616273002/diff/100001/ui/views/controls/tabl... File ui/views/controls/table/table_header.cc (right): https://codereview.chromium.org/2616273002/diff/100001/ui/views/controls/tabl... ui/views/controls/table/table_header.cc:52: set_background( On 2017/01/19 00:40:37, sky wrote: > OnNativeThemeChanged should always be called (first time is when added to a > widget). So, you shouldn't need to have an explicit call here. Done.
The CQ bit was checked by thomasanderson@google.com 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...
https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... File ui/native_theme/common_theme.cc (right): https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... ui/native_theme/common_theme.cc:271: return base_theme->GetSystemColor(NativeTheme::kColorId_DialogBackground); On 2017/01/19 02:03:23, Tom Anderson wrote: > On 2017/01/19 00:49:22, Evan Stade wrote: > > Instead of adding new colors I'd just use the menu colors that already exist > > (which gives us the right thing on an Aura theme and a reasonable thing > anywhere > > else), i.e. > > > > kEnabledMenuItemForegroundColor > > kMenuBackgroundColor > > kEnabledMenuButtonBorderColor > > > > Done. what I meant was the new ColorIds needn't exist.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/19 02:04:29, Evan Stade wrote: > https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... > File ui/native_theme/common_theme.cc (right): > > https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... > ui/native_theme/common_theme.cc:271: return > base_theme->GetSystemColor(NativeTheme::kColorId_DialogBackground); > On 2017/01/19 02:03:23, Tom Anderson wrote: > > On 2017/01/19 00:49:22, Evan Stade wrote: > > > Instead of adding new colors I'd just use the menu colors that already exist > > > (which gives us the right thing on an Aura theme and a reasonable thing > > anywhere > > > else), i.e. > > > > > > kEnabledMenuItemForegroundColor > > > kMenuBackgroundColor > > > kEnabledMenuButtonBorderColor > > > > > > > Done. > > what I meant was the new ColorIds needn't exist. Unfortunately that wouldn't work with the Adwaita theme (the one used by Ubuntu). Adwaita has light tables but dark menus :(
On 2017/01/20 00:48:47, Tom Anderson wrote: > On 2017/01/19 02:04:29, Evan Stade wrote: > > > https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... > > File ui/native_theme/common_theme.cc (right): > > > > > https://codereview.chromium.org/2616273002/diff/100001/ui/native_theme/common... > > ui/native_theme/common_theme.cc:271: return > > base_theme->GetSystemColor(NativeTheme::kColorId_DialogBackground); > > On 2017/01/19 02:03:23, Tom Anderson wrote: > > > On 2017/01/19 00:49:22, Evan Stade wrote: > > > > Instead of adding new colors I'd just use the menu colors that already > exist > > > > (which gives us the right thing on an Aura theme and a reasonable thing > > > anywhere > > > > else), i.e. > > > > > > > > kEnabledMenuItemForegroundColor > > > > kMenuBackgroundColor > > > > kEnabledMenuButtonBorderColor > > > > > > > > > > Done. > > > > what I meant was the new ColorIds needn't exist. > > Unfortunately that wouldn't work with the Adwaita theme (the one used by > Ubuntu). Adwaita has light tables but dark menus :( Why am I seeing a light bg and a blue highlight color with Adwaita?
> Why am I seeing a light bg and a blue highlight color with Adwaita? (on menus)
On 2017/01/20 01:16:22, Evan Stade wrote: > > Why am I seeing a light bg and a blue highlight color with Adwaita? > > (on menus) I meant Ambiance, sorry
On 2017/01/20 01:26:09, Tom Anderson wrote: > On 2017/01/20 01:16:22, Evan Stade wrote: > > > Why am I seeing a light bg and a blue highlight color with Adwaita? > > > > (on menus) > > I meant Ambiance, sorry both tables and menus are dark text on a beige bg for me with Ambiance.
re: new color ids, ok. Thanks for sending screenshots. https://codereview.chromium.org/2616273002/diff/120001/chrome/browser/ui/libg... File chrome/browser/ui/libgtkui/native_theme_gtk2.cc (right): https://codereview.chromium.org/2616273002/diff/120001/chrome/browser/ui/libg... chrome/browser/ui/libgtkui/native_theme_gtk2.cc:275: return GetBgColor(GetWindow(), NORMAL); why is this not GetTree? https://codereview.chromium.org/2616273002/diff/120001/ui/native_theme/common... File ui/native_theme/common_theme.cc (right): https://codereview.chromium.org/2616273002/diff/120001/ui/native_theme/common... ui/native_theme/common_theme.cc:266: return kEnabledMenuItemForegroundColor; can you write this as base_theme->GetColor(MenuColors)? That way other themes like mac which might override the menu colors and would rather use those for tables will do so. https://codereview.chromium.org/2616273002/diff/120001/ui/views/controls/tabl... File ui/views/controls/table/table_header.cc (left): https://codereview.chromium.org/2616273002/diff/120001/ui/views/controls/tabl... ui/views/controls/table/table_header.cc:57: set_background(Background::CreateVerticalGradientBackground( you've removed the last use of this function (in production code), so please remove it.
The CQ bit was checked by thomasanderson@google.com 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...
sky can you also review: ui/views/background.h ui/views/background.cc ui/views/examples/scroll_view_example.cc https://codereview.chromium.org/2616273002/diff/120001/chrome/browser/ui/libg... File chrome/browser/ui/libgtkui/native_theme_gtk2.cc (right): https://codereview.chromium.org/2616273002/diff/120001/chrome/browser/ui/libg... chrome/browser/ui/libgtkui/native_theme_gtk2.cc:275: return GetBgColor(GetWindow(), NORMAL); On 2017/01/20 01:46:02, Evan Stade wrote: > why is this not GetTree? The window color is usually slightly darker than the tree color, which would give a contrast to the header. But on second thought the tree color is probably the way to go just in case the theme chooses to use dark trees and light windows. Done. https://codereview.chromium.org/2616273002/diff/120001/ui/native_theme/common... File ui/native_theme/common_theme.cc (right): https://codereview.chromium.org/2616273002/diff/120001/ui/native_theme/common... ui/native_theme/common_theme.cc:266: return kEnabledMenuItemForegroundColor; On 2017/01/20 01:46:02, Evan Stade wrote: > can you write this as base_theme->GetColor(MenuColors)? That way other themes > like mac which might override the menu colors and would rather use those for > tables will do so. Done. https://codereview.chromium.org/2616273002/diff/120001/ui/views/controls/tabl... File ui/views/controls/table/table_header.cc (left): https://codereview.chromium.org/2616273002/diff/120001/ui/views/controls/tabl... ui/views/controls/table/table_header.cc:57: set_background(Background::CreateVerticalGradientBackground( On 2017/01/20 01:46:03, Evan Stade wrote: > you've removed the last use of this function (in production code), so please > remove it. Done. I had to update the uses in ui/views/examples/scroll_view_example.cc
https://codereview.chromium.org/2616273002/diff/140001/ui/views/examples/scro... File ui/views/examples/scroll_view_example.cc (right): https://codereview.chromium.org/2616273002/diff/140001/ui/views/examples/scro... ui/views/examples/scroll_view_example.cc:21: // ScrollView's content, which draws gradient color on background. nit: this is no longer accurate. Is the example still useful with a solid bg color? Might be best to move the vertical gradient code into this file instead of deleting it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2616273002/diff/140001/ui/views/examples/scro... File ui/views/examples/scroll_view_example.cc (right): https://codereview.chromium.org/2616273002/diff/140001/ui/views/examples/scro... ui/views/examples/scroll_view_example.cc:21: // ScrollView's content, which draws gradient color on background. On 2017/01/20 20:00:18, Evan Stade wrote: > nit: this is no longer accurate. > > Is the example still useful with a solid bg color? Might be best to move the > vertical gradient code into this file instead of deleting it. I've added the gradients back, so this should no longer apply
LGTM
On 2017/01/20 20:18:15, Tom Anderson wrote: > https://codereview.chromium.org/2616273002/diff/140001/ui/views/examples/scro... > File ui/views/examples/scroll_view_example.cc (right): > > https://codereview.chromium.org/2616273002/diff/140001/ui/views/examples/scro... > ui/views/examples/scroll_view_example.cc:21: // ScrollView's content, which > draws gradient color on background. > On 2017/01/20 20:00:18, Evan Stade wrote: > > nit: this is no longer accurate. > > > > Is the example still useful with a solid bg color? Might be best to move the > > vertical gradient code into this file instead of deleting it. > > I've added the gradients back, so this should no longer apply Why? We've still removed the last use in production code. It seems reasonable to move it into this file.
On 2017/01/20 20:40:51, Evan Stade wrote: > On 2017/01/20 20:18:15, Tom Anderson wrote: > > > https://codereview.chromium.org/2616273002/diff/140001/ui/views/examples/scro... > > File ui/views/examples/scroll_view_example.cc (right): > > > > > https://codereview.chromium.org/2616273002/diff/140001/ui/views/examples/scro... > > ui/views/examples/scroll_view_example.cc:21: // ScrollView's content, which > > draws gradient color on background. > > On 2017/01/20 20:00:18, Evan Stade wrote: > > > nit: this is no longer accurate. > > > > > > Is the example still useful with a solid bg color? Might be best to move the > > > vertical gradient code into this file instead of deleting it. > > > > I've added the gradients back, so this should no longer apply > > Why? We've still removed the last use in production code. It seems reasonable to > move it into this file. ah, nevermind, actually read the code --- lgtm!
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org Link to the patchset: https://codereview.chromium.org/2616273002/#ps160001 (title: "Add gradients back")
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": 160001, "attempt_start_ts": 1484945121731980,
"parent_rev": "efd05aec93fad28adb8bef9674530e7bbe64766f", "commit_rev":
"af0cc1e8e62b43c6db0b857c2abf1da4f99d4ef1"}
Message was sent while issue was closed.
Description was changed from ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This should improve the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org,sky@chromium.org ========== to ========== Native Themes: Add table header colors This CL adds NativeTheme enum values for table header colors. This should improve the look of dark themes. See images in: https://bugs.chromium.org/p/chromium/issues/detail?id=132847#c38 BUG=132847 R=estade@chromium.org,erg@chromium.org,sky@chromium.org Review-Url: https://codereview.chromium.org/2616273002 Cr-Commit-Position: refs/heads/master@{#445176} Committed: https://chromium.googlesource.com/chromium/src/+/af0cc1e8e62b43c6db0b857c2abf... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/af0cc1e8e62b43c6db0b857c2abf... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
