Closed Bug 1406381 Opened 7 years ago Closed 7 years ago

Paused animation with playbackRate -1 sets wrong values

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: kari_pihkala, Assigned: birtles)

Details

Attachments

(6 files, 2 obsolete files)

animate-paused.html
961 bytes, text/html
Details
Bug 1406381 - Tidy up simple-iteration-progress.html and current-iteration.html somewhat;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1406381 - Factor out common code from simple-iteration-progress.html and current-iteration.html;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1406381 - Extend assert_computed_timing_for_each_phase to accommodate negative playback rates;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1406381 - Add tests for simple iteration progress and current iteration when the playback rate is negative;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1406381 - Update the simple iteration progress calculation to match recent changes to the Web Animations specification;
59 bytes, text/x-review-board-request
hiro
: review+
Details
Attached file animate-paused.html — 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171006100327

Steps to reproduce:

I created an animation using the Web Animations API. If the animation is paused, playbackRate is set to -1 (or any other negative value) and fill is "both", then moving the current time to the end causes the animation to jump to the start.

1. Open the attached HTML page
2. Move the slider to the far right



Actual results:

The blue box jumps to the left when the slider is dragged to the far right.


Expected results:

The blue box should stay at the right side.

Google Chrome 61 displays the animation correctly. The box doesn't jump to the left.
Component: Untriaged → DOM: Animation
Product: Firefox → Core
I suspect this is a spec bug where we fail to update the progress from 0 to 1 like we would normally do in the after phase.[1]

[1] https://w3c.github.io/web-animations/#calculating-the-simple-iteration-progress
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Version: 58 Branch → Trunk
For my own notes, I suspect that algorithm should:

* DROP the condition: "the animation effect is in the after phase"
* DROP the condition: "either the active time is not zero or the iteration duration is zero"
* ADD the condition: "the active time is equal to the active-after boundary time"

I *think* that's right but there are all sorts of cases involving negative delays etc. that need to be tested.

