With APZ we want to move away from websites doing certain things in scroll event listeners, like moving around elements. This is because scroll events will be delayed relative to the user-visible scrolling, and this will result in laggy behaviour. Milan suggested we log a warning to the console or something visible to content authors so they know what's going on. We should be able to catch this (set up a RAII thing on the stack during scroll event dispatch, and then flag top/left/etc. property mutations while that thing is on the stack) and spit out the warning pretty easily.

Thoughts/suggestions?

FWIW the above patch works for the URLs in bug 1197654, bug 1209058, and bug 1230228. It doesn't work for bug 1203164 (I think they're doing the mutation in a setTimeout or RAF or something) and bug 1201996 (I wasn't sure what to look for here).

Whoops, forgot to add the new files

Comment on attachment 8695523 [details] [diff] [review] WIP Looks good so far. I'd rename mDocument to sDocument, though.

Comment on attachment 8695523 [details] [diff] [review] WIP Review of attachment 8695523 [details] [diff] [review]: ----------------------------------------------------------------- The code looks OK and you can land it but I'm not convinced this will be valuable. There are a lot of these effects and I don't think people are going to stop using them because of this warning. ::: layout/style/nsDOMCSSDeclaration.cpp @@ +80,5 @@ > { > + switch (aPropID) { > + case eCSSProperty_background_position: > + case eCSSProperty_transform: > + // TODO: add other properties that might be used for positioning 'top', 'left' probably

I agree with roc, fwiw.

I figured that in a worst-case scenario we could also reuse this machinery to disable APZ on these sites if we need to. I remember roc proposing that somewhere to deal with sites that perform worse with APZ. I do want to start getting telemetry on how frequently these effects are actually encountered though.

(In reply to Markus Stange [:mstange] from comment #4) > Comment on attachment 8695523 [details] [diff] [review] > WIP > > Looks good so far. I'd rename mDocument to sDocument, though. Why? It's a regular member of the class, not a static. Or are members of stack classes prefixed with 's'?

Oops, I misread. Ignore that. Oh, and I agree that having telemetry for this will be useful

The URL still needs finalizing

Note to self: need to add margin-top/left/right/bottom to the list as well. Facebook uses margin-top (bug 1213073).

The URL in the message may change; I'll talking to MDN folks to figure out what the right place to put it is. The rest of the patch should be ready to review.

Need a data collection peer to sign off on the histogram.

Comment on attachment 8696576 [details] [diff] [review] Part 2 - Telemetry Review of attachment 8696576 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Histograms.json @@ +10062,5 @@ > }, > + "SCROLL_LINKED_EFFECT_FOUND": { > + "alert_emails": ["kgupta@mozilla.com"], > + "bug_numbers": [1229052], > + "description": "Attempt to determine prevalence of scroll-linked effects on the web.", - nit: put description as last field - this is going to log a ton of samples right? how many exactly while scrolling a "regular size" news article? @@ +10063,5 @@ > + "SCROLL_LINKED_EFFECT_FOUND": { > + "alert_emails": ["kgupta@mozilla.com"], > + "bug_numbers": [1229052], > + "description": "Attempt to determine prevalence of scroll-linked effects on the web.", > + "expires_in_version": "55", 1.5 years in the future seems too long, make it version 50? (i.e. 6 months)

> - this is going to log a ton of samples right? how many exactly while scrolling a "regular size" news article? Oops, ignore this, I forgot to delete this comment. I know this logs only 1 sample per nsDocument

https://hg.mozilla.org/integration/mozilla-inbound/rev/53d9dfc575aa https://hg.mozilla.org/integration/mozilla-inbound/rev/f9483881e2f4

The above two patches were to fix a static analysis build error and a non-unified build error (not my code at least), respectively. And I'm going to land a third follow-up to update the warning text based on Chris Mills' suggestion which I forgot. Sorry for the long trail of follow-ups :(

... and I backed them all out because there's *still* non-unified build bustage. I'm going to file another bug for cleaning up the mess in gfx/layers/client and then reland this.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d39e49fe9583

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a805ad24ef5 https://hg.mozilla.org/integration/mozilla-inbound/rev/dc047896bdde

https://hg.mozilla.org/mozilla-central/rev/1a805ad24ef5 https://hg.mozilla.org/mozilla-central/rev/dc047896bdde

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2016/scroll-linked-positioning-effects-may-not-work-well-with-async-scrolling-console-warns/

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > I figured that in a worst-case scenario we could also reuse this machinery > to disable APZ on these sites if we need to. I remember roc proposing that > somewhere to deal with sites that perform worse with APZ. I do want to start > getting telemetry on how frequently these effects are actually encountered > though. I think you never want that, especially given that this machinary is not that effective at all. Basically the current code only works if author is touching those properties via setting element.style.property. If the author uses element.style.setProperty or deleteProperty, then no warning. More complicated, if it is element.style being set directly (e.g. element.style = "top: 10px"), it becomes more expensive to actually detect. Moreover, authors can change a CSS variable and just reference that variable in the corresponding properties, which you probably don't want to detect at all.

I'm looking at this code when I'm implementing CSSOM in stylo. I wonder whether I should add the same code there... but the existing machinary looks so broken that I don't really want to copy the code :/

You don't need to implement this in stylo. We have had the warning in there for a few versions and by the time stylo is in production it will be time to retire the warning anyway. We did decide to not use this code for anything else, and in practice it does seem to catch the majority of the cases - the issues you point out are true but we haven't seen any webpages that use those methods.

Okay, that sounds great! Thanks.