Bug 119468

Summary: [Spatial Navigation] : should prefer focusable elements with absolute positioning over other elements if both elements visible area completely intersects each other.
Product: WebKit Reporter: Abhijeet Kandalkar <kandalkar.abhijeet58>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED    
Severity: Normal CC: commit-queue, cshu, darin, dbates, enrica, gyuyoung.kim, joepeck, kandalkar.abhijeet58, kling, mitz, simon.fraser, tonikitoo, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 56938    
Attachments:
Description Flags
Test
none
Updated patch-1
none
Updated patch-2
tonikitoo: review-, tonikitoo: commit-queue-
Updated patch-3 beidson: review-

Abhijeet Kandalkar
Reported 2013-08-03 11:33:08 PDT
Created attachment 208068 [details] Test Steps : 1. Open attached html page :snav-z-index.html in browser. 2. Press DOWN arrow key till you reach to last focusable element s5. Actual Results : User not able to select the elements s2, s3, s4 although they are focusable nodes and visible to user. Expected Results: User should able to select the elements s2, s3, s4 since they are focusable nodes and visible to user. If two or more focusable nodes are completely intersecting each other so that only one of them is visible to user then preference should be given to the node on top.In case of partial intersection, user should able to select select every node visible to him. [NOTE] : Attached snav-z-index.htm file is modified version of layout test example https://svn.webkit.org/repository/webkit/trunk/LayoutTests/fast/spatial-navigation/snav-z-index.html
Attachments
Test (2.01 KB, text/html)
2013-08-03 11:33 PDT, Abhijeet Kandalkar 		no flags
Details
Updated patch-1 (9.60 KB, patch)
2013-08-04 04:48 PDT, Abhijeet Kandalkar 		no flags
DetailsFormatted DiffDiff
Updated patch-2 (9.54 KB, patch)
2013-08-04 05:29 PDT, Abhijeet Kandalkar 		tonikitoo: review-
tonikitoo: commit-queue-
DetailsFormatted DiffDiff
Updated patch-3 (17.74 KB, patch)
2013-09-26 06:23 PDT, Abhijeet Kandalkar 		beidson: review-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Abhijeet Kandalkar
Comment 1 2013-08-03 11:37:51 PDT
Working on it
Abhijeet Kandalkar
Comment 2 2013-08-04 04:48:27 PDT
Created attachment 208086 [details] Updated patch-1 Added changes to modify existing spatial navigation behavior in case of overlapping nodes. Above patch also fixes the bug https://bugs.webkit.org/show_bug.cgi?id=56938 Needs to add seperate layouttest for 56938.
Abhijeet Kandalkar
Comment 3 2013-08-04 05:29:51 PDT
Created attachment 208088 [details] Updated patch-2 Please find updated patch.
Abhijeet Kandalkar
Comment 4 2013-08-04 05:34:24 PDT
Comment on attachment 208068 [details] Test Please refer test page.
WebKit Commit Bot
Comment 5 2013-08-04 05:35:47 PDT
Attachment 208068 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Antonio Gomes
Comment 6 2013-08-05 05:53:00 PDT
Comment on attachment 208088 [details] Updated patch-2 View in context: https://bugs.webkit.org/attachment.cgi?id=208088&action=review > Source/WebCore/page/SpatialNavigation.cpp:272 > + bool pariallyIntersects = (curRect.intersects(targetRect) && !curRect.contains(targetRect) && !targetRect.contains(curRect)); 'Overlap' reads better. > LayoutTests/fast/spatial-navigation/snav-z-index.html:90 > +Spatial Navigation should prefer focusable elements with absolute positioning over other elements if both elements visible area completely intersects each other. I would replace 'absolute positioning' with higher z-index.
Antonio Gomes
Comment 7 2013-08-05 05:54:17 PDT
Comment on attachment 208088 [details] Updated patch-2 View in context: https://bugs.webkit.org/attachment.cgi?id=208088&action=review > Source/WebCore/page/FocusController.cpp:737 > + if (candidate.rect.contains(closest.rect) || closest.rect.contains(candidate.rect)) { could you explain this new 'if'? > Source/WebCore/page/SpatialNavigation.cpp:275 > + return pariallyIntersects ? (targetRect.x() < curRect.x()): (targetRect.maxX() <= curRect.x()); could you explain these new conditions?
Antonio Gomes
Comment 8 2013-08-05 06:28:45 PDT
If this patch fixes problems in test cases of bug 56938, tests here could be much improved.
Antonio Gomes
Comment 9 2013-08-16 07:25:25 PDT
Comment on attachment 208088 [details] Updated patch-2 unanswered questions. r-
Abhijeet Kandalkar
Comment 10 2013-09-26 06:23:48 PDT
Created attachment 212699 [details] Updated patch-3 > > Source/WebCore/page/FocusController.cpp:737 > > + if (candidate.rect.contains(closest.rect) || closest.rect.contains(candidate.rect)) { > > could you explain this new 'if'? To check whether candidate and closest completely intersect each other. > > Source/WebCore/page/SpatialNavigation.cpp:275 > > + return pariallyIntersects ? (targetRect.x() < curRect.x()): (targetRect.maxX() <= curRect.x()); > > could you explain these new conditions? /* Determine if curRect and targetRect overlap each other */ bool overlap = (curRect.intersects(targetRect) && !curRect.contains(targetRect) && !targetRect.contains(curRect)); /* Previously node is considered as best candidate only if its bounding rect is exactly below/up/left/right with respect to current node and that’s why overlapping nodes were neglected. Current logic checks for overlap and returns proper candidate node .*/ return overlap ? (targetRect.x() < curRect.x()): (targetRect.maxX() <= curRect.x()
Brady Eidson
Comment 11 2016-05-24 22:03:08 PDT
Comment on attachment 212699 [details] Updated patch-3 Assuming that patches for review since 2013 are stale, r-
Note You need to log in before you can comment on or make changes to this bug.