Bug 87036

Summary: RenderInline::absoluteRects does some incorrect layout math
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: aelias, eae, eric, jchaffraix, leviw, ojan, peter, simon.fraser, trchen, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66687    
Attachments:
Description Flags
Patch
none
Patch
none
Test detects code change---unsure how to finish test
none
Patch none

Adam Barth
Reported 2012-05-21 12:02:29 PDT
REGRESSION(88297) Tap shadow is down and to the right on the chrome web store
Attachments
Patch (1.80 KB, patch)
2012-05-21 12:03 PDT, Adam Barth
no flags
Patch (2.44 KB, patch)
2012-05-21 15:49 PDT, Adam Barth
no flags
Test detects code change---unsure how to finish test (3.66 KB, patch)
2012-05-21 18:02 PDT, Adam Barth
no flags
Patch (3.83 KB, patch)
2012-05-22 15:39 PDT, Adam Barth
no flags
Adam Barth
Comment 1 2012-05-21 12:03:55 PDT
Adam Barth
Comment 2 2012-05-21 12:04:35 PDT
trchen fixes this in the OS(ANDROID) branch in https://gerrit-int.chromium.org/#change,3456 Any hints about how to write a test are welcome!
Eric Seidel (no email)
Comment 3 2012-05-21 13:46:19 PDT
Comment on attachment 143072 [details] Patch How do we test this so we don't typo again?
Eric Seidel (no email)
Comment 4 2012-05-21 13:54:14 PDT
Comment on attachment 143072 [details] Patch To test this, you'll need to use something like getClientRects: https://developer.mozilla.org/en/DOM/element.getClientRects You'll need an inline (aka, span) which is split by a block to cause a continuation. That continuation itself will need to be a block, which is not immediately clear to me what cases that is. Presumably this will work: <span>foo<div>bar</div>baz</span> The code was wrong before because it was adding the box (in this case "div")'s size instead of it's location! Which is just wrong. You might have more luck with a more controlled example like this: <span><img style="width: 50px; height: 50px"><div style="width: 75px; height: 200px "></div><img style="width: 50px; height: 50px"></span> Which would allow you to better predict the sizes. Warning: css line-height and the line decent below the <img> tag will effect the total height of the span rects, so they may be 4-8px larger than what you expect. It's also possible that getClientRects doesn't use absoluteRects() (we have several similarly named rect functions) but I think it does.
Eric Seidel (no email)
Comment 5 2012-05-21 13:55:02 PDT
We may need to assist Dr. Barth in creating a test. He's not a frequent contributor to the world or rendering. :)
Tien-Ren Chen
Comment 6 2012-05-21 14:00:54 PDT
Ah ha, peter just talked to me about upstreaming this patch last week. I'll paste the conversation here: On Wed, May 16, 2012 at 5:24 PM, Tien-Ren Chen <trchen@google.com> wrote: > On Wed, May 16, 2012 at 5:19 PM, Tien-Ren Chen <trchen@google.com> wrote: >> On Wed, May 16, 2012 at 2:57 AM, Peter Beverloo <beverloo@google.com> wrote: >>> I've got a question related to this commit of yours: >>> https://gerrit-int.chromium.org/#change,3456 (bug: http://b/issue?id=5050880) >>> >>> Upstream WebKit uses size() instead of locationOffset(), so this would have >>> to be upstreamed. Is this something a layout test could be written for, as >>> it would influence behavior on upstream platforms as well? >> >> I doubt there is any real rendering code using absoluteRects(), as it >> doesn't support CSS transforms. >> Our tap highlighting code used to use absoluteRects(), but it is no >> longer the case. I believe all new code are encouraged to use >> absoluteQuads() instead. > > Ah, find an old thread discussing with <hyatt@apple.com>: > ==== >> > 2. Are we going to deprecate RenderObject::absoluteRects in favor of >> > absoluteQuads anytime soon? As they don't and will never support CSS >> > transformation. We can get rid of a lot of duplicate code by removing >> > those functions. >> Yeah it would be nice to get rid of. Nobody has gotten around to doing it. > ==== > > I don't have time for that currently, but I would say don't bother > with absoluteRects() for now. > >> I guess our goal is to de-fork from upstream. It probably won't hurt >> to revert that change. Or if we do want to upstream it, I think David >> Hyatt has the authority over that code. My opinion is that we should avoid introducing new code that depends on absoluteRect(GestureTapHighlighter is an example). I'm right now working on integrating 84487 with my version of the highlighter in 84936. I would say, leave this bug alone the GestureTapHighlighter is still broken beyond repair. I have multiple test cases that it can produce a horribly wrong highlight. (like relative positioned element)
Adam Barth
Comment 7 2012-05-21 15:00:13 PDT
Either we need to revert this change downstream or we need to land this change upstream. My understanding from Eric is that this patch is correct, which means we should fix this bug by landing this patch. I just need to find a way to test it.
Adam Barth
Comment 8 2012-05-21 15:49:06 PDT
Ojan Vafai
Comment 9 2012-05-21 16:10:03 PDT
Simon might have ideas of how to test this. I'm not too familiar with this code.
Simon Fraser (smfr)
Comment 10 2012-05-21 16:20:46 PDT
Looks like Internals::boundingBox() should hit this code path.
Adam Barth
Comment 11 2012-05-21 16:24:10 PDT
> Looks like Internals::boundingBox() should hit this code path. Thanks. I missed absoluteBoundingBoxRectIgnoringTransforms!
Adam Barth
Comment 12 2012-05-21 18:02:30 PDT
Created attachment 143155 [details] Test detects code change---unsure how to finish test
Adam Barth
Comment 13 2012-05-22 15:39:15 PDT
Eric Seidel (no email)
Comment 14 2012-05-22 15:43:35 PDT
Comment on attachment 143380 [details] Patch Yay! I would have just used js-test-pre, but this is fine.
WebKit Review Bot
Comment 15 2012-05-22 18:38:26 PDT
Comment on attachment 143380 [details] Patch Clearing flags on attachment: 143380 Committed r118094: <http://trac.webkit.org/changeset/118094>
WebKit Review Bot
Comment 16 2012-05-22 18:38:33 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.