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.
Created attachment 34367 [details] Fixes bug 28108 (control tints on Chromium/Mac)
Does this work correctly if you set "appearance" to graphite in sysprefs->appearance?
Jeremy, could you take a look? I don't even know what I am looking at here :)
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)
(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).
(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.
Created attachment 34510 [details] Updated patch (as per Avi's comments)
(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 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.
Let me see if I can make things a lot nicer. Hang on a sec....
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 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.
(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).
(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.... :-)
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.
Created attachment 34523 [details] Now with a beefier comment. Maybe it's time to start writing comments in WebKit.... :-)
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.
Created attachment 34612 [details] Updated with Jeremy's suggested comment (more or less).
Looks Good
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.
Created attachment 34664 [details] Changelog entry back, style fix. Ack! -- the directory from which you run svn-create-patch actually matters.
Comment on attachment 34664 [details] Changelog entry back, style fix. r=me, based on Mac-ies' comments above.
Landed as http://trac.webkit.org/changeset/47108.