735857 - Handle CSS transforms of fixed backgrounds per spec

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=15833

The spec now says:

"""
Fixed backgrounds are affected by any transform specified for the root element, and not by any other transforms.

Note: Thus an element with a fixed background still acts like a "porthole" into an image that's fixed to the viewport, and transforms on the element affect the porthole, not the background behind it. On the other hand, transforming the root element will still transform everything on the page, rather than everything except for fixed backgrounds.
"""

This is not anything like what Gecko does.  Other browsers all do other things.  If the spec should say something different, I'm happy to change it; otherwise Gecko should change to match the spec.

There are several reftests I wrote for the W3C test suite that could be reused here: <http://hg.csswg.org/test/rev/41a7369b5bf3>.  Gecko fails almost all of them.

Comment 1

[Botond Ballo [:botond]]

Note that since this bug has been filed, the relevant part of the spec has changed [1]. It now says:

"""
Fixed backgrounds on the root element are affected by any transform specified for that element. For all other elements that are effected by a transform (i.e. have a transform applied to them, or to any of their ancestor elements), a value of fixed for the background-attachment property is treated as if it had a value of scroll. The computed value of background-attachment is not affected. 
"""

This is still not what Gecko does, so this bug should remain open, but the behaviour to implement is the one currently spec'd.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=17521

Comment 2

[Botond Ballo [:botond]]

I'm going to take this because our preferred approach for fixing bug 1201894 involves fixing this first.

Comment 3

[Botond Ballo [:botond]]

Here's a first attempt at a patch, pushed to Try. I want to see what all it breaks in terms of existing reftests and such. Even if it doesn't break anything, the patch will still need some cleanup, performance tuning, and a new reftest.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea0ba4052d50

Comment 4

[Botond Ballo [:botond]]

Existing reftests are passing, indicating that this combination of transforms and fixed backgrounds does not currently have test coverage. I'll introduce some as part of my work in this bug.

Comment 5

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 735857 - Factor out a helper function nsLayoutUtils::IsTransformed(). r=mstange

Review commit: https://reviewboard.mozilla.org/r/48773/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48773/

Comment 6

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 735857 - Treat background-attachment:fixed as background-attachment:scroll if it's on a non-root element affected by a transform. r=mstange

Review commit: https://reviewboard.mozilla.org/r/48775/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48775/

Comment 7

[Botond Ballo [:botond]]

The updated patch is cleaned up and includes a reftest. I've limited the "performance tuning" to caching the result of IsTransformed() in nsDisplayBackgroundImage; if you think caching at other call sites is called for, let me know and I can try that.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8745007 [details]
MozReview Request: Bug 735857 - Treat background-attachment:fixed as background-attachment:scroll if it's on a non-root element affected by a transform. r=mstange

https://reviewboard.mozilla.org/r/48775/#review45609

::: layout/base/nsDisplayList.cpp:2368
(Diff revision 1)
>    uint32_t flags = aBuilder->GetBackgroundPaintFlags();
>    nsRect borderArea = nsRect(ToReferenceFrame(), mFrame->GetSize());
>    const nsStyleImageLayers::Layer &layer = mBackgroundStyle->mImage.mLayers[mLayer];
>  
>    nsBackgroundLayerState state =
>      nsCSSRendering::PrepareImageLayer(presContext, mFrame, flags,

We're calling PrepageImageLayer in the nsDisplayBackgroundImage constructor, which calls ComputeImageLayerPositioningArea. Can we make PrepageImageLayer forward the aOutIsTransformedFixed value to its caller? Then we don't need ComputeShouldTreatAsFixed.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8745006 [details]
MozReview Request: Bug 735857 - Factor out a helper function nsLayoutUtils::IsTransformed(). r=mstange

https://reviewboard.mozilla.org/r/48773/#review45611

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8745007 [details]
MozReview Request: Bug 735857 - Treat background-attachment:fixed as background-attachment:scroll if it's on a non-root element affected by a transform. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48775/diff/1-2/

Comment 11

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #8)
> We're calling PrepageImageLayer in the nsDisplayBackgroundImage constructor,
> which calls ComputeImageLayerPositioningArea. Can we make PrepageImageLayer
> forward the aOutIsTransformedFixed value to its caller?

Good idea! Implemented in the updated patch.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8745007 [details]
MozReview Request: Bug 735857 - Treat background-attachment:fixed as background-attachment:scroll if it's on a non-root element affected by a transform. r=mstange

https://reviewboard.mozilla.org/r/48775/#review45621

Looks good.

Comment 13

[Botond Ballo [:botond]]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f8ac8a910cc

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c466ec53dcff
https://hg.mozilla.org/integration/mozilla-inbound/rev/8150c0a47bd7

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c466ec53dcff
https://hg.mozilla.org/mozilla-central/rev/8150c0a47bd7

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8745007 [details]
MozReview Request: Bug 735857 - Treat background-attachment:fixed as background-attachment:scroll if it's on a non-root element affected by a transform. r=mstange

(This approval request applies to both patches together.)

Approval Request Comment
[Feature/regressing bug #]:
  This is technically a behaviour change that implements a CSS 
  spec change [1]. However, the old behaviour was regressed by 
  APZ (bug 1178298), and made worse by paint skipping (bug 
  1253860), so this is effectively a regression fix.

[User impact if declined]:
  If an element with a fixed background is affected by a transform,
  the fixed background moves incorrectly during async scrolling,
  snapping into its proper place after a repaint.

[Describe test coverage new/current, TreeHerder]:
  The patch adds a new reftest, and is passing existing tests.
  On m-c since 2016-04-26.

[Risks and why]:
  Fairly low; the changes are localized and should only impact the
  affected combination of features (fixed background + transform).

[String/UUID change made/needed]:
  None.

I'm not plannig on pursuing a beta uplift because I don't think the affected scenario is common enough to warrant it. If someone disagrees, please let me know.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=17521

Comment 18

[Botond Ballo [:botond]]

Wontfixing for 47 (and below) for now.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8745007 [details]
MozReview Request: Bug 735857 - Treat background-attachment:fixed as background-attachment:scroll if it's on a non-root element affected by a transform. r=mstange

CSS Spec Compliance, patch includes new automated tests, baked in Nightly for a week, Aurora48+

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7b7eabebdbe6
https://hg.mozilla.org/releases/mozilla-aurora/rev/990f98f0a8d6