Bug 47142

Summary: Spatial navigation not Working for Map and Area Tags
Product: WebKit Reporter: umesh <sumeshfirst>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: dbates, khaireroshan06, laszlo.gombos, pgbasin, tonikitoo, webkit.arunp, yael
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 36044, 48438, 49382    
Bug Blocks: 46905    
Attachments:
Description Flags
it is test file
none
Patch.
tonikitoo: review-
Patch
none
Patch.
none
Patch. tonikitoo: review+

Description umesh 2010-10-04 21:21:42 PDT 
The Spatial navigation finds the best candidate node and moves the focus on it.
In case the HTML page has Map and Area Tags the focus never comes on these items. The Map and Area Tags are not handled for Spatial Navigation. The Render Box does the rendering based on rectangle, the shapes such as circle, polygon are not handled as such. The test sites on which we tested are set as attachement.

For URL : https://www.icicibank.com, the focus never comes on the login form, through Spatial navigation.

For URL : https://www.ebay.com, the focus never comes on certain images implemented as map's and using Area tags.

I have attached the g.html which is just a test file, After loading this file:
1) press DOWN Arrow KEY the focus comes on the input button
2) press Down Arrow Key the focus never comes on the login form.
Comment 1 umesh 2010-10-04 21:22:34 PDT 
Created attachment 69735 [details]
it is test file
Comment 2 pgbasin 2010-10-26 23:30:58 PDT 
A simple test page:

$ cat index.html
<html>
  <body>
    <a href="#" shape="rect" name="link1">link1</a><br>

    <map name="Map">
      <area href="javascript:alert('a');" shape="rect" coords="  0,   0, 100,  40" alt="">
      <area href="javascript:alert('b');" shape="rect" coords="  0,  40, 100,  80" alt="">
    </map>
    <img src="http://www.google.com/images/logos/ps_logo2.png" usemap="#Map" width="100" height="80" border="0" alt=""><br>

    <a href="#" shape="rect" name="link2">link2</a><br>
  </body>
</html>
Comment 3 Arun Patole 2010-11-16 22:03:52 PST 
One way to solve this could be to specifically handle area/map element in spatial navigation. For every area element, while calculating the distance:
-get the parent map element
-get the image from map element
-and the call the already existing function HTMLAreaElement::getRect(RenderObject* obj) to get the area element rect instead of getting rect from renderer.
Comment 4 Yael 2010-11-21 14:59:41 PST 
I have a patch for this, but it depends on the patch at https://bugs.webkit.org/show_bug.cgi?id=49382.
Comment 5 Yael 2010-11-28 12:12:59 PST 
Created attachment 74978 [details]
Patch.

Give area elements special treatment in spatial navigation algorithm by getting the rect of the area from the associated image. Since area elements are likely to overlap if they are not rects, or if authors are not careful, we flatten the rect of area elements.
Comment 6 Antonio Gomes 2010-11-28 22:25:57 PST 
Comment on attachment 74978 [details]
Patch.

