Handle CSS transforms of fixed backgrounds per spec

RESOLVED FIXED in Firefox 48

Status

()

enhancement
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: ayg, Assigned: botond)

Tracking

(Depends on 1 bug)

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox47 wontfix, firefox48 fixed, firefox49 fixed)

Details

Attachments

(2 attachments)

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

Comment 1

4 years ago
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
Assignee

Updated

4 years ago
Blocks: 1201894
Assignee

Comment 2

3 years ago
I'm going to take this because our preferred approach for fixing bug 1201894 involves fixing this first.
Assignee: nobody → botond
Assignee

Comment 3

3 years ago
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
Assignee

Comment 4

3 years ago
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.
Assignee

Comment 7

3 years ago
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 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.
Attachment #8745007 - Flags: review?(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
Attachment #8745006 - Flags: review?(mstange) → review+
Assignee

Comment 10

3 years ago
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/
Attachment #8745007 - Flags: review?(mstange)
Assignee

Comment 11

3 years ago
(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 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.
Attachment #8745007 - Flags: review?(mstange) → review+

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c466ec53dcff
https://hg.mozilla.org/mozilla-central/rev/8150c0a47bd7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
Assignee

Updated

3 years ago
Duplicate of this bug: 1201894
Assignee

Comment 17

3 years ago
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
Attachment #8745007 - Flags: approval-mozilla-aurora?
Assignee

Comment 18

3 years ago
Wontfixing for 47 (and below) for now.
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+
Attachment #8745007 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

3 years ago
Duplicate of this bug: 1287703
Assignee

Updated

3 years ago
Depends on: 1296678
Assignee

Updated

3 years ago
Depends on: 1292499
Assignee

Updated

3 years ago
Blocks: 1319892
Depends on: 1352915
You need to log in before you can comment on or make changes to this bug.