Bug 47142

Summary:

Spatial navigation not Working for Map and Area Tags

Product:

WebKit

Reporter:

umesh <sumeshfirst>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Major

CC:

dbates, khaireroshan06, laszlo.gombos, pgbasin, tonikitoo, webkit.arunp, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

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+

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-	Details Formatted Diff Diff
Patch (23.18 KB, patch) 2010-11-29 08:30 PST, Yael	no flags	Details Formatted Diff Diff
Patch. (23.01 KB, patch) 2010-12-03 19:38 PST, Yael	no flags	Details Formatted Diff Diff
Patch. (29.60 KB, patch) 2010-12-05 09:35 PST, Yael	tonikitoo: review+	Details Formatted Diff Diff
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.