Closed Bug 1264297 Opened 8 years ago Closed 8 years ago

CSS parallax doesn't paint correctly during scroll with APZ

Categories

(Core :: Panning and Zooming, defect)

47 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 - wontfix
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: keith, Assigned: kats)

References

(Depends on 1 open bug)

Details

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

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.112 Safari/537.36

Steps to reproduce:

1) Visit: http://keithclark.co.uk/articles/practical-css-parallax/smooth-scroll/
2) Scroll up and down the page noting when the various parallax sections appear. Note: the empty space at the bottom of the page is a known issue and was reported in bug 1198135


Actual results:

Sections of the page don't paint until scrolling has completely finished. Setting `apz.paint_skipping.enabled` to false appears to fix the issue.

This is an issue in 47.0a2 Dev Edition and in the 48.0a1 (2016-04-13) Nightly


Expected results:

Page content should be visible during scroll.
Summary: CSS parallax doesn't paint correctly duing scroll with APZ → CSS parallax doesn't paint correctly during scroll with APZ

It regressed 2 times:

1) Regression 1: painting doesn't "follow" scrolling and takes time.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=eca3a6b94f7934377b099ddb22ae9c91f340cbfc&tochange=9066ef31441959ae15b83d59395ed34d3ed03c0a

Bug 1192910.

2) Regression 2: it's smooth now but blank areas are visible before painting after scrolling.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bf186476cb90f713243dd1d1d195d25de53914aa&tochange=e0b2fffc136004c1f61ec104823bd316cbb5b71f

Bug 1238564 probably.
Blocks: 1192910, 1238564
Status: UNCONFIRMED → NEW
Component: Untriaged → Panning and Zooming
Ever confirmed: true
Flags: needinfo?(mstange)
Flags: needinfo?(bugmail.mozilla)
Keywords: regression
Product: Firefox → Core
This might be caused by bug 1254260.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #2)
> This might be caused by bug 1254260.

Sounds plausible. It does seem like a painting issue, where the painted content doesn't contain the things it should from the displayport.
Depends on: 1254260
Flags: needinfo?(bugmail.mozilla)
Whiteboard: [gfx-noted]
47 will not ship with APZ, e10s turned on by default. Tracked for Fx48.
I notice that the test case in bug 1254260 has reftest attributes in the HTML, is that ready to be used as a reftest? If so I can include it in this patch.
Hmm, that test case looks incomplete. You could turn it into a reftest, but I don't think your patch will make it pass, because your patch relies on content fixing itself up after the scroll.
Attachment #8752898 - Flags: review?(mstange) → review+
Comment on attachment 8752898 [details]
MozReview Request: Bug 1264297 - Don't do paint-skipping for elements with perspective, until we can properly populate the displayport. r?mstange

https://reviewboard.mozilla.org/r/52846/#review49754

It's an unfortunate workaround, but it's better than doing nothing about this.
(In reply to Markus Stange [:mstange] from comment #8)
> Hmm, that test case looks incomplete. You could turn it into a reftest, but
> I don't think your patch will make it pass, because your patch relies on
> content fixing itself up after the scroll.

After some massaging of the test case I was able to turn it into a reftest, but as you said, this patch doesn't make it pass because using reftest-async-scroll-y is an imperfect simulation of what happens when the user scrolls. Specifically setting reftest-async-scroll-y doesn't trigger a repaint whereas that would happen with an actual user scroll, and that's what my patch here relies upon.

I tried modifying the test case to better capture the paint-skipping codepaths but wasn't very successful. I'll post the updated version of the test case to bug 1254260; presumably the correct fix will make it pass.
Assignee: nobody → bugmail.mozilla
https://hg.mozilla.org/mozilla-central/rev/d214625e5bcf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
We are not going to ship APZ with 47 so changing status-firefox47 to unaffected
Since this patch also affects 48, are you also considering to uplift this patch to 48?
Flags: needinfo?(bugmail.mozilla)
e10s and therefore APZ will be user-enableable on 47, so even though it's not a default configuration it's possible that users will enable it. For that, and for consistency with other bugs, I'd like to keep 47 as wontfix rather than unaffected.

And yes, i will request uplift after a few days.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8752898 [details]
MozReview Request: Bug 1264297 - Don't do paint-skipping for elements with perspective, until we can properly populate the displayport. r?mstange

Approval Request Comment
[Feature/regressing bug #]: Various APZ-related changes
[User impact if declined]: On some pages with perspective, while scrolling, elements don't paint properly
[Describe test coverage new/current, TreeHerder]: tested locally
[Risks and why]: pretty low-risk, this just disables an optimization in the case that the element being scrolled has perspective. The removal of the optimization mitigates the problem
[String/UUID change made/needed]: none
Attachment #8752898 - Flags: approval-mozilla-aurora?
Comment on attachment 8752898 [details]
MozReview Request: Bug 1264297 - Don't do paint-skipping for elements with perspective, until we can properly populate the displayport. r?mstange

Improve APZ, taking it.
Attachment #8752898 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.