Summary: | Transform-style should not kill position:fixed | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | 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 | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=242673 | ||||||||||||||||
Attachments: |
|
Created attachment 242989 [details]
Standalone testcase
We're generally confused about what hasTransform means. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=27566 Created attachment 243147 [details]
Patch
Created attachment 243150 [details]
Patch
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 on attachment 243150 [details]
Patch
Already reviewed the first one. Although my first comment was wrong.
(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 :| (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. *** Bug 138123 has been marked as a duplicate of this bug. *** 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 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 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 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
|
Created attachment 240523 [details] Testcase Attached testase scrolls incorrectly.