Bug 26821 – REGRESSION(r45285): focus rings are black on windows safari

• Component: CSS
• Status: RESOLVED FIXED

[Alice Liu]

due to http://trac.webkit.org/changeset/45285, focus rings are now black on windows safari instead of aqua.

[Alice Liu]

<rdar://problem/7018252>

[Mark Rowe (bdash)]

*** Bug 26822 has been marked as a duplicate of this bug. ***

[Mark Rowe (bdash)]

*** Bug 26823 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Seems easy enough to fix.  RenderThemeWin or RenderThemeSafari just need a focusRingColor method if they don't have one.

[Alice Liu]

It looks like the focusRingColor needs to stay in RenderThemeSafari to make pixel tests on Windows pass, and we also need focusRingColor to be added to RenderThemeWin to address this bug.  Do we have an alternative to duplicating RenderThemeSafari::focusRingColor into RenderThemeWin?

[Alice Liu]

Created attachment 32094 [details]
patch - just duplicates what is in RenderThemeSafari

[Eric Seidel (no email)]

Comment on attachment 32094 [details]
patch - just duplicates what is in RenderThemeSafari

Shouldn't SafariThemeWin use whatever the system theme focus color is?

[Adam Roben (:aroben)]

Comment on attachment 32094 [details]
patch - just duplicates what is in RenderThemeSafari

I agree with Eric that RenderThemeWin should probably just use the system focus ring color. RenderThemeWin has no knowledge of SafariTheme currently, and that's a good thing.

I can think of a few options for getting the behavior we want in Safari:

1) Add a preference that says "use RenderThemeSafari for the focus ring color even though we use RenderThemeWin for everything else". Safari would set this preference when it launches.
2) Add a call to set a custom focus ring color. Safari would call this, passing the focus ring color from SafariTheme.

I'm not sure which I think is better. I think I have a slight preference for (2).

What do you think, Alice?

[Adam Roben (:aroben)]

Comment on attachment 32094 [details]
patch - just duplicates what is in RenderThemeSafari

I guess I'll r- this for now while we work out what we want the solution to look like.

[Eric Seidel (no email)]

There already is code to support a custom focus ring color.  At least for Chromium tests.  So option #2 should already work.  Chromium sets a custom focus ring color when running the layout tests in order to match Safari results, iirc.

[Alice Liu]

(2) (In reply to comment #8)
> 2) Add a call to set a custom focus ring color. Safari would call this, passing
> the focus ring color from SafariTheme.

so this has to run through RenderThemeWin::focusRingColor, doesn't it?
When the focusRingColor is asked for in CSSSylteSelector::getColorFromPrimitiveValue(), as of r45285, the defaultTheme is asked to provide this color. 

as (2) is described i'm confused how RenderThemeWin::focusRingColor will play its role.  

[Eric Seidel (no email)]

I would think we could make Chromium's "custom focus ring color" implementation more general to cover this case:

5	Color RenderThemeChromiumMac::focusRingColor() const 
 	176	{ 
 	177	    if (ChromiumBridge::layoutTestMode()) 
 	178	        return oldAquaFocusRingColor(); 
 	179	 
 	180	    return systemColor(CSSValueWebkitFocusRingColor); 
 	181	} 

But maybe I'm over-engineering the problem here.  Chromium could certainly change to call a special "set custom focus ring color" perference instead of having a ChromiumBridge method to check "testing mode".

I think to fix the regression it's easiest to add "custom focus ring color" support just to RenderThemeWin.  Someone else can come back and make it more general in a second pass.

[Adam Roben (:aroben)]

