Bug 28108 – Tint of checkboxes/radio buttons/etc. not correctly updated on Chromium/Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Viet-Trung Luu]

Original bug: <http://crbug.com/18199>. The tints of controls in web content don't reflect the "(in)active state" of the window, remaining blue all the time. To check:

1. On Chromium/Mac, go to a webpage with such a control (including this one).
2. Select another window/application (e.g., Finder desktop) in such a way that you can still see the webpage.
3. Notice that the blue parts of the controls don't go grey.

[Viet-Trung Luu]

Created attachment 34367 [details]
Fixes bug 28108 (control tints on Chromium/Mac)

[Nico Weber]

Does this work correctly if you set "appearance" to graphite in sysprefs->appearance?

[Dimitri Glazkov (Google)]

Jeremy, could you take a look? I don't even know what I am looking at here :)

[John Grabowski]

2 comments.  
1) Can you confirm we get a repaint request (to cause retinting) when Chromium loses/gains focus?  
2) The explicit tinting by color/NSControlTint seems a bit specific.  Isn't there a better way to do this (e.g. setDrawingModeForFocusState: or something like that)

[Viet-Trung Luu]

(In reply to comment #2)
> Does this work correctly if you set "appearance" to graphite in
> sysprefs->appearance?

Tried it just now (must remember to add myself to the CC list).

But, shockingly, yes it works (despite the name of the Cocoa constant).

[Avi Drissman]

(In reply to comment #4)
> 2) The explicit tinting by color/NSControlTint seems a bit specific.  Isn't
> there a better way to do this (e.g. setDrawingModeForFocusState: or something
> like that)

John:

Apparently the answer to drawing active or not _is_ to use the tinting... see
http://developer.apple.com/documentation/Cocoa/Conceptual/ControlCell/SystemTintAware.html
for details.

But you can't hard-code NSBlueControlTint. Bad idea. Use [NSColor
currentControlTint] instead.

[Viet-Trung Luu]

Created attachment 34510 [details]
Updated patch (as per Avi's comments)

[Viet-Trung Luu]

(In reply to comment #4)
> 2 comments.  
> 1) Can you confirm we get a repaint request (to cause retinting) when Chromium
> loses/gains focus?  
> 2) The explicit tinting by color/NSControlTint seems a bit specific.  Isn't
> there a better way to do this (e.g. setDrawingModeForFocusState: or something
> like that)

I think Avi answered 1) on #chromium, but I can also confirm it: when we call SetActive() or whatever it is (which we do on losing/gaining focus), this causes a repaint.

Note: We should also catch changes in the system control tint, and update immediately (so you get instant gratification when using System Preferences/Appearances/...) -- Safari does (somewhat surprisingly). I'll file a Chromium bug on this, though it's pretty low-priority.

[Jeremy Moskovich]

