Bug 26821 - REGRESSION(r45285): focus rings are black on windows safari
Summary: REGRESSION(r45285): focus rings are black on windows safari
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Critical
Assignee: Alice Liu
URL: data:text/html,%3Cinput%20autofocus=t...
Keywords: InRadar
: 26822 26823 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-06-29 17:53 PDT by Alice Liu
Modified: 2009-07-06 15:12 PDT (History)
8 users (show)

See Also:


Attachments
patch - just duplicates what is in RenderThemeSafari (3.42 KB, patch)
2009-06-30 14:18 PDT, Alice Liu
aroben: review-
Details | Formatted Diff | Diff
patch (10.60 KB, patch)
2009-07-06 13:26 PDT, Alice Liu
darin: review-
Details | Formatted Diff | Diff
patch, without global initializers, compiles on mac and win (10.83 KB, patch)
2009-07-06 15:01 PDT, Alice Liu
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Liu 2009-06-29 17:53:45 PDT
due to http://trac.webkit.org/changeset/45285, focus rings are now black on windows safari instead of aqua.
Comment 1 Alice Liu 2009-06-29 17:58:22 PDT
<rdar://problem/7018252>
Comment 2 Mark Rowe (bdash) 2009-06-29 18:04:09 PDT
*** Bug 26822 has been marked as a duplicate of this bug. ***
Comment 3 Mark Rowe (bdash) 2009-06-29 18:04:13 PDT
*** Bug 26823 has been marked as a duplicate of this bug. ***
Comment 4 Eric Seidel (no email) 2009-06-29 18:04:42 PDT
Seems easy enough to fix.  RenderThemeWin or RenderThemeSafari just need a focusRingColor method if they don't have one.
Comment 5 Alice Liu 2009-06-30 12:04:55 PDT
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?
Comment 6 Alice Liu 2009-06-30 14:18:08 PDT
Created attachment 32094 [details]
patch - just duplicates what is in RenderThemeSafari
Comment 7 Eric Seidel (no email) 2009-06-30 14:21:01 PDT
Comment on attachment 32094 [details]
patch - just duplicates what is in RenderThemeSafari

Shouldn't SafariThemeWin use whatever the system theme focus color is?
Comment 8 Adam Roben (:aroben) 2009-06-30 15:53:08 PDT
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?
Comment 9 Adam Roben (:aroben) 2009-06-30 15:53:37 PDT
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.
Comment 10 Eric Seidel (no email) 2009-06-30 16:06:11 PDT
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.
Comment 11 Alice Liu 2009-06-30 16:20:14 PDT
(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.  
Comment 12 Eric Seidel (no email) 2009-06-30 16:29:33 PDT
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.
Comment 13 Adam Roben (:aroben) 2009-06-30 16:31:53 PDT
(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.
Comment 14 Adam Roben (:aroben) 2009-06-30 16:33:17 PDT
(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.
Comment 15 Alice Liu 2009-06-30 17:05:46 PDT
(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() ?
Comment 16 Jeremy Moskovich 2009-07-04 04:37:36 PDT
(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.
Comment 17 Eric Seidel (no email) 2009-07-04 10:35:31 PDT
(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.
Comment 18 Eric Seidel (no email) 2009-07-04 10:36:40 PDT
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.
Comment 19 Alice Liu 2009-07-06 13:26:44 PDT
Created attachment 32312 [details]
patch
Comment 20 Eric Seidel (no email) 2009-07-06 13:38:37 PDT
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).
Comment 21 Eric Seidel (no email) 2009-07-06 13:39:00 PDT
Thanks for taking this on, Alice.
Comment 22 Darin Adler 2009-07-06 13:39:37 PDT
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
Comment 23 Alice Liu 2009-07-06 15:01:29 PDT
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.
Comment 24 Darin Adler 2009-07-06 15:02:53 PDT
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
Comment 25 Alice Liu 2009-07-06 15:12:16 PDT
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.