Bug 48393

Summary:

Spatial Navigation: Cannot focus on some ContainerNode

Product:

WebKit

Reporter:

pgbasin

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, eric, hyatt, webkit.review.bot, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

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	Details Formatted Diff Diff
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.