Comment on attachment 34510 [details]
Updated patch (as per Avi's comments)

I'm not a reviewer, but here are my comments per dglazkov's request:

Could you add a more complete explanation here including answers to the following questions and anything else someone looking at this code further down the road:
* Why do we need this when Safari doesn't?
* Are controls repainted when in background?
* Comment with a summary of Avi's note.

> +static void updateNSCellControlTint(NSCell* cell, RenderObject* o) {
> +    if (o->document()->frame() && o->document()->frame()->page() &&
> +            o->document()->frame()->page()->focusController()) {
Could you split this whole check into a temp variable and then text that variable, focusControllerIsValid or somesuch.

> +        FocusController* focusCtlr =
> +                o->document()->frame()->page()->focusController();
> +        [cell setControlTint:(focusCtlr->isActive() ?
> +                                  [NSColor currentControlTint] :
> +                                  NSClearControlTint)];
Again temp variable and lose the ?:, should make this a little easier to read.

[Viet-Trung Luu]

Let me see if I can make things a lot nicer. Hang on a sec....

[Viet-Trung Luu]

Created attachment 34520 [details]
Updated patch

I realized that RenderTheme has an isActive() method. I also made things more consistent with the other updateFooState() methods.

[Jeremy Moskovich]

Comment on attachment 34520 [details]
Updated patch

> +// Updates the control tint of |cell| (with information from |o|).
Still missing more thorough function doc as mentioned in previous comment.
> +void RenderThemeChromiumMac::updateActiveState(NSCell* cell, const RenderObject* o) {

Other than that looks good, unless Avi or John have any objections, w/more thorough documentation I'm fine with r+ing.

[Viet-Trung Luu]

(In reply to comment #9)
> Could you add a more complete explanation here including answers to the
> following questions and anything else someone looking at this code further down
> the road:
> * Why do we need this when Safari doesn't?

When we draw our controls using Cocoa, we feed them inView:nil. The code in RenderThemeMac.mm (most similar to ours) usually feeds inView:o->view()->frameView()->documentView(), i.e., a real NSView, from which the control can determine the "active" state. The current code in
RenderThemeSafari.cpp checks isActive() in its determineState().

[In the latest version of the patch, I use isActive(), which does what I previously did manually.]

> * Are controls repainted when in background?

They are repainted whenever the FocusController's setActive() is called and this actually results in a changed state (setActive() checks). We only call down to setActive() when necessary, for the foreground view.

> * Comment with a summary of Avi's note.
> 
> > +static void updateNSCellControlTint(NSCell* cell, RenderObject* o) {
> > +    if (o->document()->frame() && o->document()->frame()->page() &&
> > +            o->document()->frame()->page()->focusController()) {
> Could you split this whole check into a temp variable and then text that
> variable, focusControllerIsValid or somesuch.

This is gone, i.e., now inside the pre-existing RenderTheme::isActive().

> > +        FocusController* focusCtlr =
> > +                o->document()->frame()->page()->focusController();
> > +        [cell setControlTint:(focusCtlr->isActive() ?
> > +                                  [NSColor currentControlTint] :
> > +                                  NSClearControlTint)];
> Again temp variable and lose the ?:, should make this a little easier to read.

Please take a look at the current patch (it still has a ?:, but in a hopefully-more-reasonable context). I also lined things up with the current updateFooState()'s.

Note that since all NSCells have an "active state"/control tint, I set said state for everything. If one is very confident, one could remove the setting of the active state for various controls (but I prefer to play it safe).

[Viet-Trung Luu]

(In reply to comment #12)
> (From update of attachment 34520 [details])
> > +// Updates the control tint of |cell| (with information from |o|).
> Still missing more thorough function doc as mentioned in previous comment.
> > +void RenderThemeChromiumMac::updateActiveState(NSCell* cell, const RenderObject* o) {

Thanks for reviewing it. I can provide more thorough documentation if you want, though my terse comment is much more than what you find for other functions in WebKit.... :-)

[Jeremy Moskovich]

Again, I'm not a WebKit reviewer, but I'd prefer this to have better documentation, to make things easier for the next person who has to poke around this file.

[Viet-Trung Luu]

Created attachment 34523 [details]
Now with a beefier comment.

Maybe it's time to start writing comments in WebKit.... :-)

[Jeremy Moskovich]

Comment on attachment 34523 [details]
Now with a beefier comment.

> +// Updates the control tint (a.k.a. active state) of |cell| (depending on
> +// |RenderTheme::isActive(o)|). This should be called before drawing any
> +// NSCell-derived control, unless you are sure that it isn't needed; in any
> +// case, it won't do any harm. We need to set the control tint explicitly since
> +// we do not give |cell| a parent NSView.

How about something like:
In the Chromium port, the renderer runs as a background process and the control's NScells lack a parent NSView. Therefore controls don't have their tint color updated correctly when the Application is activated/deactivated.
FocusController's setActive() is called when the application is activated/deactivated which causes a repaint at which time this code is called.
This function should be called before drawing any NSCell-derived controls, unless you're sure it isn't needed.

[Viet-Trung Luu]

Created attachment 34612 [details]
Updated with Jeremy's suggested comment (more or less).

[Jeremy Moskovich]

Looks Good

[Dimitri Glazkov (Google)]

Comment on attachment 34612 [details]
Updated with Jeremy's suggested comment (more or less).

From the style patrol:

Needs a ChangeLog entry.

> +void RenderThemeChromiumMac::updateActiveState(NSCell* cell, const RenderObject* o) {

Opening brace on new line.

[Viet-Trung Luu]

Created attachment 34664 [details]
Changelog entry back, style fix.

Ack! -- the directory from which you run svn-create-patch actually matters.

[Dimitri Glazkov (Google)]

Comment on attachment 34664 [details]
Changelog entry back, style fix.

r=me, based on Mac-ies' comments above.

[Dimitri Glazkov (Google)]

Comment on attachment 34664 [details]
Changelog entry back, style fix.

r=me, based on Mac-ies' comments above.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/47108.