RESOLVED FIXED 47632
Only draw focus ring in RenderInline and RenderImage if the theme is not able to draw a focus ring
https://bugs.webkit.org/show_bug.cgi?id=47632
Summary Only draw focus ring in RenderInline and RenderImage if the theme is not able...
Daniel Bates
Reported 2010-10-13 16:06:13 PDT
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.
Attachments
Patch (5.52 KB, patch)
2010-10-13 16:21 PDT, Daniel Bates
no flags
Patch with pixel-tests (113.26 KB, patch)
2010-10-13 17:08 PDT, Daniel Bates
darin: review+
Daniel Bates
Comment 1 2010-10-13 16:07:02 PDT
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
Comment 2 2010-10-13 16:21:48 PDT
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
Comment 3 2010-10-13 17:08:38 PDT
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 4 2010-10-13 17:18:35 PDT
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
Comment 5 2010-10-13 18:05:42 PDT
(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
Comment 6 2010-10-14 08:41:29 PDT
Note You need to log in before you can comment on or make changes to this bug.