Summary: | Only draw focus ring in RenderInline and RenderImage if the theme is not able to draw a focus ring | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | Layout and Rendering | Assignee: | Daniel Bates <dbates> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | hyatt, mitz, tonikitoo | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Daniel Bates
2010-10-13 16:06:13 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> 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().
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.
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. (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. Committed r69766: <http://trac.webkit.org/changeset/69766> |