Bug 52952

Created attachment 83164 [details] More simple TC

I can reproduce this bug on the trunk. Changseok, could you upload the same test case as html file?

Created attachment 84340 [details] The image is replaced.

I found the reason of this issue and created a patch to solve. But I haven't been able to run regression test(run-webkit-test) on my Ubuntu system(10.10). Is it possible to run this on Ubuntu system? I'm faceing following log message when I run regression test(run-webkit-test). >WARNING: Your platform is not recognized. Any platform-specific results will be generated in >platform/undefined. >Running build-dumprendertree >Building not defined for this platform! >Compiling DumpRenderTree failed! What does this mean?

> There is a image which is a child node of anchor node. > I think that the focused area should include image region but it doesn't. > The focused region is for only anchor node. > > I spent some time to track this issue. > It seems WebCore make right invalidate region. > But Actually rendered region is for anchor region except image region. I've seen this, and I confirm it is a bug. I will have a look.

Created attachment 85438 [details] a proposed patch I have done regression tests. It seems no problem.

Created attachment 85439 [details] a proposed patch There is mimic change removing type casting.

Comment on attachment 85439 [details] a proposed patch It needs a very least one layout test (and possibly one pixel test)

(In reply to comment #8) > (From update of attachment 85439 [details]) > It needs a very least one layout test (and possibly one pixel test) Thanks for your comment. :) By the way, I couldn't get your point. Do you want regression test result or new test cases to evaluate this patch?

(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 85439 [details] [details]) > > It needs a very least one layout test (and possibly one pixel test) > > Thanks for your comment. :) > > By the way, I couldn't get your point. > Do you want regression test result or new test cases to evaluate this patch? ChangSeok, please read these: http://www.webkit.org/blog/1452/layout-tests-theory/ and http://www.webkit.org/blog/1456/layout-tests-practice/

ChangSeok, you have to add a changeLog to your patch. http://www.webkit.org/coding/contributing.html#changelogs

Created attachment 85818 [details] proposed patch Patch updated ChangeLog & test case. Thanks, Antonio & Joone :) I think that layout test is optional but pixel test is mandatory. Because this patch doesn't affect DOM and layout tree. I'm not sure whether the test case is placed at proper directory. Please let me know, if it's false.

Comment on attachment 85818 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=85818&action=review You are only providing mac results. Maybe you plan to watch the bots to grab expected results for other ports (gtk, qt, chromium, win) > Source/WebCore/ChangeLog:6 > + Wrong tab focused region > + https://bugs.webkit.org/show_bug.cgi?id=52952 here you need to say what was the bug and how you fixed in more details. > LayoutTests/ChangeLog:7 > + Wrong tab focused region > + https://bugs.webkit.org/show_bug.cgi?id=52952 > + Ditto

Comment on attachment 85818 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=85818&action=review > Source/WebCore/rendering/RenderInline.cpp:1039 > + top = min(top, toRenderBox(currRenderObj)->logicalTop()); I added a span around the image of the attached sample page, and tested with this patch. It crashed because currRenderObj was not a RenderBox.

Created attachment 85952 [details] Updated ChangeLogs and resolved crash issue that Yael mentioned > You are only providing mac results. Maybe you plan to watch the bots to grab expected results for other ports (gtk, qt, chromium, win) Should I also submit other ports results? Hm... I haven't used qt so it sounds a little hard. :p >> Source/WebCore/ChangeLog:6 >> + Wrong tab focused region >> + https://bugs.webkit.org/show_bug.cgi?id=52952 >here you need to say what was the bug and how you fixed in more details. Done. I added more description about this patch. > LayoutTests/ChangeLog:7 > + Wrong tab focused region > + https://bugs.webkit.org/show_bug.cgi?id=52952 > + Done. In addition. The crash issue that Yael mentioned is resolved.

