Bug 74232 - Remove partially implemented per-Element visibility checks from requestAnimationFrame logic
: Remove partially implemented per-Element visibility checks from requestAnimat...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: James Robinson
:
Depends on: 74231
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-09 18:04 PST by James Robinson
Modified: 2012-04-09 04:42 PDT (History)
8 users (show)

See Also:


Attachments
Patch (15.24 KB, patch)
2011-12-09 18:18 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch for landing (15.12 KB, patch)
2012-04-06 19:13 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-12-09 18:04:52 PST
Remove partially implemented per-Element visibility checks from requestAnimationFrame logic
Comment 1 James Robinson 2011-12-09 18:18:51 PST
Created attachment 118676 [details]
Patch
Comment 2 James Robinson 2011-12-09 18:24:12 PST
Here's the thread where some of the details of this element's behavior were debated on public-web-perf:

http://lists.w3.org/Archives/Public/public-web-perf/2011May/0015.html

the part most relevant to this parameter starts around here:
http://lists.w3.org/Archives/Public/public-web-perf/2011May/0030.html

Another issue is that this parameter is very prone to misuse if it were implemented more thoroughly. For example this code:

function animateRight() {
  e.style.left.px += 10; // using imaginary typed CSSOM syntax
  requestAnimationFrame(animateRight, e);
}

will not behave at all the way the author expects if the element starts offscreen - instead of the element smoothly animating into view, it'll never animate at all.

For the record I am not at all opposed to someone implementing a better version of this later down the road. I think if it's going to be done, though, the approach will have to be somewhat different to the one here. I don't think the current code will be much help to such a future endeavor and it is a significant hindrance to other cleanups and bugfixes I have planned in this area.
Comment 3 James Robinson 2011-12-09 18:29:53 PST
To answer one of Chris' questions in another bug: The functionality that we have actually implemented is that we currently do not fire requestAnimationFrame callbacks when the element parameter was supplied if that element's style.display is "none" or if that element is part of the DOM tree. If either of these conditions are manipulated by requestAnimationFrame callbacks within the same document, then the expected thing happens (the callback fires eventually if the element gets a renderer, it doesn't if the element loses its renderer before the associated callback fires).  If either of the conditions change due to a callback running in a *different* document then the firing behavior is buggy and weird (it depends on the order in which we process documents, which is different for Chromium vs WebKit1/2).

The current behavior also means we re-resolve styles after invoking each callback even though it's typically unnecessary and inefficient.
Comment 4 Dean Jackson 2012-04-06 13:28:20 PDT
James, do you want to commit this?
Comment 5 James Robinson 2012-04-06 13:33:35 PDT
I sure do, I'll need to rebase it to ToT and retest it.  Will try to do later today.
Comment 6 James Robinson 2012-04-06 19:13:18 PDT
Created attachment 136119 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-04-09 04:42:33 PDT
Comment on attachment 136119 [details]
Patch for landing

Clearing flags on attachment: 136119

Committed r113573: <http://trac.webkit.org/changeset/113573>
Comment 8 WebKit Review Bot 2012-04-09 04:42:39 PDT
All reviewed patches have been landed.  Closing bug.