Transform-style should not kill position:fixed

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 240523 [details]
Testcase

Attached testase scrolls incorrectly.

[Simon Fraser (smfr)]

Created attachment 242989 [details]
Standalone testcase

[Simon Fraser (smfr)]

We're generally confused about what hasTransform means. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=27566

[Simon Fraser (smfr)]

Created attachment 243147 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 243150 [details]
Patch

[Dean Jackson]

Comment on attachment 243147 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Various bits of rendering code checked RenderObject::hasTransform() for various
> +        reasons. Confusingly, this meant "has transform, or preserve-3d, or perspective".

First sentence sense make no.

> Source/WebCore/rendering/RenderBox.cpp:-1921
> -    bool hasTransform = hasLayer() && layer()->transform();

You're now calling hasTransform() three times in this method. I'm not sure removing the local variable is worth it.

> Source/WebCore/rendering/RenderObject.cpp:1651
> -    return (hasLayer() && downcast<RenderLayerModelObject>(*this).layer()->transform()) || (containerObject && containerObject->style().hasPerspective());
> +    return hasTransform() || (containerObject && containerObject->style().hasPerspective());

Why did we do it this way originally? ie. why were we going to the layer rather than looking at the style (the new way)?

[Dean Jackson]

Comment on attachment 243150 [details]
Patch

Already reviewed the first one. Although my first comment was wrong.

[Dean Jackson]

(In reply to comment #5)
> Comment on attachment 243147 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243147&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Various bits of rendering code checked RenderObject::hasTransform() for various
> > +        reasons. Confusingly, this meant "has transform, or preserve-3d, or perspective".
> 
> First sentence sense make no.

Ignore this comment. I read it again and it makes sense now :|

[Simon Fraser (smfr)]

(In reply to comment #5)
> Comment on attachment 243147 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243147&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Various bits of rendering code checked RenderObject::hasTransform() for various
> > +        reasons. Confusingly, this meant "has transform, or preserve-3d, or perspective".
> 
> First sentence sense make no.
> 
> > Source/WebCore/rendering/RenderBox.cpp:-1921
> > -    bool hasTransform = hasLayer() && layer()->transform();
> 
> You're now calling hasTransform() three times in this method. I'm not sure
> removing the local variable is worth it.

Actually only once. It's called twice in another function, but only once in each code path.

> > Source/WebCore/rendering/RenderObject.cpp:1651
> > -    return (hasLayer() && downcast<RenderLayerModelObject>(*this).layer()->transform()) || (containerObject && containerObject->style().hasPerspective());
> > +    return hasTransform() || (containerObject && containerObject->style().hasPerspective());
> 
> Why did we do it this way originally? ie. why were we going to the layer
> rather than looking at the style (the new way)?

I think we could have done style().hasTransform(), but the hasLayer() check is a bit check, so cheaper.

[Simon Fraser (smfr)]

*** Bug 138123 has been marked as a duplicate of this bug. ***

[Build Bot]

Comment on attachment 243147 [details]
Patch

Attachment 243147 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6226423039655936

New failing tests:
compositing/tiling/rotated-tiled-preserve3d-clamped.html

[Build Bot]

Created attachment 243159 [details]
Archive of layout-test-results from ews106 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 243150 [details]
Patch

Attachment 243150 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5174337514504192

New failing tests:
compositing/tiling/rotated-tiled-preserve3d-clamped.html

[Build Bot]

Created attachment 243160 [details]
Archive of layout-test-results from ews104 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

https://trac.webkit.org/r177200