(In reply to comment #15) > Created an attachment (id=85952) [details] > Updated ChangeLogs and resolved crash issue that Yael mentioned > > > You are only providing mac results. Maybe you plan to watch the bots to grab expected results for other ports (gtk, qt, chromium, win) > Should I also submit other ports results? Hm... > I haven't used qt so it sounds a little hard. :p Unless you want to break all other ports, yes. You can watch the bots and grab results OR you can skip the tests for other ports (you do not want that).

Comment on attachment 85952 [details] Updated ChangeLogs and resolved crash issue that Yael mentioned View in context: https://bugs.webkit.org/attachment.cgi?id=85952&action=review > Source/WebCore/rendering/RenderInline.cpp:1038 > for (InlineFlowBox* curr = firstLineBox(); curr; curr = curr->nextLineBox()) { > RootInlineBox* root = curr->root(); > int top = max(root->lineTop(), curr->logicalTop()); > int bottom = min(root->lineBottom(), curr->logicalBottom()); > + > + Vector<IntRect> focusRingRects; > + addFocusRingRects(focusRingRects, 0, 0); addFocusRingRects should not be inside the for loop. Do we need to have both the original for loop and addFocusRingRects ?

Created attachment 86063 [details] updated patch > addFocusRingRects should not be inside the for loop. > Do we need to have both the original for loop and addFocusRingRects ? We don't. I agree that using addFocusRingRects was wrong approach. Here is a updated patch.

I don't think this is right. Please see https://bugs.webkit.org/show_bug.cgi?id=28625#c1 .

(In reply to comment #19) > I don't think this is right. Please see https://bugs.webkit.org/show_bug.cgi?id=28625#c1 . Yael, could you please advice him the right way?

(In reply to comment #19) > I don't think this is right. Please see https://bugs.webkit.org/show_bug.cgi?id=28625#c1 . Would you explain why? I had a view bug28625, but I couldn't still get your point. ''a When I run the TC of 28625 with patch and without patch, I get same results. Actually this patch may not affect b28625. The focus ring drawn in 28625 TC is different from this bug's one. It doesn't use css outline to draw focus ring, even though it doesn't reach at RenderInline::paintOutline to render it when I move focus with tab key. With this patch. I could get proper outline for anchored image like IE9, Opera10 and FF4. Please let me know your opinion in detail. Thanks :)

(In reply to comment #21) > (In reply to comment #19) > > I don't think this is right. Please see https://bugs.webkit.org/show_bug.cgi?id=28625#c1 . > > Would you explain why? > I had a view bug28625, but I couldn't still get your point. ''a > When I run the TC of 28625 with patch and without patch, I get same results. > Actually this patch may not affect b28625. > The focus ring drawn in 28625 TC is different from this bug's one. > It doesn't use css outline to draw focus ring, even though it doesn't reach at RenderInline::paintOutline to render it when I move focus with tab key. > > With this patch. I could get proper outline for anchored image like IE9, Opera10 and FF4. > Please let me know your opinion in detail. > Thanks :) I was afraid that you are reverting the fix from 28625 :) I have no objection to your patch if you tested the other test case is still passing.

(In reply to comment #22) > I was afraid that you are reverting the fix from 28625 :) I have no objection to your patch if you tested the other test case is still passing. Yael, I ran layout test(including pixel test) again on Ubuntu 10.10 & SnowLeopard, but I couldn't find any issues that this patch make. Of course, 28625 TC too. Please let me know another issue you're concerned(if exist)

Created attachment 86324 [details] updated patch Fix a bug found in real site. http://www.youtube.com/comment_servlet?all_comments=1&v=2uS8cd2OXjE (Need to log in) 'Cancel' focused region was wrong.

Created attachment 86753 [details] updated patch I fixed more specific case bug in real site.

(In reply to comment #25) > Created an attachment (id=86753) [details] > updated patch > > I fixed more specific case bug in real site. Are you updating your test cases so these "real site" problems wont regress? it is also important ...

Created attachment 87072 [details] updated patch The test case is updated. It expands to verify real site issue found.

Any progress? Somebody review this, please.

This looks about right to me. Best if hyatt were to take a look, but I could also review it when my brain isn't busy with bidi text. Ping me in a week if you haven't heard from hyatt.

(In reply to comment #29) > This looks about right to me. Best if hyatt were to take a look, but I could also review it when my brain isn't busy with bidi text. Ping me in a week if you haven't heard from hyatt. O.K Thanks you! Eric :)

Comment on attachment 87072 [details] updated patch 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?

Comment on attachment 87072 [details] updated patch 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.

Created attachment 89225 [details] updated patch updated patch to meet Eric's comment.

Comment on attachment 89225 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=89225&action=review This looks reasonable to me, but I think we should rope hyatt or mitz in here still. Lets see if we can find one of them in #webkit today. > Source/WebCore/rendering/InlineFlowBox.cpp:109 > + } else { > + m_hasTextChildrenOnly = false; > } Style nit: no { } on single line blocks. I'm surprised the style queue didn't flag this. > Source/WebCore/rendering/InlineFlowBox.h:293 > + bool m_hasTextChildrenOnly : 1; I probably would have named this m_hasOnlyTextChildren. So this is only children? Or is this true of all dependents? Do we keep this up to date when grafting on sub-trees? I'm not sure what linebox tree manipulations are allowed that we'd need to worry about.

I'm sorry this patch has been so much trouble. The lineboxtree is a non-trivial piece of the webkit code which few people are familiar with.

Comment on attachment 89225 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=89225&action=review Thanks for review again. :) >> Source/WebCore/rendering/InlineFlowBox.cpp:109 >> } > > Style nit: no { } on single line blocks. I'm surprised the style queue didn't flag this. My mistake. Done. >> Source/WebCore/rendering/InlineFlowBox.h:293 >> + bool m_hasTextChildrenOnly : 1; > > I probably would have named this m_hasOnlyTextChildren. > > So this is only children? Or is this true of all dependents? Do we keep this up to date when grafting on sub-trees? I'm not sure what linebox tree manipulations are allowed that we'd need to worry about. Done. m_hasTextChildrenOnly just hold a condition whether InlineFlowBox has only text type InlineBox child. In my understanding, current paintOutline implementation for css:outline has taken care of only text string. Because of InlineFlowBox's logicalTop. When the top of outline is decided, the logical top of InlineFlowBox is refered. Although RootInlineBox has proper top position, it could not be bigger than top of text line. I tested another case for checking m_hasOnlyTextChildren is kept up to date. Yes, it is. I added a new image child to parentAnchor node using by setTimeout JS function. The outline was expanded successfully like FF and Opera. Why it does is that m_hasOnlyTextChildren is kept up to date in InlineFlowBox::addToLine when a new InlineBox is added to InlineFlowBox.

Created attachment 89913 [details] Updated patch to meet Eric's comment.

(In reply to comment #34) > (From update of attachment 89225 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=89225&action=review > > This looks reasonable to me, but I think we should rope hyatt or mitz in here still. Lets see if we can find one of them in #webkit today. Eric. Would you let me know hyatt and mitz's irc IDs?

dhyatt and mitzpettel. IRC names are listed for most committers in Tools/Scripts/webkitpy/common/config/committers.py or on wiki.webkit.org

Comment on attachment 89913 [details] Updated patch to meet Eric's comment. View in context: https://bugs.webkit.org/attachment.cgi?id=89913&action=review Please Hyatt or Mitz, would you mind to have a look? Author, you are adding a non-port-specific test, but adding Mac-specific results only. Just be sure to follow up grabbing results from the bots to update the other platforms (qt, gtk, windows and their webkit2 variants, etc). > Source/WebCore/ChangeLog:12 > + So font height is only calculated to get top position. > + To fix this, Some lines are added to get maxium line height and to recalulate top position. Typo: - "Some" does not need capital "S". - "maxium" > Source/WebCore/rendering/InlineFlowBox.cpp:106 > m_hasTextDescendants = true; > - } > + m_hasOnlyTextChildren = false; "Descendants" x "Children" is a bit confusing here, but ok. It is not your patch's fault. > LayoutTests/ChangeLog:9 > + Test that the outline of anchor node which has non-text child node like image node. > + Layout test wouldn't verify this. Pixel test is valid. A spatial navigation test could easily very this, btw. Pixel test is also ok, but as a follow up, it would be great to add one too. > LayoutTests/fast/repaint/outline-focus-anchor-image.html:18 > + <a id="ID_parentAnchor" href="#"> Nit: not need for "ID_"

(In reply to comment #40) > Author, you are adding a non-port-specific test, but adding Mac-specific results only. Just be sure to follow up grabbing results from the bots to update the other platforms (qt, gtk, windows and their webkit2 variants, etc). >> Thank you for comments! :) >> May I ask you an ignorant question? >> As your comment, I could get other port's test results without building them. Right? You said I could get the results by just "watching bot and grabbing results". Hm.. I can't understand this. How could I do? Please let me know where I could see the bot's activity, so grab results of other ports.

(In reply to comment #41) > (In reply to comment #40) > > Author, you are adding a non-port-specific test, but adding Mac-specific results only. Just be sure to follow up grabbing results from the bots to update the other platforms (qt, gtk, windows and their webkit2 variants, etc). > >> Thank you for comments! :) > >> May I ask you an ignorant question? > >> As your comment, I could get other port's test results without building them. Right? You said I could get the results by just "watching bot and grabbing results". Hm.. I can't understand this. How could I do? > Please let me know where I could see the bot's activity, so grab results of other ports. You can access the result of all core bots, for each revision tested from here: http://build.webkit.org/results/

(In reply to comment #42) > (In reply to comment #41) > > (In reply to comment #40) > > > Author, you are adding a non-port-specific test, but adding Mac-specific results only. Just be sure to follow up grabbing results from the bots to update the other platforms (qt, gtk, windows and their webkit2 variants, etc). > > >> Thank you for comments! :) > > >> May I ask you an ignorant question? > > >> As your comment, I could get other port's test results without building them. Right? You said I could get the results by just "watching bot and grabbing results". Hm.. I can't understand this. How could I do? > > Please let me know where I could see the bot's activity, so grab results of other ports. > > You can access the result of all core bots, for each revision tested from here: http://build.webkit.org/results/ I appreciate your kind info. :) As my understanding, the test results were made when a patch was landed on main stream after passing review. Then, is it O.K that I update this patch to add results for other ports after passing review(if it's possible)? If so, I'm willing to do :)

Created attachment 99716 [details] updated patch

Comment on attachment 89913 [details] Updated patch to meet Eric's comment. View in context: https://bugs.webkit.org/attachment.cgi?id=89913&action=review Thank you for review. >> Source/WebCore/ChangeLog:12 >> + To fix this, Some lines are added to get maxium line height and to recalulate top position. > > Typo: > - "Some" does not need capital "S". > - "maxium" Ops. Sorry :p >> LayoutTests/fast/repaint/outline-focus-anchor-image.html:18 >> + <a id="ID_parentAnchor" href="#"> > > Nit: not need for "ID_" Done.

Comment on attachment 99716 [details] updated patch Attachment 99716 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8988377 New failing tests: fast/repaint/outline-focus-anchor-image.html

Created attachment 99718 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 99812 [details] updated patch

Is the problem still reproducible? Is the patch still valid? It seems there's no activity on the bug for a while.

(In reply to comment #49) > Is the problem still reproducible? Is the patch still valid? It seems there's no activity on the bug for a while. 1) Yes 2) I am not sure if the patch is still valid. I will review it tomorrow properly.

(In reply to comment #50) > (In reply to comment #49) > > Is the problem still reproducible? Is the patch still valid? It seems there's no activity on the bug for a while. > > 1) Yes > > 2) I am not sure if the patch is still valid. I will review it tomorrow properly. Thanks for your interest. I've been just waiting somebody review this. It's a little hard to take a review for layout issues. :p

Created attachment 121435 [details] Patch

Comment on attachment 121435 [details] Patch Attachment 121435 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11141435 New failing tests: fast/repaint/outline-focus-anchor-image.html

Comment on attachment 121435 [details] Patch This patch doesn't seem right. Why do we need to create a new special case for this?