Bug 151401

Summary: Amazon.com Additional Information links aren't clickable
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: Layout and RenderingAssignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bdakin, commit-queue, esprehn+autocc, glenn, hyatt, jiewen_tan, kondapallykalyan, simon.fraser, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Reduced Web Archive
none
Reduced Web Page
none
Bad Case
none
Good Case
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Jiewen Tan 2015-11-18 12:57:07 PST
* SUMMARY

Popular Amazon products have an "Additional Information" section with "Best Sellers Rank" link. These links are not clickable, yet work perfectly fine in Chrome and Firefox on same machine.

* STEPS TO REPRODUCE

1. Visit http://www.amazon.com/gp/product/B00002N66H
(or search for "pruners" and click the first product)
2. Scroll down to "Additional Information" section on right-side
3. Try to click a link

* RESULTS
You can't click on any link. The cursor doesn't change to a hand.
Comment 1 Radar WebKit Bug Importer 2015-11-18 13:25:05 PST
<rdar://problem/23599527>
Comment 2 Jiewen Tan 2015-11-18 13:35:48 PST
<rdar://problem/23454261>
Comment 3 Jiewen Tan 2015-11-18 18:57:12 PST
Created attachment 265826 [details]
Reduced Web Archive
Comment 4 Jiewen Tan 2015-11-19 12:54:12 PST
Created attachment 265888 [details]
Reduced Web Page

Need to resize the page to reproduce the bug.
Comment 5 Jiewen Tan 2015-11-20 18:59:18 PST
First of all, I have reduced the whole amazon webpage into two sample webpages. (Thanks Simon and Zalan.) The first one is the bad.html which reproduces the bug. The second one is the good.html, which displays the same content without the bug. The only difference is that the last div block is deleted in the good.html. Notice, one has to resize the width of the window to reproduce the bug.

Here are the render trees of these two webpages:
good:
(R)elative/A(B)solute/Fi(X)ed/Stick(Y) positioned, (O)verflow clipping, (A)nonymous, (G)enerated, (F)loating, has(L)ayer, (C)omposited, (D)irty layout, Dirty (S)tyle
---G-L- --* RenderView  (0.00, 0.00) (858.00, 833.00) renderer->(0x1172ec540)
-----L- --    HTML RenderBlock  (0.00, 0.00) (858.00, 27.00) renderer->(0x117340678) node->(0x117fc3ea0)
------- --      BODY RenderBody  (8.00, 8.00) (842.00, 0.00) renderer->(0x117340da8) node->(0x117fc3750)
----F-- --        DIV RenderBlock  (0.00, 0.00) (500.00, 18.00) renderer->(0x117340e60) node->(0x117fc37b8)
------- --          RootInlineBox  (0.00, 0.00) (270.55, 18.00) (0x11733e7e0) renderer->(0x117340e60)
------- --            InlineTextBox  (0.00, 0.00) (270.55, 18.00) (0x11739b9c0) run(1, 42) "foobar foobar foobar foobar foobar foobar"
------- --          #text RenderText renderer->(0x117fa0240) node->(0x117374960) length->(42) " foobar foobar foobar foobar foobar foobar"
----F-- --        DIV RenderBlock  (442.00, 0.00) (400.00, 19.00) renderer->(0x1173408a0) node->(0x117fc3820)
------- --          RootInlineBox  (0.00, 0.00) (361.25, 18.00) (0x11733e738) renderer->(0x1173408a0)
------- --            InlineTextBox  (0.00, 0.00) (361.25, 18.00) (0x11739b900) run(0, 57) "Lorem ipsum dolor sit amet, consectetur adipisicing elit."
------- --          #text RenderText renderer->(0x117fa03c0) node->(0x117374a50) length->(57) "Lorem ipsum dolor sit amet, consectetur adipisicing edit."

bad:
(R)elative/A(B)solute/Fi(X)ed/Stick(Y) positioned, (O)verflow clipping, (A)nonymous, (G)enerated, (F)loating, has(L)ayer, (C)omposited, (D)irty layout, Dirty (S)tyle
---G-L- --* RenderView  (0.00, 0.00) (858.00, 833.00) renderer->(0x1172ec540)
-----L- --    HTML RenderBlock  (0.00, 0.00) (858.00, 34.00) renderer->(0x117340678) node->(0x1173f1f70)
------- --      BODY RenderBody  (8.00, 8.00) (842.00, 18.00) renderer->(0x117340da8) node->(0x117338820)
----F-- --        DIV RenderBlock  (0.00, 0.00) (500.00, 18.00) renderer->(0x117340e60) node->(0x117338888)
------- --          RootInlineBox  (0.00, 0.00) (270.55, 18.00) (0x117fd8e70) renderer->(0x117340e60)
------- --            InlineTextBox  (0.00, 0.00) (270.55, 18.00) (0x117fbbc00) run(1, 42) "foobar foobar foobar foobar foobar foobar"
------- --          #text RenderText renderer->(0x117fbb7e0) node->(0x1173f2230) length->(42) " foobar foobar foobar foobar foobar foobar"
----F-- --        DIV RenderBlock  (442.00, 0.00) (400.00, 19.00) renderer->(0x1173408a0) node->(0x1173388f0)
------- --          RootInlineBox  (0.00, 0.00) (361.25, 18.00) (0x117fd8f18) renderer->(0x1173408a0)
------- --            InlineTextBox  (0.00, 0.00) (361.25, 18.00) (0x117fbbae0) run(0, 57) "Lorem ipsum dolor sit amet, consectetur adipisicing elit."
------- --          #text RenderText renderer->(0x117fbb960) node->(0x1173f2f00) length->(57) "Lorem ipsum dolor sit amet, consectetur adipisicing elit."
------- --        DIV RenderBlock  (0.00, 18.00) (0.00, 0.00) renderer->(0x117340958) node->(0x117fc35b0)

