Bug 138122

Summary: Transform-style should not kill position:fixed
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dino, rniwa, simon.fraser, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
Standalone testcase
none
Patch
buildbot: commit-queue-
Patch
dino: review+, buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-mountainlion-wk2
none
Archive of layout-test-results from ews104 for mac-mountainlion-wk2 none

Description Simon Fraser (smfr) 2014-10-27 21:25:22 PDT
Created attachment 240523 [details]
Testcase

Attached testase scrolls incorrectly.
Comment 1 Simon Fraser (smfr) 2014-12-09 22:35:16 PST
Created attachment 242989 [details]
Standalone testcase
Comment 2 Simon Fraser (smfr) 2014-12-11 10:49:20 PST
We're generally confused about what hasTransform means. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=27566
Comment 3 Simon Fraser (smfr) 2014-12-11 14:34:22 PST
Created attachment 243147 [details]
Patch
Comment 4 Simon Fraser (smfr) 2014-12-11 14:54:36 PST
Created attachment 243150 [details]
Patch
Comment 5 Dean Jackson 2014-12-11 14:59:50 PST
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)?
Comment 6 Dean Jackson 2014-12-11 15:00:30 PST
Comment on attachment 243150 [details]
Patch

Already reviewed the first one. Although my first comment was wrong.
Comment 7 Dean Jackson 2014-12-11 15:01:02 PST
(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 :|
Comment 8 Simon Fraser (smfr) 2014-12-11 15:04:14 PST
(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.
Comment 9 Simon Fraser (smfr) 2014-12-11 15:25:51 PST
*** Bug 138123 has been marked as a duplicate of this bug. ***
Comment 10 Build Bot 2014-12-11 15:46:06 PST
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
Comment 11 Build Bot 2014-12-11 15:46:08 PST
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
Comment 12 Build Bot 2014-12-11 15:49:55 PST
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
Comment 13 Build Bot 2014-12-11 15:49:58 PST
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
Comment 14 Simon Fraser (smfr) 2014-12-11 17:59:10 PST
https://trac.webkit.org/r177200