Summary: | Amazon.com Additional Information links aren't clickable | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Jiewen Tan
2015-11-18 12:57:07 PST
Created attachment 265826 [details]
Reduced Web Archive
Created attachment 265888 [details]
Reduced Web Page
Need to resize the page to reproduce the bug.
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. Created attachment 266024 [details]
Bad Case
Created attachment 266025 [details]
Good Case
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 Created attachment 266266 [details]
Patch
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 on attachment 266266 [details]
Patch
Seems fine but needs a test.
Created attachment 266283 [details]
Patch
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 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. Created attachment 266284 [details]
Patch
Created attachment 266285 [details]
Patch
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')"); Committed r192854: <http://trac.webkit.org/changeset/192854> |