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

[bugzilla-daemon at webkit.org]

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





--- Comment #32 from ChangSeok Oh <kevin.cs.oh at gmail.com>  2011-04-12 10:43:31 PST ---
(From update of attachment 87072)
View in context: https://bugs.webkit.org/attachment.cgi?id=87072&action=review

>> 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. :)

First of all. Thank you for review! :)
Sure. I update ChangLog.

>> 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?

A new variable is created in InlineFlowBox class to hold condition that InlineFlowBox has text children only.
And new InlineBox is checked whether it's text or not when it's added to InlineFlowBox.
In this way, we can avoid a loop like this line.
Hyatt's recent change? Do you mean bug28625 Yael mentioned above? When I test bug28625, this patch doesn't affect it.

>> 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.

Yeh. similar checking have been doing in InlineFlowBox::addToLine. A new variable in InlineFlowBox will hold the condition.

>> 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?

No. This is NOT formal CSS 2.1 test. This test case is written by myself. I just use CSS category of outline as title. :)
The title is changed to avoid confusion.
Of course, I've run layout tests(including pixel tests) several times for this patch.  No problem as I know.

>> 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?

apple.jpg was only one available image in LayoutTest/fast/repaint directory when I wrote this TC.
I didn't know that adding new image for TC was O.K. I'll replace apple.jpg with greenSquare.png submitted newly.

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