Bug 48393 - Spatial Navigation: Cannot focus on some ContainerNode
Summary: Spatial Navigation: Cannot focus on some ContainerNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 46905
  Show dependency treegraph
 
Reported: 2010-10-26 20:23 PDT by pgbasin
Modified: 2010-11-12 18:26 PST (History)
6 users (show)

See Also:


Attachments
Patch (9.95 KB, patch)
2010-11-12 14:27 PST, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pgbasin 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>
==============================================
Comment 1 pgbasin 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.
Comment 2 Yael 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.
Comment 3 Yael 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.
Comment 4 Yael 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!
Comment 5 Yael 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.
Comment 6 Dave Hyatt 2010-11-12 14:51:50 PST
Comment on attachment 73779 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2010-11-12 16:48:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 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
Comment 10 Yael 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.
Comment 11 Yael 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 :-)
Comment 12 Yael 2010-11-12 18:26:29 PST
Re-opened by mistake. Sorry.