[Webkit-unassigned] [Bug 52952] Focus ring for anchor with inline image is incorrect

Mon Apr 11 06:10:09 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=52952

Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #87072|review?, commit-queue?      |review-
               Flag|                            |

--- Comment #31 from Eric Seidel <eric at webkit.org>  2011-04-11 06:10:09 PST ---
(From update of attachment 87072)
View in context: https://bugs.webkit.org/attachment.cgi?id=87072&action=review

I think we need another round and some questions answered.  I'm sorry I didn't look at this sooner.  Thank you very much for the reminder email!

> Source/WebCore/ChangeLog:5
> +        Wrong tab focused region

You might want to update the bug titles in the ChangeLogs.  Not important, but the new title is definitely more descriptive than the old one. :)

> Source/WebCore/rendering/RenderInline.cpp:1039
> +        for (InlineBox* child = curr->firstChild(); child; child = child->next()) {

Hmm... Maybe we should be caching this information on the root line boxes instead?

How is this affected by hyatt's recent change of removing many lineboxes from the linebox tree?

> Source/WebCore/rendering/RenderInline.cpp:1041
> +            if (!renderObj->isText()) {

I take it the text case is already handled somewhere else?

It seems we should be saving this information on the parent linebox as part of layout()?  But I iadmit not to be a painting expert for the linebox tree yet.

> LayoutTests/fast/repaint/outline-focus-anchor-image.html:4
> +    <title>CSS 2.1 Test Suite: outline</title>

So is this a modified version of a CSS2.1 test?  Or is this just imported directly from CSS 2.1's test suite? I believe we already have the css 2.1 test suite imported into LayoutTests.  Have you run all the layout tests to see if your change affects any of the rest of them?

> LayoutTests/fast/repaint/outline-focus-anchor-image.html:25
> +        Text before image anchor <img src="resources/apple.jpg" width=100px height=100px /><br>

apple.jpg seems to be missing from your patch.  Again, you might not even need this test as it may already be in our repository, unless you modified an existing CSS21 test?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.