Bug 64201

Summary: Composited fixed position layers have an incorrect overlap map entry
Product: WebKit Reporter: Adrienne Walker <enne>
Component: New BugsAssignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, enne, eric, jamesr, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Adrienne Walker 2011-07-08 14:10:56 PDT
Composited fixed position layers have an incorrect overlap map entry
Comment 1 Adrienne Walker 2011-07-08 14:22:10 PDT
Created attachment 100162 [details]
Patch
Comment 2 Adrienne Walker 2011-07-08 14:23:48 PDT
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.
Comment 3 Simon Fraser (smfr) 2011-07-08 14:32:53 PDT
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.
Comment 4 Adrienne Walker 2011-07-08 14:39:19 PDT
(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
Comment 5 Adrienne Walker 2011-07-13 16:02:11 PDT
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?
Comment 6 Eric Seidel (no email) 2012-02-16 14:20:30 PST
Looks like this 6-month old patch has been forgotten?
Comment 7 Ojan Vafai 2012-04-20 18:21:08 PDT
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"?
Comment 8 Adrienne Walker 2012-05-25 17:07:16 PDT
Created attachment 144173 [details]
Patch
Comment 9 Simon Fraser (smfr) 2012-05-25 17:20:00 PDT
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.
Comment 10 Adrienne Walker 2012-05-29 12:19:48 PDT
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?
Comment 11 Simon Fraser (smfr) 2012-05-29 12:35:54 PDT
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.
Comment 12 Adrienne Walker 2012-05-29 12:49:08 PDT
(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?
Comment 13 Simon Fraser (smfr) 2012-05-29 13:10:44 PDT
We still need the part of your patch that sets isFixed at the start.
Comment 14 Adrienne Walker 2012-05-29 15:23:05 PDT
Created attachment 144633 [details]
Patch
Comment 15 Darin Adler 2012-05-29 16:15:36 PDT
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
> +2012-05-25  Adrienne Walker  <enne@google.com>
> +
> +
> +        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
> +2012-05-25  Adrienne Walker  <enne@google.com>
> +
> +        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.
Comment 16 Darin Adler 2012-05-29 16:25:19 PDT
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.
Comment 17 Adrienne Walker 2012-05-29 18:02:33 PDT
(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.
Comment 18 Adrienne Walker 2012-05-30 12:51:50 PDT
Committed r118957: <http://trac.webkit.org/changeset/118957>
Comment 19 Radar WebKit Bug Importer 2012-05-30 12:55:45 PDT
<rdar://problem/11559084>