Bug 26691 – Cleanup: Move focusRingColor to RenderTheme

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jeremy Moskovich]

focusRingColor() should really live in RenderTheme.

Currently each port defines focusRingColor() in a different place.

[Jeremy Moskovich]

Created attachment 31808 [details]
patch 1

This CL is larger than I'd like but most of it is just removal of old code.

The behavior of several ports changes with this CL, full details are in the attached changelog.

[Jeremy Moskovich]

Created attachment 31816 [details]
patch 2

Fixup style issues, change c -> color in a couple of places.

[Jeremy Moskovich]

Created attachment 31945 [details]
patch 3

Use ChromiumBridge::layoutTestMode() in RenderThemeChromiumMac which is more correct.

This change removes one of the roadblocks to turning on Pixel tests for Chromium on OSX.

[Eric Seidel (no email)]

Comment on attachment 31945 [details]
patch 3

Style error:
 175 Color RenderThemeChromiumMac::focusRingColor() const {

I worry that we're no longer caching the focus ring color.
 425             color = convertNSColorToColor([NSColor keyboardFocusIndicatorColor]);
could be expensive to call every time if it goes down the "allocate a bitmap" case.

Also, are you sure that keyboardFocusIndicatorColor will respect focus ring changes?  i.e. have you tested changing the focus ring color from System Prefernces while Safari is open and verified that the focus ring colors still update?  (Actually they probably don't update until they're redrawn.)

I take it nsColor(color) is no longer used?

In general this looks fine.  A PLT run would tell us if the focus ring color caching has caused a regression.

r- for the possible performance regression.  We need to investigate that keyboardFocusIndicatorColor properly updates on focus color change in the Appearance control panel.

[Jeremy Moskovich]

Thanks for looking at this Eric:

* RenderTheme already has a cache which we make use of.
* nsColor() is still used in accessibility/mac/AccessibilityObjectWrapper.mm
* Empirically NSSystemColorsDidChangeNotification is fired when the focusRingColor changes so it doesn't look like we need to add a notification (I did test this).

New patch with style fix will follow shortly...

[Jeremy Moskovich]

Created attachment 31949 [details]
patch 4

* Fix style issues.
* Added a manual test to make sure we update the focus ring color correctly when the OS colors change.

[Eric Seidel (no email)]

Comment on attachment 31949 [details]
patch 4

Fantastic!  Please fix your email address in your ChangeLogs.

You might want to set EMAIL_ADDRESS to your real email to prevent this in the future.  see prepare-ChangeLog for more information

[Brent Fulgham]

This patch looks as though it will break the behavior under Windows Cairo.  If this patch is non landed by Monday, I'll try landing it on my Cairo Windows box, confirm any break, and propose a patch to correct it under this issue.

[Brent Fulgham]

(In reply to comment #8)
> This patch looks as though it will break the behavior under Windows Cairo.  If
> this patch is non landed by Monday, I'll try landing it on my Cairo Windows
> box, confirm any break, and propose a patch to correct it under this issue.
> 
Just replying to myself, it seems like RenderThemeWin needs to provide the correct focus ring color; otherwise we will end up with Black on Windows (Cairo backend) according to the current proposed patch.

[Jeremy Moskovich]

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

[Jeremy Moskovich]

Hi Brent,
I landed this patch yesterday.

I tried to be careful about breaking other ports and cc-ed everyone who seemed relevant, and got no response.

I did check with Hyatt before starting on this patch and he seemed to think the current behavior was wrong and that Cairo in itself should not dictate the focus ring color (as was the case before this patch, as far as I can understand) but this should be set per-platform in the relevant RenderTheme*

Overriding focusRingColor in RenderThemeWin to restore the previous behavior should be straightforward, I would have done it in this patch except for the above.

[Brent Fulgham]

(In reply to comment #11)
> I tried to be careful about breaking other ports and cc-ed everyone who seemed
> relevant, and got no response.

Sorry about that; I was on vacation and didn't notice this patch. I would have thought
the Gtk guys might have commented but didn't see anything.
 
> I did check with Hyatt before starting on this patch and he seemed to think the
> current behavior was wrong and that Cairo in itself should not dictate the
> focus ring color (as was the case before this patch, as far as I can tell.

Oh, definitely. Your patch is way better than the status quo. The current Windows/Gtk
Cairo behavior was an old kludge to get things working and has long been in
need of some refactoring.

I'll give it a shot on Monday and propose a patch if it seems to be working
strangely.  
> understand) but this should be set per-platform in the relevant RenderTheme*
> 
> Overriding focusRingColor in RenderThemeWin to restore the previous behavior
> should be straightforward, I would have done it in this patch except for the
> above.
>