Closed Bug 1229052 Opened 9 years ago Closed 9 years ago

Log a warning if webpage is updating positioning properties during a scroll event listener

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(2 files, 4 obsolete files)

Part 1 - Log a console message
12.80 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 2 - Telemetry
2.03 KB, patch
vladan
: review+
Details | Diff | Splinter Review 
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.
Assignee: nobody → bugmail.mozilla
Attached patch WIP (obsolete) — — Splinter Review 
Thoughts/suggestions?
Attachment #8695521 - Flags: feedback?(roc)
Attachment #8695521 - Flags: feedback?(mstange)
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).
Attached patch WIP (obsolete) — — Splinter Review 
Whoops, forgot to add the new files
Attachment #8695521 - Attachment is obsolete: true
Attachment #8695521 - Flags: feedback?(roc)
Attachment #8695521 - Flags: feedback?(mstange)
Attachment #8695523 - Flags: feedback?(roc)
Attachment #8695523 - Flags: feedback?(mstange)
Comment on attachment 8695523 [details] [diff] [review]
WIP

Looks good so far. I'd rename mDocument to sDocument, though.
Attachment #8695523 - Flags: feedback?(mstange) → feedback+
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
Attachment #8695523 - Flags: feedback?(roc) → feedback+
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
Attached patch Part 1 - Log a console message (obsolete) — — Splinter Review 
The URL still needs finalizing
Attachment #8695523 - Attachment is obsolete: true
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.
Attachment #8695953 - Attachment is obsolete: true
Attachment #8696574 - Flags: review?(roc)
Need a data collection peer to sign off on the histogram.
Attachment #8695954 - Attachment is obsolete: true
Attachment #8696576 - Flags: review?(vladan.bugzilla)
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)
Attachment #8696576 - Flags: review?(vladan.bugzilla) → review+
> - 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
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.
https://hg.mozilla.org/mozilla-central/rev/1a805ad24ef5
https://hg.mozilla.org/mozilla-central/rev/dc047896bdde
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: