RESOLVED FIXED 47142
Spatial navigation not Working for Map and Area Tags 
https://bugs.webkit.org/show_bug.cgi?id=47142
Summary Spatial navigation not Working for Map and Area Tags
umesh
Reported 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.
Attachments
it is test file (1.28 KB, text/html)
2010-10-04 21:22 PDT, umesh 		no flags
Details
Patch. (27.17 KB, patch)
2010-11-28 12:12 PST, Yael 		tonikitoo: review-
DetailsFormatted DiffDiff
Patch (23.18 KB, patch)
2010-11-29 08:30 PST, Yael 		no flags
DetailsFormatted DiffDiff
Patch. (23.01 KB, patch)
2010-12-03 19:38 PST, Yael 		no flags
DetailsFormatted DiffDiff
Patch. (29.60 KB, patch)
2010-12-05 09:35 PST, Yael 		tonikitoo: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
umesh
Comment 1 2010-10-04 21:22:34 PDT
Created attachment 69735 [details] it is test file
pgbasin
Comment 2 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>
Arun Patole
Comment 3 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.
Yael
Comment 4 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.
Yael
Comment 5 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.
Antonio Gomes
Comment 6 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 ...
pgbasin
Comment 7 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.
Yael
Comment 8 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.
Yael
Comment 9 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!
Yael
Comment 10 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.
Yael
Comment 11 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 :-)
Yael
Comment 12 2010-12-03 19:38:26 PST
Created attachment 75591 [details] Patch. Re-baseline the patch.
Yael
Comment 13 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 .
Antonio Gomes
Comment 14 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 :(
Yael
Comment 15 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.
Antonio Gomes
Comment 16 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); > >
Yael
Comment 17 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.
Antonio Gomes
Comment 18 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.
Yael
Comment 19 2010-12-07 11:06:14 PST
Committed r73452: <http://trac.webkit.org/changeset/73452>
Roshan Khaire
Comment 20 2016-11-14 04:07:08 PST
Thanks For Post. https://www.hdfc.com/home-loans-salaried
Note You need to log in before you can comment on or make changes to this bug.