Bug 28108 - Tint of checkboxes/radio buttons/etc. not correctly updated on Chromium/Mac
Summary: Tint of checkboxes/radio buttons/etc. not correctly updated on Chromium/Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-08 08:49 PDT by Viet-Trung Luu
Modified: 2009-08-12 09:01 PDT (History)
8 users (show)

See Also:


Attachments
Fixes bug 28108 (control tints on Chromium/Mac) (5.76 KB, patch)
2009-08-08 08:52 PDT, Viet-Trung Luu
no flags Details | Formatted Diff | Diff
Updated patch (as per Avi's comments) (5.79 KB, patch)
2009-08-10 13:40 PDT, Viet-Trung Luu
no flags Details | Formatted Diff | Diff
Updated patch (4.29 KB, patch)
2009-08-10 15:04 PDT, Viet-Trung Luu
no flags Details | Formatted Diff | Diff
Now with a beefier comment. (4.56 KB, patch)
2009-08-10 15:44 PDT, Viet-Trung Luu
no flags Details | Formatted Diff | Diff
Updated with Jeremy's suggested comment (more or less). (4.79 KB, patch)
2009-08-11 16:38 PDT, Viet-Trung Luu
dglazkov: review-
Details | Formatted Diff | Diff
Changelog entry back, style fix. (6.11 KB, patch)
2009-08-12 08:48 PDT, Viet-Trung Luu
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viet-Trung Luu 2009-08-08 08:49:40 PDT
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.
Comment 1 Viet-Trung Luu 2009-08-08 08:52:27 PDT
Created attachment 34367 [details]
Fixes bug 28108 (control tints on Chromium/Mac)
Comment 2 Nico Weber 2009-08-08 09:04:05 PDT
Does this work correctly if you set "appearance" to graphite in sysprefs->appearance?
Comment 3 Dimitri Glazkov (Google) 2009-08-08 09:04:56 PDT
Jeremy, could you take a look? I don't even know what I am looking at here :)
Comment 4 John Grabowski 2009-08-10 12:59:10 PDT
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)
Comment 5 Viet-Trung Luu 2009-08-10 13:04:10 PDT
(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).
Comment 6 Avi Drissman 2009-08-10 13:08:51 PDT
(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.
Comment 7 Viet-Trung Luu 2009-08-10 13:40:05 PDT
Created attachment 34510 [details]
Updated patch (as per Avi's comments)
Comment 8 Viet-Trung Luu 2009-08-10 13:44:02 PDT
(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.
Comment 9 Jeremy Moskovich 2009-08-10 13:47:33 PDT
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.
Comment 10 Viet-Trung Luu 2009-08-10 14:10:16 PDT
Let me see if I can make things a lot nicer. Hang on a sec....
Comment 11 Viet-Trung Luu 2009-08-10 15:04:45 PDT
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.
Comment 12 Jeremy Moskovich 2009-08-10 15:13:41 PDT
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.
Comment 13 Viet-Trung Luu 2009-08-10 15:18:53 PDT
(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).
Comment 14 Viet-Trung Luu 2009-08-10 15:21:56 PDT
(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.... :-)
Comment 15 Jeremy Moskovich 2009-08-10 15:27:44 PDT
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.
Comment 16 Viet-Trung Luu 2009-08-10 15:44:14 PDT
Created attachment 34523 [details]
Now with a beefier comment.

Maybe it's time to start writing comments in WebKit.... :-)
Comment 17 Jeremy Moskovich 2009-08-10 15:52:52 PDT
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.
Comment 18 Viet-Trung Luu 2009-08-11 16:38:20 PDT
Created attachment 34612 [details]
Updated with Jeremy's suggested comment (more or less).
Comment 19 Jeremy Moskovich 2009-08-11 16:42:48 PDT
Looks Good
Comment 20 Dimitri Glazkov (Google) 2009-08-12 08:38:28 PDT
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.
Comment 21 Viet-Trung Luu 2009-08-12 08:48:55 PDT
Created attachment 34664 [details]
Changelog entry back, style fix.

Ack! -- the directory from which you run svn-create-patch actually matters.
Comment 22 Dimitri Glazkov (Google) 2009-08-12 08:59:01 PDT
Comment on attachment 34664 [details]
Changelog entry back, style fix.

r=me, based on Mac-ies' comments above.
Comment 23 Dimitri Glazkov (Google) 2009-08-12 09:01:10 PDT
Comment on attachment 34664 [details]
Changelog entry back, style fix.

r=me, based on Mac-ies' comments above.
Comment 24 Dimitri Glazkov (Google) 2009-08-12 09:01:34 PDT
Landed as http://trac.webkit.org/changeset/47108.