Bug 48393

Summary: Spatial Navigation: Cannot focus on some ContainerNode
Product: WebKit Reporter: pgbasin
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, hyatt, webkit.review.bot, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 46905    
Attachments:
Description Flags
Patch none

pgbasin
Reported 2010-10-26 20:23:57 PDT
In the test page below, the node "llinker0" cannot be focused. ============================================== <html> <body> <a href="#">This is an element</a><br> <div> <a href="#" name="llinker0" id=llinker0> <img src="images/btn.gif" height="42" width="76" alt="llinker0" border="0"/> </a> </div> <div> <a href="#">This is an element</a><br> </div> <a href="#">This is an element</a><br> <a href="#" name="llinker1" id=llinker1> <img src="images/btn.gif" height="42" width="76" alt="llinker1" border="0"/> </a><br> <a href="#">This is an element</a><br> </body> </html> ==============================================
Attachments
Patch (9.95 KB, patch)
2010-11-12 14:27 PST, Yael 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
pgbasin
Comment 1 2010-10-26 20:26:55 PDT
I traced the code, and found that Node::getRect() can return correct rect, however ContainerNode::getRect() cannot. And here is a workaround: svn diff WebCore/page/SpatialNavigation.cpp Index: WebCore/page/SpatialNavigation.cpp =================================================================== --- WebCore/page/SpatialNavigation.cpp (revision 70438) +++ WebCore/page/SpatialNavigation.cpp (working copy) @@ -105,7 +105,8 @@ { ASSERT(render && render->node()); - IntRect rect = render->node()->getRect(); + //IntRect rect = render->node()->getRect(); + IntRect rect = render->absoluteBoundingBoxRect(); // In cases when the |render|'s associated node is in a scrollable inner // document, we only consider its scrollOffset if it is not offscreen.
Yael
Comment 2 2010-11-12 07:51:51 PST
This test is still failing, even with the patch at https://bugs.webkit.org/show_bug.cgi?id=49382. I am going to look at this failure.
Yael
Comment 3 2010-11-12 09:30:57 PST
If you remove all the white spaces after the image, getRect() does return the image size correctly. The problem starts at ContainerNode::getLowerRightCorner(). It gets confused by the empty white space, with size 0, and thinks that there is no content at all.
Yael
Comment 4 2010-11-12 09:48:59 PST
I can reproduce this problem with this code: <html> <body> <a href="#">This is an element</a><br> <div> <a href="#"><img src="images/btn.gif" height="42" width="76"/> </a> </div> <a href="#">This is an element</a> </body> </html> If I remove the space between the image and the closing anchor, the problem goes away, and getRect() on the anchor element reports the correct size. Dave, would you have some ideas about what is going on? thanks!
Yael
Comment 5 2010-11-12 14:27:11 PST
Created attachment 73779 [details] Patch ContainerNode::getLowerRightCorner() assumes that the lowest right corner of its last child is its own lowest right corner. If that child is an empty text node after an image, it does not have size and position information, so we should go to the previous child.
Dave Hyatt
Comment 6 2010-11-12 14:51:50 PST
Comment on attachment 73779 [details] Patch r=me
WebKit Commit Bot
Comment 7 2010-11-12 16:48:22 PST
Comment on attachment 73779 [details] Patch Clearing flags on attachment: 73779 Committed r71956: <http://trac.webkit.org/changeset/71956>
WebKit Commit Bot
Comment 8 2010-11-12 16:48:29 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 9 2010-11-12 17:53:00 PST
http://trac.webkit.org/changeset/71956 might have broken Leopard Intel Release (Tests) The following tests are not passing: svg/dom/SVGScriptElement/script-load-and-error-events.svg
Yael
Comment 10 2010-11-12 18:16:55 PST
(In reply to comment #9) > http://trac.webkit.org/changeset/71956 might have broken Leopard Intel Release (Tests) > The following tests are not passing: > svg/dom/SVGScriptElement/script-load-and-error-events.svg All tests on Leopard Intel Release are passing after http://trac.webkit.org/changeset/71956, so I guess it was a false alarm.
Yael
Comment 11 2010-11-12 18:17:34 PST
(In reply to comment #10) > (In reply to comment #9) > > http://trac.webkit.org/changeset/71956 might have broken Leopard Intel Release (Tests) > > The following tests are not passing: > > svg/dom/SVGScriptElement/script-load-and-error-events.svg > > All tests on Leopard Intel Release are passing after http://trac.webkit.org/changeset/71956, so I guess it was a false alarm. I meant http://trac.webkit.org/changeset/71958 :-)
Yael
Comment 12 2010-11-12 18:26:29 PST
Re-opened by mistake. Sorry.
Note You need to log in before you can comment on or make changes to this bug.