Bug 87036 - RenderInline::absoluteRects does some incorrect layout math
Summary: RenderInline::absoluteRects does some incorrect layout math
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-05-21 12:02 PDT by Adam Barth
Modified: 2012-05-22 18:38 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.80 KB, patch)
2012-05-21 12:03 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2012-05-21 15:49 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Test detects code change---unsure how to finish test (3.66 KB, patch)
2012-05-21 18:02 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2012-05-22 15:39 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-05-21 12:02:29 PDT
REGRESSION(88297) Tap shadow is down and to the right on the chrome web store
Comment 1 Adam Barth 2012-05-21 12:03:55 PDT
Created attachment 143072 [details]
Patch
Comment 2 Adam Barth 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!
Comment 3 Eric Seidel (no email) 2012-05-21 13:46:19 PDT
Comment on attachment 143072 [details]
Patch

How do we test this so we don't typo again?
Comment 4 Eric Seidel (no email) 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.
Comment 5 Eric Seidel (no email) 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. :)
Comment 6 Tien-Ren Chen 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)
Comment 7 Adam Barth 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.
Comment 8 Adam Barth 2012-05-21 15:49:06 PDT
Created attachment 143118 [details]
Patch
Comment 9 Ojan Vafai 2012-05-21 16:10:03 PDT
Simon might have ideas of how to test this. I'm not too familiar with this code.
Comment 10 Simon Fraser (smfr) 2012-05-21 16:20:46 PDT
Looks like Internals::boundingBox() should hit this code path.
Comment 11 Adam Barth 2012-05-21 16:24:10 PDT
> Looks like Internals::boundingBox() should hit this code path.

Thanks.  I missed absoluteBoundingBoxRectIgnoringTransforms!
Comment 12 Adam Barth 2012-05-21 18:02:30 PDT
Created attachment 143155 [details]
Test detects code change---unsure how to finish test
Comment 13 Adam Barth 2012-05-22 15:39:15 PDT
Created attachment 143380 [details]
Patch
Comment 14 Eric Seidel (no email) 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.
Comment 15 WebKit Review Bot 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>
Comment 16 WebKit Review Bot 2012-05-22 18:38:33 PDT
All reviewed patches have been landed.  Closing bug.