Bug 97229

Summary:

Spatial navigation is not working for anchor with inline div with inline img

Product:

WebKit

Reporter:

ssseintr <ssseintr2>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

UNCONFIRMED

Severity:

Normal

CC:

ararunprasad, arurajku, buildbot, commit-queue, deesaha, jose.lejin, kandalkar.abhijeet58, oleg_smirnov, rniwa, simon.fraser, suneshgopinath, tonikitoo, webkit.arunp

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

46905

Attachments:

Description	Flags
spatial navigation is not working.	none
modified example of spatial navigation not working.	none
Test App	none
Patch 1	none
Patch 2	none
Patch 3	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion	none
Updated patch-4	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2	none
Updated patch-5	beidson: review-, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2	none

ssseintr

Reported 2012-09-20 10:01:19 PDT

Just try to open index.html and then press down or up key. Spatial navigation not working between the images. There is a image which is a child node of div node. Div is child of anchor node. I spent some time to track this issue. Spatial navigation only considered the anchor region. It is not considered the image width and height. So, FocusCandidate rect value is incorrect. But, absolute coordinates should be considered the image properties.

Attachments
spatial navigation is not working. (5.04 KB, text/html) 2012-09-20 10:05 PDT, ssseintr	no flags	Details
modified example of spatial navigation not working. (5.52 KB, application/x-bzip) 2012-09-21 06:06 PDT, ssseintr	no flags	Details
Test App (761 bytes, text/html) 2013-04-03 04:02 PDT, Sunesh Chekkiyil	no flags	Details
Patch 1 (14.60 KB, patch) 2013-08-21 10:22 PDT, Abhijeet Kandalkar	no flags	Details Formatted Diff Diff
Patch 2 (14.61 KB, patch) 2013-08-21 10:34 PDT, Abhijeet Kandalkar	no flags	Details Formatted Diff Diff
Patch 3 (16.07 KB, patch) 2013-09-03 07:59 PDT, Abhijeet Kandalkar	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (599.01 KB, application/zip) 2013-09-03 10:15 PDT, Build Bot	no flags	Details
Updated patch-4 (16.93 KB, patch) 2013-09-12 10:46 PDT, Abhijeet Kandalkar	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (511.46 KB, application/zip) 2013-09-12 17:00 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (1.91 MB, application/zip) 2013-09-12 22:15 PDT, Build Bot	no flags	Details
Updated patch-5 (15.00 KB, patch) 2013-09-16 09:06 PDT, Abhijeet Kandalkar	beidson: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (538.57 KB, application/zip) 2013-09-16 09:43 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (469.87 KB, application/zip) 2013-09-16 12:52 PDT, Build Bot	no flags	Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

ssseintr

Comment 1 2012-09-20 10:05:25 PDT

Created attachment 164935 [details] spatial navigation is not working.

ssseintr

Comment 2 2012-09-21 06:06:08 PDT

Created attachment 165125 [details] modified example of spatial navigation not working. Sorry! I wrongly updated the example file. Now, I correctly updated the example.

ssseintr

Comment 3 2012-09-28 06:16:51 PDT

Same test case working fine in Opera browser.

Abhijeet Kandalkar

Comment 4 2013-03-30 12:26:32 PDT

I have tested this scenario,it is reproducible.Can you please assign it to me.

Sunesh Chekkiyil

Comment 5 2013-04-03 04:02:03 PDT

Created attachment 196321 [details] Test App

Sunesh Chekkiyil

Comment 6 2013-04-03 04:03:24 PDT

Any update on proposed patch ?

Abhijeet Kandalkar

Comment 7 2013-05-19 14:50:27 PDT

I have patch,will update soon.

Arunprasad Rajkumar

Comment 8 2013-08-21 02:58:22 PDT

Issue was discussed in https://lists.webkit.org/pipermail/webkit-help/2012-September/003245.html

Abhijeet Kandalkar

Comment 9 2013-08-21 03:05:33 PDT

I have discussed this issue with Antonio, current Spatial Navigation algorithm doesnt take care block element inside inline elements.I am working on it. It is duplicate of https://bugs.webkit.org/show_bug.cgi?id=97229 Please mark it as duplicate.

Abhijeet Kandalkar

Comment 10 2013-08-21 03:07:17 PDT

Sorry for wrong update. It is duplicate of https://bugs.webkit.org/show_bug.cgi?id=119328 Please mark it as duplicate.

Arunprasad Rajkumar

Comment 11 2013-08-21 03:08:16 PDT

(In reply to comment #9) > I have discussed this issue with Antonio, current Spatial Navigation algorithm doesnt take care block element inside inline elements.I am working on it. That is really nice.

Arunprasad Rajkumar

Comment 12 2013-08-21 03:10:15 PDT

(In reply to comment #10) > Sorry for wrong update. > > It is duplicate of https://bugs.webkit.org/show_bug.cgi?id=119328 > Please mark it as duplicate. Thanks, Actually https://bugs.webkit.org/show_bug.cgi?id=119328 is a duplicate of this bug. I will ask Lejin to mark #119328 as duplicate.

Abhijeet Kandalkar

Comment 13 2013-08-21 10:22:58 PDT

Created attachment 209283 [details] Patch 1 Spatial navigation considered only clipped rectangle or bounding box of a node to calculate the best candidate node. But in case of inline elements with blocks as children clipped rectangle or bounding box value is not useful, We need the complete bounding box of such elements to decide best candidate node. However in some use cases clipped rectangle value is more appropriate while in other absolute bounding box should be considered. Patch considered inline with blocks as children’s as special case and use absolute reactance to decide best candidate node

WebKit Commit Bot

Comment 14 2013-08-21 10:25:47 PDT

Attachment 209283 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/spatial-navigation/snav-clipped-overflowed-content-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-clipped-overflowed-content.html', u'LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden.html', u'LayoutTests/fast/spatial-navigation/snav-inline-with-blocks-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-inline-with-blocks.html', u'LayoutTests/fast/spatial-navigation/snav-only-clipped-overflow-content-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-only-clipped-overflow-content.html', u'LayoutTests/fast/spatial-navigation/snav-simple-content-overflow-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-simple-content-overflow.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/SpatialNavigation.cpp']" exit_code: 1 Source/WebCore/page/SpatialNavigation.cpp:331: Missing space before ( in for( [whitespace/parens] [5] Source/WebCore/page/SpatialNavigation.cpp:332: Missing space before ( in if( [whitespace/parens] [5] Source/WebCore/page/SpatialNavigation.cpp:539: Missing space before ( in for( [whitespace/parens] [5] Source/WebCore/page/SpatialNavigation.cpp:540: Missing space before ( in if( [whitespace/parens] [5] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Abhijeet Kandalkar

Comment 15 2013-08-21 10:34:21 PDT

Created attachment 209285 [details] Patch 2 Updated patch.

Antonio Gomes

Comment 16 2013-08-21 10:37:50 PDT

(In reply to comment #15) > Created an attachment (id=209285) [details] > Patch 2 > > Updated patch. +smfr for the bounding box bits.

Simon Fraser (smfr)

Comment 17 2013-08-21 10:46:54 PDT

Comment on attachment 209285 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=209285&action=review > Source/WebCore/page/SpatialNavigation.cpp:340 > + bool blockInsideInline = false; > + if (render->isRenderInline()) { > + for (Node* curr = node->firstChild(); curr; curr = curr->nextSibling()) { > + if (curr->renderer() && curr->renderer()->isRenderBlock()) { > + blockInsideInline = true; > + break; > + } > + } > + } > + > + LayoutRect rect = blockInsideInline ? node->renderer()->absoluteBoundingBoxRect() : node->boundingBox(); > if (rect.isEmpty()) It's weird to jump between nodes and renders like this. Can't this code just use absoluteFocusRingQuads or similar?

Abhijeet Kandalkar

Comment 18 2013-08-24 11:47:02 PDT

absoluteBoundingBoxRect() seems to be better. I tried with absoluteFocusRingQuads() but it doesnt solve purpose.

Jose Lejin PJ

Comment 19 2013-08-25 22:26:48 PDT

*** Bug 119328 has been marked as a duplicate of this bug. ***

Abhijeet Kandalkar

Comment 20 2013-09-03 07:59:28 PDT

Created attachment 210373 [details] Patch 3 Hi Simon, I have updated patch as per your comments, please have look of it. Below 5 test cases are working fine(as per user expectation) by just updating new test results. The new behavior is identical to the TAB key navigation. 1. LayoutTests/fast/spatial-navigation/snav-container-white-space.html 2. LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden.html 3. LayoutTests/fast/spatial-navigation/snav-simple-content-overflow.html 4. LayoutTests/fast/spatial-navigation/snav-clipped-overflowed-content.html 5. LayoutTests/fast/spatial-navigation/snav-only-clipped-overflow-content.html But, below are the 2 test cases are failing. The behavior of these two test cases after updating new test result are not as per user expectations.(Test expectations are failing) 1. LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden.html 2. LayoutTests/fast/spatial-navigation/snav-imagemap-overlapped-areas.html As discussed with you, Can I land the patch, mentioning above 2 failed cases as "Test expectations are failing" . - if Yes, 1. How to mark these 2 test cases as "Test expectations are failing" status. 2. I will raise two separate bug for these failed test cases and fix it as separate issue.

WebKit Commit Bot

Comment 21 2013-09-03 08:02:09 PDT

Attachment 210373 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/spatial-navigation/snav-clipped-overflowed-content-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-clipped-overflowed-content.html', u'LayoutTests/fast/spatial-navigation/snav-container-white-space-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-container-white-space.html', u'LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden.html', u'LayoutTests/fast/spatial-navigation/snav-inline-with-blocks-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-inline-with-blocks.html', u'LayoutTests/fast/spatial-navigation/snav-only-clipped-overflow-content-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-only-clipped-overflow-content.html', u'LayoutTests/fast/spatial-navigation/snav-simple-content-overflow-expected.txt', u'LayoutTests/fast/spatial-navigation/snav-simple-content-overflow.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/SpatialNavigation.cpp']" exit_code: 1 Source/WebCore/page/SpatialNavigation.cpp:45: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 22 2013-09-03 10:15:20 PDT

Comment on attachment 210373 [details] Patch 3 Attachment 210373 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1694283 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html fast/spatial-navigation/snav-div-overflow-scrol-hidden.html

Build Bot

Comment 23 2013-09-03 10:15:24 PDT

Created attachment 210382 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Abhijeet Kandalkar

Comment 24 2013-09-12 10:46:39 PDT

Created attachment 211443 [details] Updated patch-4 Patch for bug https://bugs.webkit.org/show_bug.cgi?id=119468 will fix failing tests.

Build Bot

Comment 25 2013-09-12 17:00:11 PDT

Comment on attachment 211443 [details] Updated patch-4 Attachment 211443 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1855100 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html fast/spatial-navigation/snav-div-overflow-scrol-hidden.html

Build Bot

Comment 26 2013-09-12 17:00:15 PDT

Created attachment 211492 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.4

Build Bot

Comment 27 2013-09-12 22:15:04 PDT

Comment on attachment 211443 [details] Updated patch-4 Attachment 211443 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1883020 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html fast/spatial-navigation/snav-div-overflow-scrol-hidden.html

Build Bot

Comment 28 2013-09-12 22:15:07 PDT

Created attachment 211506 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4

Abhijeet Kandalkar

Comment 29 2013-09-16 09:06:04 PDT

Created attachment 211797 [details] Updated patch-5 Hi Simon, I have modified definition of rectToAbsoluteCoordinates() function as per your comments. This function returns absolute coordinate of node in main root view. We can’t remove this function completely as it is called from many places. So, modifying definition of complete function will bring consistency in Spatial Navigation code. I have uploaded working patch. Please let me know your review commnets.

Build Bot

Comment 30 2013-09-16 09:43:34 PDT

Comment on attachment 211797 [details] Updated patch-5 Attachment 211797 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1869149 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html fast/spatial-navigation/snav-div-overflow-scrol-hidden.html

Build Bot

Comment 31 2013-09-16 09:43:37 PDT

Created attachment 211803 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Build Bot

Comment 32 2013-09-16 12:52:29 PDT

Comment on attachment 211797 [details] Updated patch-5 Attachment 211797 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1917097 New failing tests: fast/spatial-navigation/snav-imagemap-overlapped-areas.html fast/spatial-navigation/snav-div-overflow-scrol-hidden.html

Build Bot

Comment 33 2013-09-16 12:52:33 PDT

Created attachment 211823 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Brady Eidson

Comment 34 2016-05-24 22:02:58 PDT

Comment on attachment 211797 [details] Updated patch-5 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.