Closed Bug 1460234 Opened 6 years ago Closed 6 years ago

Final frame of Web Animation sometimes not applied

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: rearnshaw, Assigned: birtles)

Details

Attachments

(3 files)

Test keyframe offset behavior
609 bytes, text/html
Details
CSV of frame offsets to final opacity
1.89 KB, text/csv
Details
Bug 1460234 - Calculate the position in a keyframe segment using double precision;
59 bytes, text/x-review-board-request
hiro
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.117 Safari/537.36

Steps to reproduce:

<div id="A">Hello</div>
<script>
  document.getElementById('A').animate(
    [
      {"offset":0,"opacity":"0"},
      {"offset":0.2,"opacity":"1","easing":"step-end"},
      {"offset":1,"opacity":"0"}
    ],
    {
      duration: 5000,
      fill: 'both',
    }
  );
</script>


Actual results:

"Hello" fades in over 1 second and remains visible permanently.


Expected results:

"Hello" should have disappeared after 5 seconds.
Observed in 59.0.2 and 52.6.0 ESR

Changing the offset of the second keyframe sometimes fixes it. The problem doesn't occur at 0.1, 0.3, 0.5, 0.6, but does at 0.4. Changing the duration seems to have no effect.
That's because 'step-end' is specified there?p
Flags: needinfo?(rearnshaw)
Test that tries increasing offsets and records the computed style at the end of each animation.
Flags: needinfo?(rearnshaw)
Without the 'step-end' the animation behaves as expected, with a linear ease between the last two frames. But it should be possible to have the last frame stepped into.

It's also not consistent, whether the last frame is applied depends on the offset of the second frame. Any value above 0.5 and it works, below 0.5 it might or might not depending on the specific value.
Results of running the keyframe-test.html on 59.0.2

offset,computed opacity
Yes, this looks like a rounding problem. Thanks for the great report.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Version: 59 Branch → Trunk
I started having a quick look into this and it appears that when we do the following calculation in Gecko_GetPositionInSegment:

  double positionInSegment =
    (aProgress - aSegment->mFromKey) / (aSegment->mToKey - aSegment->mFromKey);

with values:

  double positionInSegment = 1.0 - 0.2 / 1.0 - 0.2;

we can end up with a value that is just short of 1.0 (e.g. 0.99999999999).

That's probably at least in part because mFromKey / mToKey are stored as floats.

Then in ComputedTiming's StepTiming function we'll do:

  int32_t step = floor(aPortion * aSteps);

with values:

  int32_t step = floor(0.99999999999 * 1);

and get:

  int32_t step = 0

When mFromKey is 0.3, however, we end up with step = 1.

I'm not sure yet if we should just store mFromKey / mToKey as doubles or if we should do some fudging here.
I've also confirmed that this goes back until at least Firefox 51.
(In reply to Brian Birtles (:birtles) from comment #7)
> I started having a quick look into this and it appears that when we do the
> following calculation in Gecko_GetPositionInSegment:
> 
>   double positionInSegment =
>     (aProgress - aSegment->mFromKey) / (aSegment->mToKey -
> aSegment->mFromKey);
> 
> with values:
> 
>   double positionInSegment = 1.0 - 0.2 / 1.0 - 0.2;
> 
> we can end up with a value that is just short of 1.0 (e.g. 0.99999999999).

Specifically mFromKey is represented as 0.20000000298023223877 and the second 1.0 above is a float, so the subtraction for the denominator takes place in float space while the subtraction for the numerator takes place in double space (since the first 1.0, aProgress, is a double) and we end up with:

   0.79999999701976776123 / 0.80000001192092895508 = 0.99999998137354872974

If we simply cast the second 1.0 to a double (i.e. do both subtractions in double space) the problem goes away.

I'm unsure if that's really sufficient, however, and I wonder if we should make AnimationPropertySegment use doubles anyway.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Hmm, now I wonder if this test should really go in Web Animations instead of CSS timing...
Comment on attachment 8981303 [details]
Bug 1460234 - Calculate the position in a keyframe segment using double precision;

Canceling review while I rewrite the test as a Web Animations test.
Attachment #8981303 - Flags: review?(hikezoe)
Comment on attachment 8981303 [details]
Bug 1460234 - Calculate the position in a keyframe segment using double precision;

https://reviewboard.mozilla.org/r/247412/#review253422

I believe there is no other place having this kind of calculation issues?
Attachment #8981303 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Comment on attachment 8981303 [details]
> Bug 1460234 - Calculate the position in a keyframe segment using double
> precision;
> 
> https://reviewboard.mozilla.org/r/247412/#review253422
> 
> I believe there is no other place having this kind of calculation issues?

There might be. I had a quick look and couldn't see any obvious ones though, so I thought I'd rather wait until we discover them than speculatively add complexity to the code.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f21b5dd70a8
Calculate the position in a keyframe segment using double precision; r=hiro
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11217 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/3f21b5dd70a8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: