Skip to content

Commit

Permalink
fix: Change requestNamedAnimationFrame to apply last change per frame…
Browse files Browse the repository at this point in the history
… instead of first (#8799)

## Description
The current implementation of `requestNamedAnimationFrame` prevents
multiple updates on a frame but by disregarding all but the first
request per frame. This throttling behaviour is apparent when playing a
very short video - if a `timeupdate` occurs just before the `ended`
event, the progress bar position on the `timeupdate` is set at say 98%
and 100% from the `ended` is discarded. Although #8633 removed the
throttle from the `ended` handler itself, the throttle and non-throttled
update can still both execute between frames.

## Specific Changes proposed
Changes the implementation to apply only the last callback instead. If
any exist they will be cancelled. There will still be only one update,
but now it's the last.
Updates tests to reflect the changed behaviour. 
Fixes #8782

## Requirements Checklist
- [x] Feature implemented / Bug fixed
- [ ] If necessary, more likely in a feature request than a bug fix
- [x] Change has been verified in an actual browser (Chrome, Firefox,
IE)
  - [x] Unit Tests updated or fixed
  - [ ] Docs/guides updated
- [ ] Example created ([starter template on
JSBin](https://codepen.io/gkatsev/pen/GwZegv?editors=1000#0))
- [x] Has no DOM changes which impact accessiblilty or trigger warnings
(e.g. Chrome issues tab)
  - [x] Has no changes to JSDoc which cause `npm run docs:api` to error
- [ ] Reviewed by Two Core Contributors
  • Loading branch information
mister-ben authored Aug 13, 2024
1 parent 790078b commit e715145
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 10 deletions.
2 changes: 1 addition & 1 deletion src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -1724,7 +1724,7 @@ class Component {
*/
requestNamedAnimationFrame(name, fn) {
if (this.namedRafs_.has(name)) {
return;
this.cancelNamedAnimationFrame(name);
}
this.clearTimersOnDispose_();

Expand Down
18 changes: 9 additions & 9 deletions test/unit/component.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1377,7 +1377,7 @@ QUnit.test('requestNamedAnimationFrame should only allow one raf of a specific n

comp.requestNamedAnimationFrame(name, handlerTwo);

assert.deepEqual(cancelNames, [], 'no named cancels');
assert.deepEqual(cancelNames, ['testing'], 'one handler was cancelled');
assert.equal(comp.namedRafs_.size, 1, 'still only one named raf');
assert.equal(comp.rafIds_.size, 1, 'still only one raf id');

Expand All @@ -1386,16 +1386,16 @@ QUnit.test('requestNamedAnimationFrame should only allow one raf of a specific n
assert.equal(comp.namedRafs_.size, 0, 'we removed a named raf');
assert.equal(comp.rafIds_.size, 0, 'we removed a raf id');
assert.deepEqual(calls, {
one: 1,
two: 0,
one: 0,
two: 1,
three: 0
}, 'only handlerOne was called');
}, 'only handlerTwo was called');

comp.requestNamedAnimationFrame(name, handlerOne);
comp.requestNamedAnimationFrame(name, handlerTwo);
comp.requestNamedAnimationFrame(name, handlerThree);

assert.deepEqual(cancelNames, [], 'no named cancels for testing');
assert.deepEqual(cancelNames, ['testing', 'testing', 'testing'], 'two more cancels');
assert.equal(comp.namedRafs_.size, 1, 'only added one named raf');
assert.equal(comp.rafIds_.size, 1, 'only added one named raf');

Expand All @@ -1404,10 +1404,10 @@ QUnit.test('requestNamedAnimationFrame should only allow one raf of a specific n
assert.equal(comp.namedRafs_.size, 0, 'we removed a named raf');
assert.equal(comp.rafIds_.size, 0, 'we removed a raf id');
assert.deepEqual(calls, {
one: 2,
two: 0,
three: 0
}, 'only the handlerOne called');
one: 0,
two: 1,
three: 1
}, 'now handlerThree has also been called');

window.requestAnimationFrame = oldRAF;
window.cancelAnimationFrame = oldCAF;
Expand Down

0 comments on commit e715145

Please sign in to comment.