Bug 64201 – Composited fixed position layers have an incorrect overlap map entry

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

Composited fixed position layers have an incorrect overlap map entry

[Adrienne Walker]

Created attachment 100162 [details]
Patch

[Adrienne Walker]

smfr: To answer your next question, I'm not seeing this on any real site.  I was trying to repro bug 64010 and found this bug instead.

[Simon Fraser (smfr)]

Comment on attachment 100162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100162&action=review

> Source/WebCore/rendering/RenderLayerCompositor.cpp:559
> +    bool fixed = layer->renderer()->style()->position() == FixedPosition;
> +    IntRect layerBounds = layer->renderer()->localToAbsoluteQuad(FloatRect(layer->localBoundingBox()), fixed).enclosingBoundingBox();

I don't think you need to send 'fixed' in here. RenderBox::mapLocalToContainer() should figure that out.

[Adrienne Walker]

(In reply to comment #3)
> (From update of attachment 100162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100162&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:559
> > +    bool fixed = layer->renderer()->style()->position() == FixedPosition;
> > +    IntRect layerBounds = layer->renderer()->localToAbsoluteQuad(FloatRect(layer->localBoundingBox()), fixed).enclosingBoundingBox();
> 
> I don't think you need to send 'fixed' in here. RenderBox::mapLocalToContainer() should figure that out.

It doesn't.  RenderBox::mapLocalToContainer only sets 'fixed' to true if there's not a transform on the layer.

http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBox.cpp#L1231

[Adrienne Walker]

smfr: Do you feel like this should be solved some other way, such as via a change to RenderBox? Is there somebody else you'd recommend me to CC and talk to about this?

[Eric Seidel (no email)]

Looks like this 6-month old patch has been forgotten?

[Ojan Vafai]

Comment on attachment 100162 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100162&action=review

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:559
>>> +    IntRect layerBounds = layer->renderer()->localToAbsoluteQuad(FloatRect(layer->localBoundingBox()), fixed).enclosingBoundingBox();
>> 
>> I don't think you need to send 'fixed' in here. RenderBox::mapLocalToContainer() should figure that out.
> 
> It doesn't.  RenderBox::mapLocalToContainer only sets 'fixed' to true if there's not a transform on the layer.
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBox.cpp#L1231

What breaks if you change the RenderBox code to "fixed |= fixedPos"?

[Adrienne Walker]

Created attachment 144173 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 144173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144173&action=review

> Source/WebCore/rendering/RenderGeometryMap.cpp:150
> -            inFixed &= currStep->m_isFixedPosition;
> +            inFixed = currStep->m_isFixedPosition;

I still don't think this is right. This should be identical to RenderBox::mapLocalToContainer(). It's:

if (we have a transform)
  if (this layer is fixed)
    treat as fixed
  otherwise
    transform turns off fixed positioning.

This is the part of the css transforms spec that says "transformed act as contains for fixed position".

A testcase would have a position:fixed inside a transformed element.

[Adrienne Walker]

Comment on attachment 144173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144173&action=review

>> Source/WebCore/rendering/RenderGeometryMap.cpp:150
>> +            inFixed = currStep->m_isFixedPosition;
> 
> I still don't think this is right. This should be identical to RenderBox::mapLocalToContainer(). It's:
> 
> if (we have a transform)
>   if (this layer is fixed)
>     treat as fixed
>   otherwise
>     transform turns off fixed positioning.
> 
> This is the part of the css transforms spec that says "transformed act as contains for fixed position".
> 
> A testcase would have a position:fixed inside a transformed element.

Yeah, this definitely needs a few more test cases.

Do you agree that the &= is incorrect?  An initial value of inFixed=false along with &= means that a transformed fixed pos layer will only be treated as fixed if some non-transformed fixed pos layer precedes it, which seems incorrect to me.  Are you suggesting (like ojan) that this should be |= instead?

[Simon Fraser (smfr)]

It should be:

if (currStep-> m_hasTransform) {
  if (!currStep->m_isFixedPosition)
    inFixed = false;
} else
  inFixed |= currStep->m_isFixedPosition;

which I'm pretty sure is equivalent to the &= in the code.

[Adrienne Walker]

(In reply to comment #11)
> It should be:
> 
> if (currStep-> m_hasTransform) {
>   if (!currStep->m_isFixedPosition)
>     inFixed = false;
> } else
>   inFixed |= currStep->m_isFixedPosition;
> 
> which I'm pretty sure is equivalent to the &= in the code.

For a case where all layers that have fixed position also have transforms, how does inFixed ever become true?

[Simon Fraser (smfr)]

We still need the part of your patch that sets isFixed at the start.

[Adrienne Walker]

Created attachment 144633 [details]
Patch

[Darin Adler]

Comment on attachment 144633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144633&action=review

I see no changes in this patch that should have any effect. The new code does the same thing the old code did, just in a slightly different way. Do these tests fail without the bug fix? How do they fail? What exactly was wrong with the old code?

> Source/WebCore/ChangeLog:31
> [phone]  Adrienne Walker  <[e-mail]>
> +
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: compositing/layer-creation/fixed-position-overlap.html
> +
> +        * rendering/RenderGeometryMap.cpp:
> +        (WebCore::RenderGeometryMap::absoluteRect):
> +        (WebCore::RenderGeometryMap::mapToAbsolute):
> +

Doubled change log entry.

> LayoutTests/ChangeLog:32
> [phone]  Adrienne Walker  <[e-mail]>
> +
> +        Composited fixed position layers have an incorrect overlap map entry
> +        https://bugs.webkit.org/show_bug.cgi?id=64201
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * compositing/layer-creation/fixed-position-overlap-expected.txt: Added.
> +        * compositing/layer-creation/fixed-position-overlap.html: Added.
> +

Doubled entry here too.

[Darin Adler]

Let me be more specific. The new code has this structure:

    if (a) {
        if (!b)
            c = false;
    } else if (b)
        c = true;

The old code this:

    if (a)
        c &= b;
    else
        c |= b;

Both of these code snippets do the same thing.

[Adrienne Walker]

(In reply to comment #16)
> Let me be more specific. The new code has this structure:
> 
>     if (a) {
>         if (!b)
>             c = false;
>     } else if (b)
>         c = true;

Your simplification is incorrect.  The new code has the following structure:

if (a && !b)
    c = false;
else if (b)
    c = true;

The difference is that c is set to true if (a && b).

The attached compositing/layer-creation/fixed-position-and-transform.html fails TEXT+IMAGE without the code changes.

[Adrienne Walker]

Committed r118957: <http://trac.webkit.org/changeset/118957>

[Radar WebKit Bug Importer]

<rdar://problem/11559084>