Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1819443002:
MacViews: draw combobox arrow backgrounds (Closed)
|
Created:
4 years, 9 months ago by Elly Fong-Jones Modified:
4 years, 9 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: draw combobox arrow backgrounds
Combobox arrow backgrounds on Mac are a different color from the
combobox text
background. This CL introduces a Mac-specific ComboboxBackgroundMac
class and a shim in PlatformStyle to allocate combobox backgrounds,
which returns null on all other platforms.
BUG=543683
Committed: https://crrev.com/bbfac0443569e1049214e2904524a31ddc16f951
Cr-Commit-Position: refs/heads/master@{#382910}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixes! #
Total comments: 13
Patch Set 3 : use Background instead of a separate Part #
Total comments: 30
Patch Set 4 : final fixes #Patch Set 5 : fix linux build #
Total comments: 2
Messages
Total messages: 31 (13 generated)
Description was changed from ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a separate UI part for combobox arrow backgrounds, and adds hooks in NativeTheme to draw them in platform-specific ways. This CL also adds a utility class gfx::Canvas::ScopedState, which encapsulates this common pattern: canvas->Save(); canvas->ClipRect(...); // or other stateful operations canvas->Restore(); BUG=543683 TEST=adhoc Build views_examples_with_content_exe and check the Combobox page. ========== to ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a separate UI part for combobox arrow backgrounds, and adds hooks in NativeTheme to draw them in platform-specific ways. This CL also adds a utility class gfx::Canvas::ScopedState, which encapsulates this common pattern: canvas->Save(); canvas->ClipRect(...); // or other stateful operations canvas->Restore(); BUG=543683 TEST=adhoc Build views_examples_with_content_exe and check the Combobox page. ==========
ellyjones@chromium.org changed reviewers: + tapted@chromium.org
tapted: ptal? :)
We can perhaps pull out the `make-gradient-in-the-style-we-want-for-macviews` logic. Maybe that belongs in platform_style_mac.h/cc, and it can be used by DialogButtonBorderMac and ComboboxBorderMac Gah, I really need to update that cofaux CL at https://codereview.chromium.org/1569113002/ -- it should be using this make-gradient utility function too. https://codereview.chromium.org/1819443002/diff/1/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/1819443002/diff/1/ui/gfx/canvas.h#newcode77 ui/gfx/canvas.h:77: // This class represents a scoped state of a Canvas. When a ScopedState is ui/gfx/scoped_canvas.h? https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_theme.h File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme.h:48: kComboboxArrowBackground, My first thought (I don't know if it's a good one or not) is to make this part of the border. That's what's done currently for buttons. that could let us get away with fewer (or no?) changes to combobox.cc -- maybe we just need to ensure isets are dealt with properly. https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_mac.mm (right): https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_mac.mm:280: SkColor light_blue = SkColorSetRGB(0x4e, 0x97, 0xfd); Are there things we can pick from ui/gfx/color_palette.h? The mocks we got are meant to be inspired by MD colors, so it might be something from this list (otherwise we should maybe add it, since it might be an "official" color https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_mac.mm:284: // kNormal? kDisabled. There's also "invalid"
On 2016/03/21 07:09:04, tapted wrote: > We can perhaps pull out the `make-gradient-in-the-style-we-want-for-macviews` > logic. Maybe that belongs in platform_style_mac.h/cc, and it can be used by > DialogButtonBorderMac and ComboboxBorderMac I factored it out so it's now NativeThemeMac::GetButtonBackgroundShader(). I looked at PlatformStyleMac but I think it's the wrong layer of abstraction for this. > Gah, I really need to update that cofaux CL at > https://codereview.chromium.org/1569113002/ -- it should be using this > make-gradient utility function too. > > https://codereview.chromium.org/1819443002/diff/1/ui/gfx/canvas.h > File ui/gfx/canvas.h (right): > > https://codereview.chromium.org/1819443002/diff/1/ui/gfx/canvas.h#newcode77 > ui/gfx/canvas.h:77: // This class represents a scoped state of a Canvas. When a > ScopedState is > ui/gfx/scoped_canvas.h? > > https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_theme.h > File ui/native_theme/native_theme.h (right): > > https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... > ui/native_theme/native_theme.h:48: kComboboxArrowBackground, > My first thought (I don't know if it's a good one or not) is to make this part > of the border. That's what's done currently for buttons. > > that could let us get away with fewer (or no?) changes to combobox.cc -- maybe > we just need to ensure isets are dealt with properly. > > https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... > File ui/native_theme/native_theme_mac.mm (right): > > https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... > ui/native_theme/native_theme_mac.mm:280: SkColor light_blue = > SkColorSetRGB(0x4e, 0x97, 0xfd); > Are there things we can pick from ui/gfx/color_palette.h? The mocks we got are > meant to be inspired by MD colors, so it might be something from this list > (otherwise we should maybe add it, since it might be an "official" color Intriguingly, I had a look at the MD color palette as published here: https://www.google.com/design/spec/style/color.html#color-color-palette and these aren't the MD colors at all, but some other strange colors. I ended up adding some constants for the actual MD color values. > https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... > ui/native_theme/native_theme_mac.mm:284: // kNormal? > kDisabled. There's also "invalid" Ah, done. I also added a disabled combobox to views_examples_with_content_exe so you can see what it looks like.
https://codereview.chromium.org/1819443002/diff/1/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/1819443002/diff/1/ui/gfx/canvas.h#newcode77 ui/gfx/canvas.h:77: // This class represents a scoped state of a Canvas. When a ScopedState is On 2016/03/21 07:09:04, tapted wrote: > ui/gfx/scoped_canvas.h? Ach! The idiom I'm used to is having these Scoped* classes as inner classes of the thing they're scoping, instead of separate classes. Oops :) Deleted this. https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_theme.h File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme.h:48: kComboboxArrowBackground, On 2016/03/21 07:09:04, tapted wrote: > My first thought (I don't know if it's a good one or not) is to make this part > of the border. That's what's done currently for buttons. > > that could let us get away with fewer (or no?) changes to combobox.cc -- maybe > we just need to ensure isets are dealt with properly. Hm. I don't really like the idea of having part of the control (the arrow button) be drawn outside/over the "border" of the control. We'd also basically need a custom "combobox mac border" class to implement this, I think. One other design wrinkle is that the border is usually painted last, but if we relied on the border to draw the arrow background, we'd need to draw the arrow after that part of the border. https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_mac.mm (right): https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_mac.mm:280: SkColor light_blue = SkColorSetRGB(0x4e, 0x97, 0xfd); On 2016/03/21 07:09:04, tapted wrote: > Are there things we can pick from ui/gfx/color_palette.h? The mocks we got are > meant to be inspired by MD colors, so it might be something from this list > (otherwise we should maybe add it, since it might be an "official" color Done. kGoogleBlue300 and kGoogleBlue700 turn out to produce a pleasing gradient. https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_mac.mm:284: // kNormal? On 2016/03/21 07:09:04, tapted wrote: > kDisabled. There's also "invalid" Done.
gah - I still want to try this out, but my laptop build just got clobbered by an xcode upgrade and a spew of "fatal error..file modified since precompiled header was built.". Anyway, let's loop in sky to get opinions on NativeTheme vs PlatformStyle. https://codereview.chromium.org/1819443002/diff/20001/ui/gfx/color_palette.h File ui/gfx/color_palette.h (right): https://codereview.chromium.org/1819443002/diff/20001/ui/gfx/color_palette.h#... ui/gfx/color_palette.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Don't forget to update the CL description. Removing the TEST= line might be a good idea too. QA use these as a signal to verify fixes for regressions and things, but this doesn't really need QA verification. https://codereview.chromium.org/1819443002/diff/20001/ui/gfx/color_palette.h#... ui/gfx/color_palette.h:36: const SkColor kMaterialGrey300 = SkColorSetRGB(0xE0, 0xE0, 0xE0); Grey->Gray? (It's probably confusing to spell Color with US-spelling and Grey with UK-spelling), even if the page at https://www.google.com/design/spec/style/color.html#color-color-palette does spell it "grey". Or maybe "grey" is more common in the US than I reali[sz]e. But, ugh, kChromeIconGrey above is already "Grey". IMO we should change that in a separate CL, unless there's already precedent for using `Grey` for colours elsewhere in Chrome. https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_mac.mm (right): https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:273: typedef SkColor ColorByState[NativeTheme::State::kNumStates]; nit: typedef const? or const below for start/end colors https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:283: // kNormal? I think this can be removed, or replaced with a comment like, // Order is Disabled, Hovered, Normal, Pressed. Although it's annoying this is a different order to views::Button::ButtonState (normal, hovered, pressed, disabled). It takes up 5 lines anyway, so we could do start_colors[...DISABLED] = ... start_colors[...HOVERED] = ... (compilers are smart these days right?) https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:285: gfx::kMaterialBlue300, I'm pretty sure we don't want hover states - this should be the same as 'Normal' https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:311: const int kComboboxBorderRadius = 5; I think we want a constant in native_theme_mac.h or platform_style[_mac].h, since it's needed for other controls too. https://codereview.chromium.org/1819443002/diff/20001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/1819443002/diff/20001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:821: gfx::ScopedCanvas scoped_canvas(canvas); This is a lot of code for something Mac-specific - I think combobox doesn't have a background set via View::set_background(..). A Background::Paint(..) override really feels like the right place for this -- something created by PlatformStyle (default implementation returns null). Background::Paint(..) takes in a views::View* -- we can DCHECK view->GetClassName() == Combobox::kViewClassName and cast it to get the arrow size. (also there's Background::CreateVerticalMultiColorGradientBackground but it probably won't handle the corners) Then, I'm uncertain where the line between the Background override and NativeTheme should be. Since it's mac-specific and not actually reaching into anything from the native OS, I think we should not modify NativeTheme. I like GetButtonBackgroundShader though. I'm just not sure if it belongs on NativeThemeMac or in platform_style_mac.h - let's ask sky
https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_theme.h File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1819443002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme.h:48: kComboboxArrowBackground, On 2016/03/21 18:53:56, Elly Jones wrote: > On 2016/03/21 07:09:04, tapted wrote: > > My first thought (I don't know if it's a good one or not) is to make this part > > of the border. That's what's done currently for buttons. > > > > that could let us get away with fewer (or no?) changes to combobox.cc -- maybe > > we just need to ensure isets are dealt with properly. > > Hm. I don't really like the idea of having part of the control (the arrow > button) be drawn outside/over the "border" of the control. We'd also basically > need a custom "combobox mac border" class to implement this, I think. One other > design wrinkle is that the border is usually painted last, but if we relied on > the border to draw the arrow background, we'd need to draw the arrow after that > part of the border. oops - missed this. Borders is largely how more complex buttons are currently done. E.g. the stuff in ui/views/controls/button/label_button_border.h, Gtk2UI::CreateNativeBorder/GtkButtonPainter/GtkButtonImageSource and ui/views/style/mac/dialog_button_border_mac.h View::OnPaint defaults to background, then border, (then back into View::Paint to paint children). If something overrides View::OnPaint (like combobox!) it has can control the order - I don't think there's a problem problem moving OnPaintBorder up (or even calling super::OnPaint). In fact, if Combobox used a child views::Label for its text the way most other controls do it would have no option but to paint its border before the text. (and yes, combobox should perhaps also use a views::Label for its text, but that's orthogonal...)
Description was changed from ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a separate UI part for combobox arrow backgrounds, and adds hooks in NativeTheme to draw them in platform-specific ways. This CL also adds a utility class gfx::Canvas::ScopedState, which encapsulates this common pattern: canvas->Save(); canvas->ClipRect(...); // or other stateful operations canvas->Restore(); BUG=543683 TEST=adhoc Build views_examples_with_content_exe and check the Combobox page. ========== to ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a Mac-specific ComboboxBackgroundMac class and a shim in PlatformStyle to allocate combobox backgrounds, which returns null on all other platforms. BUG=543683 ==========
ellyjones@chromium.org changed reviewers: + sky@chromium.org
tapted, sky: ptal? :) https://codereview.chromium.org/1819443002/diff/20001/ui/gfx/color_palette.h File ui/gfx/color_palette.h (right): https://codereview.chromium.org/1819443002/diff/20001/ui/gfx/color_palette.h#... ui/gfx/color_palette.h:36: const SkColor kMaterialGrey300 = SkColorSetRGB(0xE0, 0xE0, 0xE0); On 2016/03/22 11:20:26, tapted wrote: > Grey->Gray? (It's probably confusing to spell Color with US-spelling and Grey > with UK-spelling), even if the page at > https://www.google.com/design/spec/style/color.html#color-color-palette does > spell it "grey". Or maybe "grey" is more common in the US than I reali[sz]e. I believe it is. I would spell it as "grey", and a quick poll of my US coworkers indicates that "grey" is preferred. > But, ugh, kChromeIconGrey above is already "Grey". IMO we should change that in > a separate CL, unless there's already precedent for using `Grey` for colours > elsewhere in Chrome. I think "grey" is actually the proper name for it, even in the US. Irritatingly, "gray" is way more common in chromium :) https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_mac.mm (right): https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:273: typedef SkColor ColorByState[NativeTheme::State::kNumStates]; On 2016/03/22 11:20:26, tapted wrote: > nit: typedef const? or const below for start/end colors Acknowledged. https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:283: // kNormal? On 2016/03/22 11:20:26, tapted wrote: > I think this can be removed, or replaced with a comment like, > > // Order is Disabled, Hovered, Normal, Pressed. > > Although it's annoying this is a different order to views::Button::ButtonState > (normal, hovered, pressed, disabled). It takes up 5 lines anyway, so we could do > > start_colors[...DISABLED] = ... > start_colors[...HOVERED] = ... > > (compilers are smart these days right?) I can only do this by stripping off the const, since default initialization of const variables isn't allowed and C++ does not have designated initializers, but I like it more than the other thing, so away the const goes. https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:285: gfx::kMaterialBlue300, On 2016/03/22 11:20:26, tapted wrote: > I'm pretty sure we don't want hover states - this should be the same as 'Normal' Done. https://codereview.chromium.org/1819443002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:311: const int kComboboxBorderRadius = 5; On 2016/03/22 11:20:26, tapted wrote: > I think we want a constant in native_theme_mac.h or platform_style[_mac].h, > since it's needed for other controls too. Done. https://codereview.chromium.org/1819443002/diff/20001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/1819443002/diff/20001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:821: gfx::ScopedCanvas scoped_canvas(canvas); On 2016/03/22 11:20:26, tapted wrote: > This is a lot of code for something Mac-specific - I think combobox doesn't have > a background set via View::set_background(..). A Background::Paint(..) override > really feels like the right place for this -- something created by PlatformStyle > (default implementation returns null). > > Background::Paint(..) takes in a views::View* -- we can DCHECK > view->GetClassName() == Combobox::kViewClassName and cast it to get the arrow > size. > > (also there's Background::CreateVerticalMultiColorGradientBackground but it > probably won't handle the corners) > > Then, I'm uncertain where the line between the Background override and > NativeTheme should be. Since it's mac-specific and not actually reaching into > anything from the native OS, I think we should not modify NativeTheme. > > I like GetButtonBackgroundShader though. I'm just not sure if it belongs on > NativeThemeMac or in platform_style_mac.h - let's ask sky Done.
lgtm - just a bunch of nits Q for sky: does NativeThemeMac seem like the right place for GetButtonBackgroundShader? PlatformStyleMac is another option, but there's some TODO(beng) in background.cc like "this should be in NativeTheme", so I'm not sure whre the line should be drawn. Note this NativeTheme vs PlatformStyle also comes up in https://crrev.com/1817613002/ which draws the combobox arrow. https://codereview.chromium.org/1819443002/diff/40001/ui/gfx/color_palette.h File ui/gfx/color_palette.h (right): https://codereview.chromium.org/1819443002/diff/40001/ui/gfx/color_palette.h#... ui/gfx/color_palette.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. CL description nit: wrap to 72 chars https://codereview.chromium.org/1819443002/diff/40001/ui/native_theme/native_... File ui/native_theme/native_theme_mac.h (right): https://codereview.chromium.org/1819443002/diff/40001/ui/native_theme/native_... ui/native_theme/native_theme_mac.h:35: static skia::RefPtr<SkShader> GetButtonBackgroundShader( nit: should have a comment https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:358: // set_background takes ownership but takes a raw pointer. nit: set_background() https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:942: int Combobox::GetArrowButtonWidth() const { nit: move up so it matches declaration order https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.h (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... ui/views/controls/combobox/combobox.h:109: int GetArrowButtonWidth() const; nit: move up before overrides also comment - something to distinguish from just ArrowSize().width(), maybe // Width of the button containing the disclosure arrow (arrow size, plus padding). (or we could call drop arrow completely and call it the GetDisclosureButtonWidth(), but I was a bit confused when I first saw `disclosure`, so I'm not sure I like that) https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... File ui/views/style/mac/combobox_background_mac.h (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:12: } // namespace gfx nit: comments don't usually get put on the namespace blocks used for forward decs https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:16: class ComboboxBackgroundMac : public Background { needs a brief comment https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:18: ComboboxBackgroundMac() {} nit: these should be defined in the .cc [there's usually a clang chromium-style warning about cases where the destructor is non-trivial - technically the destructor *is* trivial, but the compiler shouldn't know that since Background's destructor is in its .cc] https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:21: void Paint(gfx::Canvas* canvas, View* view) const override; nit: // Background: https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:22: }; nit: private: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... File ui/views/style/platform_style.cc (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... ui/views/style/platform_style.cc:18: scoped_ptr<Background> PlatformStyle::CreateComboboxBackground() { nit: move below CreateCombboxBorder https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... File ui/views/style/platform_style_mac.mm (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... ui/views/style/platform_style_mac.mm:17: scoped_ptr<Background> PlatformStyle::CreateComboboxBackground() { nit: move below CreateComboboxBorder
sky@chromium.org changed reviewers: + estade@chromium.org
+estade in case he's curious about the changes to ui/gfx/color_palette.h . LGTM https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... File ui/views/style/platform_style.cc (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... ui/views/style/platform_style.cc:19: return make_scoped_ptr(nullptr); nit: return nullptr;
found time to play with this. still lgtm - just needs a visual tweak https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... File ui/views/style/mac/combobox_background_mac.cc (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.cc:21: bounds.Inset(bounds.width() - combobox->GetArrowButtonWidth(), 0, 0, 0); There's a minor visual glitch here -- see the unfocused border at http://crbug.com/543683#c6. These lines should be gfx::RectF bounds(combobox->GetLocalBounds()); bounds.Inset(bounds.width() - combobox->GetArrowButtonWidth(), 0.5, 0.5, 0.5); (plus a change below) The 0.5 is to cater for the border stroke inset (which is done so that the border stroke aligns to a single line of pixels). I think a 0.5 literal is fine (perhaps with a comment) since "half a pixel" should always work regardless of the stroke thickness (the border will just paint over it, except for that half-pixel on the outside of the rounded bits, which is causing the glitch) There may be more to do when we update the focus ring style (but for now it looks a bit funny with the glitch). https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.cc:43: fill_rect.setRectRadii(gfx::RectToSkRect(bounds), curves); this will need to be fill_rect.setRectRadii(gfx::RectFToSkRect(bounds), curves);
Description was changed from ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a Mac-specific ComboboxBackgroundMac class and a shim in PlatformStyle to allocate combobox backgrounds, which returns null on all other platforms. BUG=543683 ========== to ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a Mac-specific ComboboxBackgroundMac class and a shim in PlatformStyle to allocate combobox backgrounds, which returns null on all other platforms. BUG=543683 ==========
https://codereview.chromium.org/1819443002/diff/40001/ui/gfx/color_palette.h File ui/gfx/color_palette.h (right): https://codereview.chromium.org/1819443002/diff/40001/ui/gfx/color_palette.h#... ui/gfx/color_palette.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/22 23:04:32, tapted (OOO.travel to Apr 5th) wrote: > CL description nit: wrap to 72 chars Done. https://codereview.chromium.org/1819443002/diff/40001/ui/native_theme/native_... File ui/native_theme/native_theme_mac.h (right): https://codereview.chromium.org/1819443002/diff/40001/ui/native_theme/native_... ui/native_theme/native_theme_mac.h:35: static skia::RefPtr<SkShader> GetButtonBackgroundShader( On 2016/03/22 23:04:32, tapted (OOO.travel to Apr 5th) wrote: > nit: should have a comment Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:358: // set_background takes ownership but takes a raw pointer. On 2016/03/22 23:04:32, tapted (OOO.travel to Apr 5th) wrote: > nit: set_background() Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:942: int Combobox::GetArrowButtonWidth() const { On 2016/03/22 23:04:32, tapted (OOO.travel to Apr 5th) wrote: > nit: move up so it matches declaration order Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.h (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/controls/combo... ui/views/controls/combobox/combobox.h:109: int GetArrowButtonWidth() const; On 2016/03/22 23:04:32, tapted (OOO.travel to Apr 5th) wrote: > nit: move up before overrides > > also comment - something to distinguish from just ArrowSize().width(), maybe > > // Width of the button containing the disclosure arrow (arrow size, plus > padding). > > (or we could call drop arrow completely and call it the > GetDisclosureButtonWidth(), but I was a bit confused when I first saw > `disclosure`, so I'm not sure I like that) Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... File ui/views/style/mac/combobox_background_mac.cc (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.cc:21: bounds.Inset(bounds.width() - combobox->GetArrowButtonWidth(), 0, 0, 0); On 2016/03/23 03:08:01, tapted (OOO.travel to Apr 5th) wrote: > There's a minor visual glitch here -- see the unfocused border at > http://crbug.com/543683#c6. These lines should be > > gfx::RectF bounds(combobox->GetLocalBounds()); > bounds.Inset(bounds.width() - combobox->GetArrowButtonWidth(), 0.5, 0.5, 0.5); > > (plus a change below) > > The 0.5 is to cater for the border stroke inset (which is done so that the > border stroke aligns to a single line of pixels). I think a 0.5 literal is fine > (perhaps with a comment) since "half a pixel" should always work regardless of > the stroke thickness (the border will just paint over it, except for that > half-pixel on the outside of the rounded bits, which is causing the glitch) > > There may be more to do when we update the focus ring style (but for now it > looks a bit funny with the glitch). Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.cc:43: fill_rect.setRectRadii(gfx::RectToSkRect(bounds), curves); On 2016/03/23 03:08:01, tapted (OOO.travel to Apr 5th) wrote: > this will need to be > > fill_rect.setRectRadii(gfx::RectFToSkRect(bounds), curves); Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... File ui/views/style/mac/combobox_background_mac.h (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:12: } // namespace gfx On 2016/03/22 23:04:33, tapted (OOO.travel to Apr 5th) wrote: > nit: comments don't usually get put on the namespace blocks used for forward > decs Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:16: class ComboboxBackgroundMac : public Background { On 2016/03/22 23:04:32, tapted (OOO.travel to Apr 5th) wrote: > needs a brief comment Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:18: ComboboxBackgroundMac() {} On 2016/03/22 23:04:32, tapted (OOO.travel to Apr 5th) wrote: > nit: these should be defined in the .cc [there's usually a clang chromium-style > warning about cases where the destructor is non-trivial - technically the > destructor *is* trivial, but the compiler shouldn't know that since Background's > destructor is in its .cc] Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:21: void Paint(gfx::Canvas* canvas, View* view) const override; On 2016/03/22 23:04:33, tapted (OOO.travel to Apr 5th) wrote: > nit: // Background: Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/mac/comb... ui/views/style/mac/combobox_background_mac.h:22: }; On 2016/03/22 23:04:32, tapted (OOO.travel to Apr 5th) wrote: > nit: private: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... File ui/views/style/platform_style.cc (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... ui/views/style/platform_style.cc:18: scoped_ptr<Background> PlatformStyle::CreateComboboxBackground() { On 2016/03/22 23:04:33, tapted (OOO.travel to Apr 5th) wrote: > nit: move below CreateCombboxBorder Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... ui/views/style/platform_style.cc:19: return make_scoped_ptr(nullptr); On 2016/03/22 23:54:30, sky wrote: > nit: return nullptr; Done. https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... File ui/views/style/platform_style_mac.mm (right): https://codereview.chromium.org/1819443002/diff/40001/ui/views/style/platform... ui/views/style/platform_style_mac.mm:17: scoped_ptr<Background> PlatformStyle::CreateComboboxBackground() { On 2016/03/22 23:04:33, tapted (OOO.travel to Apr 5th) wrote: > nit: move below CreateComboboxBorder Done.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1819443002/#ps60001 (title: "final fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819443002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819443002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1819443002/#ps80001 (title: "fix linux build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819443002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819443002/80001
https://codereview.chromium.org/1819443002/diff/80001/ui/gfx/color_palette.h File ui/gfx/color_palette.h (right): https://codereview.chromium.org/1819443002/diff/80001/ui/gfx/color_palette.h#... ui/gfx/color_palette.h:34: const SkColor kMaterialBlue700 = SkColorSetRGB(0x19, 0x76, 0xD2); I'm kinda surprised we wouldn't want to use GoogleBlue everywhere. Are you sure the designers are synced up on this decision? sgabriel@ has been the one defining colors for us on cros/windows/linux material design.
https://codereview.chromium.org/1819443002/diff/80001/ui/gfx/color_palette.h File ui/gfx/color_palette.h (right): https://codereview.chromium.org/1819443002/diff/80001/ui/gfx/color_palette.h#... ui/gfx/color_palette.h:34: const SkColor kMaterialBlue700 = SkColorSetRGB(0x19, 0x76, 0xD2); On 2016/03/23 18:50:26, Evan Stade wrote: > I'm kinda surprised we wouldn't want to use GoogleBlue everywhere. Are you sure > the designers are synced up on this decision? sgabriel@ has been the one > defining colors for us on cros/windows/linux material design. I just chatted with sgabriel@ about this, and they pointed me at the palette that defines the kGoogle* colors; I'll land a followup CL to remove these and switch over to using the kGoogle* ones (and fix the comment above them so it doesn't mislead anyone else into doing this).
Message was sent while issue was closed.
Description was changed from ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a Mac-specific ComboboxBackgroundMac class and a shim in PlatformStyle to allocate combobox backgrounds, which returns null on all other platforms. BUG=543683 ========== to ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a Mac-specific ComboboxBackgroundMac class and a shim in PlatformStyle to allocate combobox backgrounds, which returns null on all other platforms. BUG=543683 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a Mac-specific ComboboxBackgroundMac class and a shim in PlatformStyle to allocate combobox backgrounds, which returns null on all other platforms. BUG=543683 ========== to ========== MacViews: draw combobox arrow backgrounds Combobox arrow backgrounds on Mac are a different color from the combobox text background. This CL introduces a Mac-specific ComboboxBackgroundMac class and a shim in PlatformStyle to allocate combobox backgrounds, which returns null on all other platforms. BUG=543683 Committed: https://crrev.com/bbfac0443569e1049214e2904524a31ddc16f951 Cr-Commit-Position: refs/heads/master@{#382910} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bbfac0443569e1049214e2904524a31ddc16f951 Cr-Commit-Position: refs/heads/master@{#382910} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