A very importance fact here is the BODY renderer of the good one has 0 height while the bad one has 18.00 height. It has a crucial impact on WebCore while it is executing hitTest on them. G stands for good, and B stands for bad afterwards. Here is the stack trace of the two test cases.

RenderBlock::nodeAtPoint
RenderObject::hitTest
RenderLayer::hitTestContents
RenderLayer::hitTestContentsForFragments
RenderLayer::hitTestLayer
RenderLayer::hitTestList
RenderLayer::hitTestLayer
RenderLayer::hitTest
RenderView::hitTest

Things diverge at RenderBlock::nodeAtPoint. They both get to RenderBlock.cpp::l2477 with the renderer as the HTML RenderBlock. However, the hitTestContents will return true for B but false for G. Hence, B will terminate the executation with the result from hitTestContents which is the first DIV RenderBlock. At the meantime, G will return the result from hitTestFloats which is the InlineTextBox of the second DIV RenderBlock. The reason here is since B's BODY RenderBody has height, so WebCore will continue examining whether it hits the content of its children. Finally, WebCore confirms it at RenderObject.cpp::l1748 with the renderer as the first DIV RenderBlock. On the other side, since G's BODY RenderBody has no height, WebCore will return false at RenderBlock.cpp::l2423 without examing BODY's children. That's why we get different results.

A possible fix for this bug could be flipping the execution sequence of RenderBlock.cpp::l2477 and RenderBlock.cpp::l2481, as for the painting system, it will first paint contents than floats. And then for hit test, it should do it reversedly. I am currently running layout tests against my tentative fix. I am hoping to have comments from experts in this area.
Comment 6 Jiewen Tan 2015-11-20 18:59:45 PST
Created attachment 266024 [details]
Bad Case
Comment 7 Jiewen Tan 2015-11-20 19:00:02 PST
Created attachment 266025 [details]
Good Case
Comment 9 Jiewen Tan 2015-11-30 13:11:37 PST
Created attachment 266266 [details]
Patch
Comment 10 Simon Fraser (smfr) 2015-11-30 13:26:42 PST
Comment on attachment 266266 [details]
Patch

r- for lack of test, but I think the change is OK (as long as Dave Hyatt is OK with it).
Comment 11 Dave Hyatt 2015-11-30 14:03:54 PST
Comment on attachment 266266 [details]
Patch

Seems fine but needs a test.
Comment 12 Jiewen Tan 2015-11-30 14:30:15 PST
Created attachment 266283 [details]
Patch
Comment 13 Simon Fraser (smfr) 2015-11-30 14:34:05 PST
Comment on attachment 266283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266283&action=review

> LayoutTests/fast/block/float/hit-test-on-overlapping-floats.html:8
> +            width: 500px;
> +			background-color: silver;

Looks like you have tabs in the file.

> LayoutTests/fast/block/float/hit-test-on-overlapping-floats.html:23
> +            <div id="leftc"> foobar foobar foobar foobar foobar foobar</div>
> +            <div id="rightc">Lorem ipsum dolor sit amet, consectetur adipisicing elit.</div>

You don't need the garbage text in the divs.

> LayoutTests/fast/block/float/hit-test-on-overlapping-floats.html:24
> +            <div style="height:0px; width:0px; background-color: blue; clear:left;"></div>

Why the background color if it's zero size?

> LayoutTests/fast/block/float/hit-test-on-overlapping-floats.html:33
> +            document.write("PASS. Hit Test locates the expected element.")

Don't use document.write. Use js-test-pre.js/js-test-post.js
Comment 14 zalan 2015-11-30 14:51:57 PST
Comment on attachment 266283 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266283&action=review

> Source/WebCore/ChangeLog:9
> +        The cause of this issue is that the hit test function could not locate the expected layer to hit.

What do you mean by layer here? (in WebKit context, layer means RenderLayer)

> Source/WebCore/ChangeLog:12
> +        In this case, the left column is overlapped with the right column. More precisely, the left column
> +        covers the right column from the view of original hit test function. Consequently, all those links
> +        are hidden by the left column. Therefore, those links cannot be clicked. To fix this, the executation

I don't think this is relevant here. The issue here is simply that the painting order is different from the hittest order so we can end up with visible but unreachable content.
Comment 15 Jiewen Tan 2015-11-30 14:53:32 PST
Created attachment 266284 [details]
Patch
Comment 16 Jiewen Tan 2015-11-30 14:59:44 PST
Created attachment 266285 [details]
Patch
Comment 17 Darin Adler 2015-11-30 15:41:46 PST
Comment on attachment 266285 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266285&action=review

Seems clearly correct, unless a CSS expert like Hyatt wants to tell us that it’s the painting that’s wrong.

> LayoutTests/fast/block/float/hit-test-on-overlapping-floats.html:30
> +        var expectedElement = document.getElementById('rightc');
> +        var element = document.elementFromPoint(450, 10);
> +        shouldBe(element, expectedElement);

This is incorrect. It should be written like this:

    shouldBe("document.elementFromPoint(450, 10)", "document.getElementById('rights')");
Comment 18 Jiewen Tan 2015-11-30 18:53:37 PST
Committed r192854: <http://trac.webkit.org/changeset/192854>