Bug 64201

Created attachment 100162 [details] Patch

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 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.

(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

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?

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

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"?

Created attachment 144173 [details] Patch

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 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?

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.

(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?

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

Created attachment 144633 [details] Patch

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.

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.

(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.

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

<rdar://problem/11559084>