Bug 47632 – Only draw focus ring in RenderInline and RenderImage if the theme is not able to draw a focus ring

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Daniel Bates]

We should only draw a focus ring if the theme is not able to draw a focus ring (i.e. RenderTheme::supportsFocusRing returns false).

Currently, only RenderObject::paintOutline() checks that the theme is unable to draw a focus ring before drawing one. Both RenderInline::paintOutline() and RenderImage::paintFocusRings() draw a focus ring regardless of whether the theme is able to draw a focus ring. Instead, these methods should only draw a focus ring if the theme is not able to draw one. This will make the code consistent with the behavior in RenderObject::paintOutline() as well as support port-specific focus ring drawing and/or disabling of focus ring drawing.

[Daniel Bates]

For completeness:

There are exactly three call sites of GraphicsContext::drawFocusRing():

RenderObject::paintOutline():
<http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderObject.cpp?rev=68911#L1187>

RenderInline::paintOutline():
<http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderInline.cpp?rev=69220#L1002>

And

RenderImage::paintFocusRings():
<http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderImage.cpp?rev=69423#L335>

[Daniel Bates]

Created attachment 70681 [details]
Patch

This patch also extracts the common code focus ring drawing code from RenderObject::paintOutline() and RenderInline::paintOutline() into RenderObject::paintFocusRing().

[Daniel Bates]

Created attachment 70686 [details]
Patch with pixel-tests

Added Mac pixel tests to ensure that we don't regress focus ring drawing on the Mac port. Will follow up to generate pixel test results for Windows (if they differ) as well as other ports.

While working on this patch, I noticed that on Mac Safari the default focus ring color for <area> (when the CSS outline-color property is unspecified) is black which differs from the default Aqua-blue focus ring color for both hyperlinks and form controls. This seems weird. We should look into this further to ensure this is expected and I think this is best addressed in a separate bug.

[Darin Adler]

Comment on attachment 70686 [details]
Patch with pixel-tests

View in context: https://bugs.webkit.org/attachment.cgi?id=70686&action=review

> WebCore/rendering/RenderInline.cpp:995
> +    paintFocusRing(graphicsContext, tx, ty, styleToUse);

The vast majority of elements don’t have a focus ring to draw, but now we will make a function call and check the theme before we quickly discover there is nothing to draw.

> WebCore/rendering/RenderObject.cpp:1197
> +    paintFocusRing(graphicsContext, tx, ty, styleToUse);

Same comment here. Can we preserve the inline fast check even if the bulk of the painting is left in a non-inline function.

[Daniel Bates]

(In reply to comment #4)
> (From update of attachment 70686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70686&action=review
> 
> > WebCore/rendering/RenderInline.cpp:995
> > +    paintFocusRing(graphicsContext, tx, ty, styleToUse);
> 
> The vast majority of elements don’t have a focus ring to draw, but now we will make a function call and check the theme before we quickly discover there is nothing to draw.
> 

Will move checks for styleToUse->outlineStyleIsAuto(), hasOutlineAnnotation(), and theme->supportsFocusRing() from inside paintFocusRing() to before the call to paintFocusRing(). That is, the call sites will have the form:

if ((styleToUse->outlineStyleIsAuto() || hasOutlineAnnotation()) && !theme()->supportsFocusRing(styleToUse))
    paintFocusRing(graphicsContext, tx, ty, styleToUse);

> > WebCore/rendering/RenderObject.cpp:1197
> > +    paintFocusRing(graphicsContext, tx, ty, styleToUse);
> 
> Same comment here. Can we preserve the inline fast check even if the bulk of the painting is left in a non-inline function.

Ditto.

Will make both changes before I land.

[Daniel Bates]

Committed r69766: <http://trac.webkit.org/changeset/69766>