1264297 - CSS parallax doesn't paint correctly during scroll with APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[keith]

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.

Comment 1

[Loic]

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.

Comment 2

[Markus Stange [:mstange]]

This might be caused by bug 1254260.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

Comment 4

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

47 will not ship with APZ, e10s turned on by default. Tracked for Fx48.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1264297 - Don't do paint-skipping for elements with perspective, until we can properly populate the displayport. r?mstange

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 8

[Markus Stange [:mstange]]

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.

Comment 9

[Markus Stange [:mstange]]

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.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d214625e5bcf

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/d214625e5bcf

Comment 13

[Gerry Chang [:gchang]]

We are not going to ship APZ with 47 so changing status-firefox47 to unaffected

Comment 14

[Gerry Chang [:gchang]]

Since this patch also affects 48, are you also considering to uplift this patch to 48?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

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

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b235305398fd