Bug 151401 – Amazon.com Additional Information links aren't clickable

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jiewen Tan]

* 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.

[Radar WebKit Bug Importer]

<rdar://problem/23599527>

[Jiewen Tan]

<rdar://problem/23454261>

[Jiewen Tan]

Created attachment 265826 [details]
Reduced Web Archive

[Jiewen Tan]

Created attachment 265888 [details]
Reduced Web Page

Need to resize the page to reproduce the bug.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 266024 [details]
Bad Case

[Jiewen Tan]

Created attachment 266025 [details]
Good Case

[Jiewen Tan]

The original link has been modified. Here is a replaced one:
http://www.amazon.com/gp/product/B00YD3DBOC/ref=s9_simh_gw_g147_i1_r?pf_rd_m=ATVPDKIKX0DER&pf_rd_s=desktop-3&pf_rd_r=1Y72PMYVMZER122JHWTB&pf_rd_t=36701&pf_rd_p=2084660942&pf_rd_i=desktop

[Jiewen Tan]

Created attachment 266266 [details]
Patch

[Simon Fraser (smfr)]

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).

[Dave Hyatt]

Comment on attachment 266266 [details]
Patch

Seems fine but needs a test.

[Jiewen Tan]

Created attachment 266283 [details]
Patch

[Simon Fraser (smfr)]

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

[zalan]

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.

[Jiewen Tan]

Created attachment 266284 [details]
Patch

[Jiewen Tan]

Created attachment 266285 [details]
Patch

[Darin Adler]

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')");

[Jiewen Tan]

Committed r192854: <http://trac.webkit.org/changeset/192854>