(In reply to comment #11)
> (2) (In reply to comment #8)
> > 2) Add a call to set a custom focus ring color. Safari would call this, passing
> > the focus ring color from SafariTheme.
> 
> so this has to run through RenderThemeWin::focusRingColor, doesn't it?
> When the focusRingColor is asked for in
> CSSSylteSelector::getColorFromPrimitiveValue(), as of r45285, the defaultTheme
> is asked to provide this color. 
> 
> as (2) is described i'm confused how RenderThemeWin::focusRingColor will play
> its role.  

getColorFromPrimitiveValue could be the place where the custom color is retrieved. If no custom color is specified, then it would ask the defaultTheme. Doing it this way would make it work for all platforms, and maybe could replace Chromium's special test mode flag.

[Adam Roben (:aroben)]

(In reply to comment #13)
> (In reply to comment #11)
> > (2) (In reply to comment #8)
> > > 2) Add a call to set a custom focus ring color. Safari would call this, passing
> > > the focus ring color from SafariTheme.
> > 
> > so this has to run through RenderThemeWin::focusRingColor, doesn't it?
> > When the focusRingColor is asked for in
> > CSSSylteSelector::getColorFromPrimitiveValue(), as of r45285, the defaultTheme
> > is asked to provide this color. 
> > 
> > as (2) is described i'm confused how RenderThemeWin::focusRingColor will play
> > its role.  
> 
> getColorFromPrimitiveValue could be the place where the custom color is
> retrieved. If no custom color is specified, then it would ask the defaultTheme.
> Doing it this way would make it work for all platforms, and maybe could replace
> Chromium's special test mode flag.

Another option would be to move the implementation of focusRingColor into the base RenderTheme class. That function would basically do:

return customFocusRingColor ? customFocusRingColor : platformFocusRingColor()

And platformFocusRingColor() would do exactly what focusRingColor() does today.

[Alice Liu]

(In reply to comment #14)
> 
> Another option would be to move the implementation of focusRingColor into the
> base RenderTheme class. That function would basically do:
> 
> return customFocusRingColor ? customFocusRingColor : platformFocusRingColor()
> 
> And platformFocusRingColor() would do exactly what focusRingColor() does today.
> 

Just so i'm clear, is RenderThemeChromiumMac the only subclass that would want to set this custom color, that is, in the case of ChromiumBridge::layoutTestMode() ?

[Jeremy Moskovich]

(In reply to comment #15)
> Just so i'm clear, is RenderThemeChromiumMac the only subclass that would want
> to set this custom color, that is, in the case of
> ChromiumBridge::layoutTestMode() ?

(sorry for not commenting earlier, but I've been travelling :( )

Not sure if this answers your question, but to the best of my knowledge, there's no way to tell from inside WebKit whether you're running in layout test mode, in the Chromium port ChromiumBridge::layoutTestMode() gives this information.

The Mac port of Chromium aims to reuse the expected pixel results checked into the tree and thus the focus ring color needs to match that used in the Mac version of WebKit i.e. the behavior of RenderThemeMac.

RenderThemeMac in turn uses a fixed focus ring color in layout test mode, rather than a color corrected focus ring color as is used in normal operation (which to my understanding can change arbitrarily) in order to guarantee a consistent focus ring color in pixel tests regardless of the system the tests are run on.

RenderThemeMac uses a function defined in ColorMac that's toggled by DRT via an accessor method in WebView in order to signal that we're in layout test mode so it can use the "fixed" focus ring color.

[Eric Seidel (no email)]

(In reply to comment #16)
> (In reply to comment #15)

@jeremy:

I believe the model which is being proposed above would involve the embedding app setting a custom focus ring color used to override the normal platform one.  If chromium used this same code path, then test_shell would also set a custom override color when running in "layout test mode".  DumpRenderTree on windows has the same need.  It wants to use the RenderThemeWin, but override the focus ring color to match the Mac (assuming I'm understanding aroben and aliu correctly).  So it seems like there is code to be shared here.

[Eric Seidel (no email)]

I would argue that we should fix the regression in whatever way necessary for Safari-Win and Chroimium can come along later and do the code sharing.  ChromiumBridge isn't in the WebKit tree, so it's impossible for webkit folks to do any related refactoring.

[Alice Liu]

Created attachment 32312 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 32312 [details]
patch

Color has no destructor, but there still may be an issue with using a global Color object.  You'd have to ask Darin.  Certainly the constructor for customFocusRingColor will run at library load time and that could slow down startup.

I think I would have used a class static instead of a file static, but I'm not sure that that would work around the load-time contructor issue, just would have been different encapsulation of the data.

The approach in general looks fine. I would have probably used an if with early return for:
+    return customFocusRingColor.isValid() ? customFocusRingColor : defaultTheme()->platformFocusRingColor();
instead of a ternary operator.

platform seems a bit strange of a name for the focusRingColor since it's not necessarily platform specific (as is the case with RenderThemeSafari).

[Eric Seidel (no email)]

Thanks for taking this on, Alice.

[Darin Adler]

Comment on attachment 32312 [details]
patch

Looks good.

> +static Color customFocusRingColor;

This would fail on Mac (scripts would catch it around link time) because it creates a constructor that runs at framework load time and a destructor that runs at exit time. For an example of the correct idiom for this type of thing, you could look at a function like keepAliveSet() in Frame.cpp that shows how to use DEFINE_STATIC_LOCAL to avoid the exit-time destructor and a function to avoid a load-time constructor.

> +void RenderTheme::setCustomFocusRingColor(Color c)

We'd typically use "const Color&" as the type for the argument to avoid copying the color.

> +    virtual Color platformFocusRingColor() const { return Color(); }

I think platformFocusRingColor should probably be pure virtual instead of returning a transparent color. Or it could return opaque black?

review- because of the load-time and exit-time issue

[Alice Liu]

Created attachment 32330 [details]
patch, without global initializers, compiles on mac and win

I did consider making platformFocusRingColor pure virtual, but doing it that way would have required RenderThemeWin to provide an implementation, and i wasn't convinced that providing an invalid or black color there would have better than providing an invalid or black color in RenderTheme::platformFocusRingColor along with the possible confusion about why RenderThemeWin isn't appearing to provide an aqua-looking color.  In this patch I did change to black as you suggested.

[Darin Adler]

Comment on attachment 32330 [details]
patch, without global initializers, compiles on mac and win

> +    Color& color = customFocusRingColor();
> +    color = c;

Could just be done like this:

    customFocusRingColor() = c;

r=me

[Alice Liu]

Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.order
Sending        WebCore/css/CSSStyleSelector.cpp
Sending        WebCore/platform/graphics/Color.h
Sending        WebCore/rendering/RenderTheme.cpp
Sending        WebCore/rendering/RenderTheme.h
Sending        WebCore/rendering/RenderThemeChromiumMac.h
Sending        WebCore/rendering/RenderThemeChromiumMac.mm
Sending        WebCore/rendering/RenderThemeChromiumSkia.cpp
Sending        WebCore/rendering/RenderThemeChromiumSkia.h
Sending        WebCore/rendering/RenderThemeMac.h
Sending        WebCore/rendering/RenderThemeMac.mm
Sending        WebCore/rendering/RenderThemeSafari.cpp
Sending        WebCore/rendering/RenderThemeSafari.h
Transmitting file data ..............
Committed revision 45569.