Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
120823003:
[Views] Only paint background of text portion of LabelButton in inverted color theme. (Closed)
|
Created:
6 years, 11 months ago by Greg Billock Modified:
6 years, 11 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Views] Only paint background of text portion of LabelButton in inverted color theme.
R=msw@chromium.org
BUG=330067
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243402
Patch Set 1 #
Total comments: 4
Patch Set 2 : label_ #
Total comments: 7
Patch Set 3 : Put back buttons on theme change #Patch Set 4 : . #
Total comments: 2
Patch Set 5 : . #
Messages
Total messages: 16 (0 generated)
LGTM with nits; thanks! https://codereview.chromium.org/120823003/diff/1/ui/views/controls/button/lab... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/1/ui/views/controls/button/lab... ui/views/controls/button/label_button.cc:282: canvas->FillRect(rect, label()->background_color()); nit: s/label()/label_/; inline label_->bounds(); drop the curly braces. https://codereview.chromium.org/120823003/diff/1/ui/views/controls/button/lab... ui/views/controls/button/label_button.cc:307: params->button.background_color = label()->background_color(); nit: s/label()/label_/ here for consistency.
+sky for owners https://codereview.chromium.org/120823003/diff/1/ui/views/controls/button/lab... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/1/ui/views/controls/button/lab... ui/views/controls/button/label_button.cc:282: canvas->FillRect(rect, label()->background_color()); On 2014/01/03 00:16:39, msw wrote: > nit: s/label()/label_/; inline label_->bounds(); drop the curly braces. Done. https://codereview.chromium.org/120823003/diff/1/ui/views/controls/button/lab... ui/views/controls/button/label_button.cc:307: params->button.background_color = label()->background_color(); On 2014/01/03 00:16:39, msw wrote: > nit: s/label()/label_/ here for consistency. Done.
https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... ui/views/controls/button/label_button.cc:280: if (gfx::IsInvertedColorScheme()) This ends up being after the border is painted. Don't we want this in LabelButton::OnPaintBackground? Or possibly set as a Background?
https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... ui/views/controls/button/label_button.cc:280: if (gfx::IsInvertedColorScheme()) On 2014/01/06 20:20:37, sky wrote: > This ends up being after the border is painted. Don't we want this in > LabelButton::OnPaintBackground? Or possibly set as a Background? I placed this here to try and mimic the sequencing from where it was in the LabelButtonBorder paint order. Having it as a background would paint before the border. That's possibly better; I was aiming for conservativeness in this patch, and also thought that since this isn't edge-to-edge it might be unexpected in either a background or a border. Accomplishing the goal by setting something on the label_ itself might be an answer, but that's a bit more invasive...
https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... ui/views/controls/button/label_button.cc:280: if (gfx::IsInvertedColorScheme()) On 2014/01/06 21:13:44, Greg Billock wrote: > On 2014/01/06 20:20:37, sky wrote: > > This ends up being after the border is painted. Don't we want this in > > LabelButton::OnPaintBackground? Or possibly set as a Background? > > I placed this here to try and mimic the sequencing from where it was in the > LabelButtonBorder paint order. Having it as a background would paint before the > border. That's possibly better; I was aiming for conservativeness in this patch, > and also thought that since this isn't edge-to-edge it might be unexpected in > either a background or a border. Accomplishing the goal by setting something on > the label_ itself might be an answer, but that's a bit more invasive... Do label_->set_background(CreateSolidBackground(SK_ColorBLACK)) on line 329.
https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... ui/views/controls/button/label_button.cc:280: if (gfx::IsInvertedColorScheme()) On 2014/01/06 21:50:57, msw wrote: > On 2014/01/06 21:13:44, Greg Billock wrote: > > On 2014/01/06 20:20:37, sky wrote: > > > This ends up being after the border is painted. Don't we want this in > > > LabelButton::OnPaintBackground? Or possibly set as a Background? > > > > I placed this here to try and mimic the sequencing from where it was in the > > LabelButtonBorder paint order. Having it as a background would paint before > the > > border. That's possibly better; I was aiming for conservativeness in this > patch, > > and also thought that since this isn't edge-to-edge it might be unexpected in > > either a background or a border. Accomplishing the goal by setting something > on > > the label_ itself might be an answer, but that's a bit more invasive... > > Do label_->set_background(CreateSolidBackground(SK_ColorBLACK)) on line 329. That's true, we're kind of already pushing the label around in this case. Is that going to mess anything up? If not, let's just do that.
https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... ui/views/controls/button/label_button.cc:280: if (gfx::IsInvertedColorScheme()) On 2014/01/06 22:14:46, Greg Billock wrote: > On 2014/01/06 21:50:57, msw wrote: > > On 2014/01/06 21:13:44, Greg Billock wrote: > > > On 2014/01/06 20:20:37, sky wrote: > > > > This ends up being after the border is painted. Don't we want this in > > > > LabelButton::OnPaintBackground? Or possibly set as a Background? > > > > > > I placed this here to try and mimic the sequencing from where it was in the > > > LabelButtonBorder paint order. Having it as a background would paint before > > the > > > border. That's possibly better; I was aiming for conservativeness in this > > patch, > > > and also thought that since this isn't edge-to-edge it might be unexpected > in > > > either a background or a border. Accomplishing the goal by setting something > > on > > > the label_ itself might be an answer, but that's a bit more invasive... > > > > Do label_->set_background(CreateSolidBackground(SK_ColorBLACK)) on line 329. > > That's true, we're kind of already pushing the label around in this case. Is > that going to mess anything up? If not, let's just do that. Neither Label nor LabelButton seem to set any background on this label view, so that suggestion should work okay. My only other suggestion is to set_background(NULL) (clear any black background) when ResetColorsFromNativeTheme is called and gfx::IsInvertedColorScheme() is false.
https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... ui/views/controls/button/label_button.cc:280: if (gfx::IsInvertedColorScheme()) On 2014/01/06 22:34:55, msw wrote: > On 2014/01/06 22:14:46, Greg Billock wrote: > > On 2014/01/06 21:50:57, msw wrote: > > > On 2014/01/06 21:13:44, Greg Billock wrote: > > > > On 2014/01/06 20:20:37, sky wrote: > > > > > This ends up being after the border is painted. Don't we want this in > > > > > LabelButton::OnPaintBackground? Or possibly set as a Background? > > > > > > > > I placed this here to try and mimic the sequencing from where it was in > the > > > > LabelButtonBorder paint order. Having it as a background would paint > before > > > the > > > > border. That's possibly better; I was aiming for conservativeness in this > > > patch, > > > > and also thought that since this isn't edge-to-edge it might be unexpected > > in > > > > either a background or a border. Accomplishing the goal by setting > something > > > on > > > > the label_ itself might be an answer, but that's a bit more invasive... > > > > > > Do label_->set_background(CreateSolidBackground(SK_ColorBLACK)) on line 329. > > > > That's true, we're kind of already pushing the label around in this case. Is > > that going to mess anything up? If not, let's just do that. > > Neither Label nor LabelButton seem to set any background on this label view, so > that suggestion should work okay. My only other suggestion is to > set_background(NULL) (clear any black background) when > ResetColorsFromNativeTheme is called and gfx::IsInvertedColorScheme() is false. ok. This seems to work fine.* * There are still a couple oddities. The omnibox in HC theme is editing strangely -- doesn't look like it repaints background after theme change. Same thing looks like it goes for buttons -- they aren't repainting successfully, but closing and opening the browser succeeds. I'm guessing this may be lower down in the stack?
The latest patch set LGTM with a nit. https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/60001/ui/views/controls/button... ui/views/controls/button/label_button.cc:280: if (gfx::IsInvertedColorScheme()) On 2014/01/06 23:51:02, Greg Billock wrote: > On 2014/01/06 22:34:55, msw wrote: > > On 2014/01/06 22:14:46, Greg Billock wrote: > > > On 2014/01/06 21:50:57, msw wrote: > > > > On 2014/01/06 21:13:44, Greg Billock wrote: > > > > > On 2014/01/06 20:20:37, sky wrote: > > > > > > This ends up being after the border is painted. Don't we want this in > > > > > > LabelButton::OnPaintBackground? Or possibly set as a Background? > > > > > > > > > > I placed this here to try and mimic the sequencing from where it was in > > the > > > > > LabelButtonBorder paint order. Having it as a background would paint > > before > > > > the > > > > > border. That's possibly better; I was aiming for conservativeness in > this > > > > patch, > > > > > and also thought that since this isn't edge-to-edge it might be > unexpected > > > in > > > > > either a background or a border. Accomplishing the goal by setting > > something > > > > on > > > > > the label_ itself might be an answer, but that's a bit more invasive... > > > > > > > > Do label_->set_background(CreateSolidBackground(SK_ColorBLACK)) on line > 329. > > > > > > That's true, we're kind of already pushing the label around in this case. Is > > > that going to mess anything up? If not, let's just do that. > > > > Neither Label nor LabelButton seem to set any background on this label view, > so > > that suggestion should work okay. My only other suggestion is to > > set_background(NULL) (clear any black background) when > > ResetColorsFromNativeTheme is called and gfx::IsInvertedColorScheme() is > false. > > ok. This seems to work fine.* > > * There are still a couple oddities. The omnibox in HC theme is editing > strangely -- doesn't look like it repaints background after theme change. Same > thing looks like it goes for buttons -- they aren't repainting successfully, but > closing and opening the browser succeeds. I'm guessing this may be lower down in > the stack? Yeah, I think these are long-standing issues; many views need to similarly ResetColorsFromNativeTheme on theme changes and aren't doing so. As long as everything looks correct when re-launching the browser after a theme change, it'll be acceptable for now. https://codereview.chromium.org/120823003/diff/160001/ui/views/controls/butto... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/160001/ui/views/controls/butto... ui/views/controls/button/label_button.cc:280: nit: remove blank line
LGTM
https://codereview.chromium.org/120823003/diff/160001/ui/views/controls/butto... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/120823003/diff/160001/ui/views/controls/butto... ui/views/controls/button/label_button.cc:280: On 2014/01/07 00:00:54, msw wrote: > nit: remove blank line Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/120823003/210001
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/120823003/210001
Message was sent while issue was closed.
Change committed as 243402 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