I think this patch does more that fixing this bug, and because of this, I dislike it. Changes in FocusCandidate should come in its own patch, and should not change any functionally, so no test needed, etc ...
Comment 7 pgbasin 2010-11-29 00:34:15 PST 
I tested the patch, and the area element can get focus.
However, I cannot see any focus ring.
Comment 8 Yael 2010-11-29 05:33:16 PST 
Focus(In reply to comment #7)
> I tested the patch, and the area element can get focus.
> However, I cannot see any focus ring.

The focus ring of area elements is being worked on in https://bugs.webkit.org/show_bug.cgi?id=49888.
Comment 9 Yael 2010-11-29 05:36:55 PST 
(In reply to comment #7)
> I tested the patch, and the area element can get focus.
> However, I cannot see any focus ring.

BTW, thanks for testing!
Comment 10 Yael 2010-11-29 07:17:26 PST 
(In reply to comment #6)
> (From update of attachment 74978 [details])
> I think this patch does more that fixing this bug, and because of this, I dislike it. Changes in FocusCandidate should come in its own patch, and should not change any functionally, so no test needed, etc ...

Filed https://bugs.webkit.org/show_bug.cgi?id=50153.
Comment 11 Yael 2010-11-29 08:30:28 PST 
Created attachment 75031 [details]
Patch

Update the patch after http://trac.webkit.org/changeset/72797.
BTW, the style error seems bogus to me :-)
Comment 12 Yael 2010-12-03 19:38:26 PST 
Created attachment 75591 [details]
Patch.

Re-baseline the patch.
Comment 13 Yael 2010-12-04 18:59:16 PST 
(In reply to comment #12)
> Created an attachment (id=75591) [details]
> Patch.
> 
> Re-baseline the patch.

This patch is actually very simple. It does 2 things:
1. HTMLAreaElements are special in that they do not have a rect of their own. So we create a special constructor for FocusController, that explicitly takes an HTMLAreaElement as a parameter instead of a Node. We then use the rect of the area in the image for the spatial navigation algorithm.

2. HTMLAreaElements are not necessarily rects, so instead of using the whole rect of the area, we create a thin rect, 1 pixel thick, in attempt to overcome the overlapping problem. An example for this overlapping is http://www.mozilla.org/access/qa/tiny/map_area.html .
Comment 14 Antonio Gomes 2010-12-04 20:20:18 PST 
Comment on attachment 75591 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=75591&action=review

Looks fine, but we might be able to make it better. Lets see answers for some comments and questions I have before go for it.

> WebCore/page/FocusController.cpp:480
> +            if (!node->isKeyboardFocusable(event))
> +                continue;

Lets make one the two checks for !node->isKeyboardFocusable(event) in a follow up.

> WebCore/page/SpatialNavigation.cpp:87
> +    actualNode = area;
> +    node = image;

I do not like the special case you gave for area elements. Maybe we should have a FocusCandidate.isAreaElement() method instead and handling the changes accordingly. It is very implicit for who read the code what or when use 'node' instead of 'actualNode' , and vice-versa.

Could we try something to improve this?

> WebCore/page/SpatialNavigation.cpp:701
> +    IntRect rect = virtualRectForDirection(direction, rectToAbsoluteCoordinates(area->document()->frame(), area->getRect(area->imageElement()->renderer())), 1);

Could you please explain the '1' with other words here. I did not follow it :(
Comment 15 Yael 2010-12-05 07:01:52 PST 
(In reply to comment #14)
Thanks for he review, Antonio.

> I do not like the special case you gave for area elements. Maybe we should have a FocusCandidate.isAreaElement() method instead and handling the changes accordingly. It is very implicit for who read the code what or when use 'node' instead of 'actualNode' , and vice-versa.
> 
> Could we try something to improve this?
I agree that node and actualNode are poor choice for names. How about if we turn FocusCandidate into a class with 2 accessor functions: visibleNode() and focusableNode() ? If we add FocusCandidate.isAreaElement(), the logic of handling HTMLAreaElements would be spread through the spatial navigation code instead of being encapsulated in FocusCandidate, which is what I was trying to do.


> > WebCore/page/SpatialNavigation.cpp:701
> > +    IntRect rect = virtualRectForDirection(direction, rectToAbsoluteCoordinates(area->document()->frame(), area->getRect(area->imageElement()->renderer())), 1);
> 
> Could you please explain the '1' with other words here. I did not follow it :(

I was hoping that comment #13 would explain that :-)
HTMLAreaElements are not necessarily rects, and if we use the bounding rects for the spatial navigation algorithm, we might skip some overlapping nodes. Just look at http://www.mozilla.org/access/qa/tiny/map_area.html for an example of a "busy" image map.
In order to make that page work, I needed a way to "shrink" the bounding rects of HTMLAreaElements. The way I did it is by constructing a rect that starts at the same position as HTMLAreaElement::getRect(), but it has a 1 pixel width or height, depending on the direction of navigation.
This avoids the overlap, and now I can reach all the areas in that imagemap.
This works well also with the imagemap at the top of www.ebay.com, which is not using rects either.
Comment 16 Antonio Gomes 2010-12-05 07:27:16 PST 
(In reply to comment #15)
> (In reply to comment #14)
> Thanks for he review, Antonio.
> 
> > I do not like the special case you gave for area elements. Maybe we should have a FocusCandidate.isAreaElement() method instead and handling the changes accordingly. It is very implicit for who read the code what or when use 'node' instead of 'actualNode' , and vice-versa.
> > 
> > Could we try something to improve this?
> I agree that node and actualNode are poor choice for names. How about if we turn FocusCandidate into a class with 2 accessor functions: visibleNode() and focusableNode() ?

I think these (better names) would be the code more readable, Yael. 

> If we add FocusCandidate.isAreaElement(), the logic of handling HTMLAreaElements would be spread through the spatial navigation code instead of being encapsulated in FocusCandidate, which is what I was trying to do.
> 
> 
> > > WebCore/page/SpatialNavigation.cpp:701
> > > +    IntRect rect = virtualRectForDirection(direction, rectToAbsoluteCoordinates(area->document()->frame(), area->getRect(area->imageElement()->renderer())), 1);
> >
Comment 17 Yael 2010-12-05 09:35:25 PST 
Created attachment 75631 [details]
Patch.

Change from previous patch:
Rename node => visibleNode
Rename actualNode => focusableNode

Consolidated isFocusableNode, as requested in comment #14.
Comment 18 Antonio Gomes 2010-12-07 06:57:43 PST 
Comment on attachment 75631 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=75631&action=review

> WebCore/page/SpatialNavigation.h:123
> +    Node* visibleNode;
>      Node* enclosingScrollableBox;
> +    Node* focusableNode;

Please add a comment in the code explaining the difference between visibleNode and focusableNode, and when to use one or the other.

I would also like to see asserts in the code if one is trying to use one, but should be using the other.
Comment 19 Yael 2010-12-07 11:06:14 PST 
Committed r73452: <http://trac.webkit.org/changeset/73452>
Comment 20 Roshan Khaire 2016-11-14 04:07:08 PST 
Thanks For Post. https://www.hdfc.com/home-loans-salaried