Fixing this will require the spec changes, the web-platform-tests for this, and the (trivial) code fix.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
status-firefox57: --- → wontfix
status-firefox58: --- → affected
I've spent most of today working on this. It's hard. There are lots of edge cases involving zero duration intervals. Ultimately, though, I think the current behavior of the model with regards to end delays is wrong. I'll write up a spec issue.
Attached patch Patch (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Test changes (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
      
  


  

    
    

    
      
  


  

        Attachment #8917700 -
        Attachment is obsolete: true

        Attachment #8917701 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8918119 [details]
Bug 1406381 - Tidy up simple-iteration-progress.html and current-iteration.html somewhat;

https://reviewboard.mozilla.org/r/188986/#review194304

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/current-iteration.html:32
(Diff revision 1)
> -      if (typeof currentTest.after !== 'undefined') {
> +      } else if (currentTest.active !== undefined) {
> +        assert_false('Test specifies an active phase iteration to check but the'
> +                     + ' animation has a zero duration');
> +      }

} else {
  assert_equals(currentTest.active, undefined)
}

?

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/current-iteration.html:42
(Diff revision 1)
> +      // After phase
> +      if (anim.effect.getComputedTiming().activeDuration !== Infinity) {
>          anim.finish();
>          assert_equals(anim.effect.getComputedTiming().currentIteration,
>                        currentTest.after);
> +      } else if (currentTest.after !== undefined) {

Likewise above.

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/simple-iteration-progress.html:33
(Diff revision 1)
> +      // Active phase
> +      if (anim.effect.getComputedTiming().activeDuration > 0) {
> -      anim.currentTime = currentTest.input.delay || 0;
> +        anim.currentTime = currentTest.input.delay || 0;
> -      assert_equals(anim.effect.getComputedTiming().progress,
> +        assert_equals(anim.effect.getComputedTiming().progress,
> -                    currentTest.active);
> +                      currentTest.active);
> -      if (typeof currentTest.after !== 'undefined') {
> +      } else if (currentTest.active !== undefined) {

Likewise.

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/simple-iteration-progress.html:43
(Diff revision 1)
> +      // After phase
> +      if (anim.effect.getComputedTiming().activeDuration !== Infinity) {
>          anim.finish();
>          assert_equals(anim.effect.getComputedTiming().progress,
>                        currentTest.after);
> +      } else if (currentTest.after !== undefined) {

Likewise.

        Attachment #8918119 -
        Flags: review?(hikezoe) → review+

    
    

    
  
Comment on attachment 8918120 [details]
Bug 1406381 - Factor out common code from simple-iteration-progress.html and current-iteration.html;

https://reviewboard.mozilla.org/r/188988/#review194310

        Attachment #8918120 -
        Flags: review?(hikezoe) → review+

    
    

    
  
Comment on attachment 8918121 [details]
Bug 1406381 - Extend assert_computed_timing_for_each_phase to accommodate negative playback rates;

https://reviewboard.mozilla.org/r/188990/#review194312

::: testing/web-platform/tests/web-animations/resources/effect-tests.js:41
(Diff revision 1)
>    assert_equals(effect.getComputedTiming()[property], values.before,
>                  `Value of ${property} in the before phase`);
>  
>    // Active phase
>    if (effect.getComputedTiming().activeDuration > 0) {
>      animation.currentTime = effect.getComputedTiming().delay;

We can drop this line?

        Attachment #8918121 -
        Flags: review?(hikezoe) → review+

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Comment on attachment 8918119 [details]
> Bug 1406381 - Tidy up simple-iteration-progress.html and
> current-iteration.html somewhat;
> 
> https://reviewboard.mozilla.org/r/188986/#review194304
> 
> :::
> testing/web-platform/tests/web-animations/timing-model/animation-effects/
> current-iteration.html:32
> (Diff revision 1)
> > -      if (typeof currentTest.after !== 'undefined') {
> > +      } else if (currentTest.active !== undefined) {
> > +        assert_false('Test specifies an active phase iteration to check but the'
> > +                     + ' animation has a zero duration');
> > +      }
> 
> } else {
>   assert_equals(currentTest.active, undefined)
> }
> 
> ?

I'm going to fix this in the second patch since that code moves (and there are fewer places to fix it after the move).

    
    

    
  
Comment on attachment 8918123 [details]
Bug 1406381 - Update the simple iteration progress calculation to match recent changes to the Web Animations specification;

https://reviewboard.mozilla.org/r/188994/#review194324

        Attachment #8918123 -
        Flags: review?(hikezoe) → review+

    
    

    
  
Comment on attachment 8918122 [details]
Bug 1406381 - Add tests for simple iteration progress and current iteration when the playback rate is negative;

https://reviewboard.mozilla.org/r/188992/#review194314

::: testing/web-platform/meta/web-animations/timing-model/animation-effects/current-iteration.html.ini:3
(Diff revision 1)
> +[current-iteration.html]
> +  type: testharness
> +  [Test negative playback rate: duration: 1 delay: 1 fill: both playbackRate: -1]

Oh now I realized that we should use join(', ').

::: testing/web-platform/tests/web-animations/timing-model/animation-effects/simple-iteration-progress.html:570
(Diff revision 1)
> +    input:    { duration: 1,
> +                delay: 1,
> +                fill: 'both' },
> +    playbackRate: -1,
> +    before: 0,
> +    active: 1,

Just to confirm, current gecko returns 0 for the active?

        Attachment #8918122 -
        Flags: review?(hikezoe) → review+

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> Comment on attachment 8918122 [details]
> Bug 1406381 - Add tests for simple iteration progress and current iteration
> when the playback rate is negative;
> 
> https://reviewboard.mozilla.org/r/188992/#review194314
> 
> :::
> testing/web-platform/meta/web-animations/timing-model/animation-effects/
> current-iteration.html.ini:3
> (Diff revision 1)
> > +[current-iteration.html]
> > +  type: testharness
> > +  [Test negative playback rate: duration: 1 delay: 1 fill: both playbackRate: -1]
> 
> Oh now I realized that we should use join(', ').

Yeah, I deliberately didn't do that since I was trying not to change the existing test descriptions in case others browsers have annotated failures based on the test description.

I've generally been taking the approach of changing test descriptions when they're wrong or inconsistent, but otherwise trying not to change them.

Let me see if I can find any annotations in the chromium source.

> :::
> testing/web-platform/tests/web-animations/timing-model/animation-effects/
> simple-iteration-progress.html:570
> (Diff revision 1)
> > +    input:    { duration: 1,
> > +                delay: 1,
> > +                fill: 'both' },
> > +    playbackRate: -1,
> > +    before: 0,
> > +    active: 1,
> 
> Just to confirm, current gecko returns 0 for the active?

Right.

    
    

    
  
(In reply to Brian Birtles (:birtles) from comment #19)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> > Comment on attachment 8918122 [details]
> > Bug 1406381 - Add tests for simple iteration progress and current iteration
> > when the playback rate is negative;
> > 
> > https://reviewboard.mozilla.org/r/188992/#review194314
> > 
> > :::
> > testing/web-platform/meta/web-animations/timing-model/animation-effects/
> > current-iteration.html.ini:3
> > (Diff revision 1)
> > > +[current-iteration.html]
> > > +  type: testharness
> > > +  [Test negative playback rate: duration: 1 delay: 1 fill: both playbackRate: -1]
> > 
> > Oh now I realized that we should use join(', ').
> 
> Yeah, I deliberately didn't do that since I was trying not to change the
> existing test descriptions in case others browsers have annotated failures
> based on the test description.
> 
> I've generally been taking the approach of changing test descriptions when
> they're wrong or inconsistent, but otherwise trying not to change them.
> 
> Let me see if I can find any annotations in the chromium source.

Oops, looks like I already changed the test description by adding a space after ':'.

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/web-animations/timing-model/animation-effects/current-iteration-expected.txt?sq=package:chromium

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05bea3733908
Tidy up simple-iteration-progress.html and current-iteration.html somewhat; r=hiro
https://hg.mozilla.org/integration/autoland/rev/2035e28610da
Factor out common code from simple-iteration-progress.html and current-iteration.html; r=hiro
https://hg.mozilla.org/integration/autoland/rev/7db086f55adb
Extend assert_computed_timing_for_each_phase to accommodate negative playback rates; r=hiro
https://hg.mozilla.org/integration/autoland/rev/bfd71e39abde
Add tests for simple iteration progress and current iteration when the playback rate is negative; r=hiro
https://hg.mozilla.org/integration/autoland/rev/c85fbd16178d
Update the simple iteration progress calculation to match recent changes to the Web Animations specification; r=hiro

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: