Open Bug 1267656 Opened 7 years ago Updated 4 years ago

background-attachment: local rendered incorrectly during async scrolling

Categories

(Core :: Panning and Zooming, defect, P3)

48 Branch
defect

Tracking

()

Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fix-optional
firefox52 --- fix-optional
firefox53 --- fix-optional
firefox54 --- fix-optional

People

(Reporter: cmills, Assigned: tnikkel)

References

(Blocks 1 open bug, )

Details

(Keywords: correctness, regression, Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

I wrote a background-attachment demo to go along with one of my recent MDN Learning Area articles:

http://mdn.github.io/learning-area/css/styling-boxes/backgrounds/background-attachment.html

In latest Nightly/Dev Edition, the boxes with fixed and scroll applied seem to work ok, but the box with local applied doesn't work properly, afaics. The background should scroll as the element content scrolls, but it doesn't — it works the same as background-attachment: scroll.

The demo seems to work correctly in latest Chrome and Firefox release.
Guessing this is apz related based on nightly/dev edition failing, but release working.
Flags: needinfo?(botond)
Confirmed - APZ has regressed the behaviour of background-attachment:local. It looks like this property may need compositor support, similar to how background-attachment:fixed does.
Blocks: apz-desktop
Component: Layout → Panning and Zooming
Flags: needinfo?(botond)
Keywords: correctness
Whiteboard: [gfx-noted]
Summary: background-attachment: local regression? → background-attachment: local rendered incorrectly during async scrolling
It looks like Layout needs to handle layerization of the background image in much the same way as it handles the caret in scrollable textboxes - by applying a scroll clip to the nsDisplayBackgroundImage that is associated with the scroll frame (so that APZ sees the right frame metrics), and by setting the clip of that scroll clip to the background clip region. It'll also need to enlarge the bounds of the display item to the display port of the scroll frame.
Oh, I was wrong about how background-clip interacts with background-attachment: local. The clip isn't fixed to the scroll frame; it's moving along with the contents. This makes things easier because we don't need to create a special scroll clip.
It looks like we should be able to just put the background into the same layer as the scrolled content, except if there is something from the frame's decoration that needs to go in between the background and the content, for example an inset box shadow.
Matt, do you think something like this would work? 1. In nsDisplayBackgroundImage::AppendBackgroundItemsToTop, if aFrame is an nsIScrollableFrame, skip any layers with attachment:local. 2. In ScrollFrameHelper::BuildDisplayList, once the scroll clip and dirty rect have been set up for the scrolled contents, create all the background image display items that we skipped before. 3. afterwards, sort the display list so that the background image display items are in the right order.
Flags: needinfo?(matt.woodrow)
Could we just determine the scrollclip and dirty rect before we create the items for the attachment local items to avoid having to sort the list afterwards?
We also need to know the animated geometry root. Which might change after we've descended into the scroll frame's children due to scroll handoff.
As discussed on irc, it might be easiest to build the items at the normal time, but track them (on the builder?), so we can re-visit them once the correct clip is in place and update the clip/dirty rect.
Flags: needinfo?(matt.woodrow)
Doesn't look like we'll get to this in 47. From comment 8, it sounds like we do have a plan for tackling this, but we need somebody to claim ownership.

I'm also tagging this as a regression from APZ because it works fine without APZ.
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → 48 Branch
Parking with Timothy for now, although he may not have cycles to get to it this month. If anybody else wants to steal it feel free.
Assignee: nobody → tnikkel
Bumping this to fix-optional for 48 since this is a relatively new CSS feature and we haven't run into any actual websites using it or having this problem.
Attachment #8771512 - Flags: review+
In comment 11 some months  ago we didn't know of any examples of this in the real world - has that changed? 
Marking fix-optional for 51, we could still take a fix in early beta 51 (next few weeks) but it doesn't seem crucial.
Looking pretty unlikely we're going to get a fix for this in time for backport to 51/52, but leaving them set to fix-optional just in case.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Bumping this to fix-optional for 48 since this is a relatively new CSS
> feature and we haven't run into any actual websites using it or having this
> problem.

Is this still true, kats?
Flags: needinfo?(bugmail)
Attached patch wipSplinter Review
Here is a work in progress that I got working a long time ago. It includes some stuff for testing (making all scroll frames active all the time). It's probably bitrotten.
(In reply to Andrew Overholt [:overholt] from comment #19)
> Is this still true, kats?

As far as I know, yes. The only person who's reported it that I can recall is the one in bug 1276131, and that was a reduced test case on codepen, not a live website.
Flags: needinfo?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> (In reply to Andrew Overholt [:overholt] from comment #19)
> > Is this still true, kats?
> 
> As far as I know, yes. The only person who's reported it that I can recall
> is the one in bug 1276131, and that was a reduced test case on codepen, not
> a live website.

Thanks, that means I'll keep the fix-optional for 53 and 54.
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
You need to log in before you can comment on or make changes to this bug.