Bug 97229 – Spatial navigation is not working for anchor with inline div with inline img

• Status: UNCONFIRMED
• Component: DOM

[ssseintr]

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.

[ssseintr]

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

[ssseintr]

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]

Same test case working fine in Opera browser.

[Abhijeet Kandalkar]

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

[Sunesh Chekkiyil]

Created attachment 196321 [details]
Test App

[Sunesh Chekkiyil]

Any update on proposed patch ?

[Abhijeet Kandalkar]

I have patch,will update soon.

[Arunprasad Rajkumar]

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

[Abhijeet Kandalkar]

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]

Sorry for wrong update.

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

[Arunprasad Rajkumar]

(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]

(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]

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]

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]

Created attachment 209285 [details]
Patch 2

Updated patch.

[Antonio Gomes]

(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 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]

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

[Jose Lejin PJ]

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

[Abhijeet Kandalkar]

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]

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 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]

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]

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 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]

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 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]

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]

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 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]

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 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]

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

Assuming that patches for review since 2013 are stale, r-