Bug 87036 – RenderInline::absoluteRects does some incorrect layout math

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

REGRESSION(88297) Tap shadow is down and to the right on the chrome web store

[Adam Barth]

Created attachment 143072 [details]
Patch

[Adam Barth]

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 on attachment 143072 [details]
Patch

How do we test this so we don't typo again?

[Eric Seidel (no email)]

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

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]

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 <[e-mail]> wrote:
> On Wed, May 16, 2012 at 5:19 PM, Tien-Ren Chen <[e-mail]> wrote:
>> On Wed, May 16, 2012 at 2:57 AM, Peter Beverloo <[e-mail]> 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 <[e-mail]>:
> ====
>> > 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]

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]

Created attachment 143118 [details]
Patch

[Ojan Vafai]

Simon might have ideas of how to test this. I'm not too familiar with this code.

[Simon Fraser (smfr)]

Looks like Internals::boundingBox() should hit this code path.

[Adam Barth]

> Looks like Internals::boundingBox() should hit this code path.

Thanks.  I missed absoluteBoundingBoxRectIgnoringTransforms!

[Adam Barth]

Created attachment 143155 [details]
Test detects code change---unsure how to finish test

[Adam Barth]

Created attachment 143380 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 143380 [details]
Patch

Yay!  I would have just used js-test-pre, but this is fine.

[WebKit Review Bot]

Comment on attachment 143380 [details]
Patch

Clearing flags on attachment: 143380

Committed r118094: <http://trac.webkit.org/changeset/118094>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.