1208204 - Pressing space in the animation inspector should toggle play/pause

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

This is a pretty common thing for animation tools, so whenever the panel is focused, pressing space should pause/play if there are animations currently displayed.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

I'm interested in working on this. Can you assign it to me please ? Thanks

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

Review commit: https://reviewboard.mozilla.org/r/34903/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34903/

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

https://reviewboard.mozilla.org/r/34903/#review31533

Looks good. I have made a couple of comments in the code. And this also needs a new test in devtools/client/animationinspector/test/

::: devtools/client/animationinspector/animation-panel.js:145
(Diff revision 1)
> +    document.addEventListener("keydown", this.onKeyDown, false);

I guess you mean `removeEventListener`.

::: devtools/client/animationinspector/animation-panel.js:159
(Diff revision 1)
> +    if (event.keyCode === keyEvent.DOM_VK_SPACE) {

Please add a comment before this `if` explaining what's the expected behavior on <space>.

Also, I think we want to also have <space> pause all animations on the page when there are none in the timeline.
We have 2 modes in the animation-inspector:

- when there are animations in the timeline, then the toolbar controls those animations
- when there are no animations in the timeline (i.e. the element you have selected has no animated children, or there are animations in another iframe, ...), then there's a global toolbar displayed with just a single pause button that controls all animations in the page

So, I think that pressing <space> should detect which mode is current and do the right thing.

If `AnimationsController.animationPlayers` is empty, then call `toggleAll`, otherwise call `playPauseTimeline`.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34903/diff/1-2/

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Hi,

I edited the code to match your comments.
I also made onKeyDown async so we can wait for it in the test.
In the test I'm not simulating keydown explicitly ( with KeyboardEvent ) as it does not provide a way to know when the listener function is over ( or maybe there's a way, and I don't know it).
I call panel.onKeyDown so that I know when it's over to test some assertion.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

https://reviewboard.mozilla.org/r/34903/#review31629

::: devtools/client/animationinspector/animation-panel.js:203
(Diff revisions 1 - 2)
>    playPauseTimeline: function() {

This function, as of now, is only an event handler. With this change, we want to also make it a function we call outside of events.
I suggest making a few cosmetic changes to make this easier to deal with in the future:

```
onTimelinePlayClicked: function() {
  this.playPauseTimeline().catch(e => console.error(e));
},

playPauseTimeline: function() {
  return AnimationsController
    .toggleCurrentAnimations(this.timelineData.isMoving)
    .then(() => this.refreshAnimationsStateAndUI());
},
```

Also add the right jsdoc comments for these 2 functions. And also please do the same for rewindTimeline.
This means that in `initialize`, when we bind the functions, you should replace `playPauseTimeline` with `onTimelinePlayClicked` as well as where the event listeners are added/removed.

This way, it's easier to know which one is called as an event handler, and which other one can be called from anywhere.
The difference is that the event handler one does its own error handling (with the `catch`), while its up to callers of playPauseTimeline to catch promise rejections where/how they want.
In your case, the code in `onKeyDown` should do it.

It's also more consistent with how `onRateChanged` is named. All event handler callbacks should really be named with `on<something>` anyway.

::: devtools/client/animationinspector/test/browser_animation_space_keyboard_shortcuts.js:23
(Diff revision 2)
> +  let keyboardEvent = doc.createEvent("KeyboardEvent");

So, at least for when there are animations in the timeline, I don't think calling onKeyDown directly with this event is required.
When `playPauseTimeline` gets called, that calls `refreshAnimationsStateAndUI` which then calls `refreshAnimationsUI` which emits a `UI_UPDATED_EVENT` event.

So you could simply start listening for this event (`let onUpdated = panel.once(panel.UI_UPDATED_EVENT)`), then simulate an actual keypress event using EventUtils, and then `yield onUpdated`.

Shorter test, and easier to read, I think.
Generally speaking, calling a callback instead of simulating the event isn't usually a problem, we've done it in a few places. It's better still to simulate the event when we can because that way you really simulate what the user would do, and it's especially important to assert that the listeners were added correctly.
In this particular case, it's not a big deal, but still, I think simulating the event would make the test simpler.

::: devtools/client/animationinspector/test/browser_animation_space_keyboard_shortcuts.js:41
(Diff revision 2)
> +  yield onUpdated;

The last 4 lines can be changed to:
`yield selectNode(".animated", inspector);`

Indeed, no need for the separate `animatedNode` since it's only used in this one place, and no need to wait for the `UI_UPDATED_EVENT` here since the promise returned by `selectNode` already does this (via the inspector-updated event).

::: devtools/client/animationinspector/test/browser_animation_space_keyboard_shortcuts.js:53
(Diff revision 2)
> +  info("Select node without animation, simulate spacebar stroke and check" +

Can you please split this into 2 small tests?
One that only tests play/pause in the timeline, and the other that only tests play/payse globally, when no animations are displayed in the timeline.

For the second part, what we really want to test here is that when you press <space> and there is no timeline, then the global pause button is actioned.
We don't really care that the animations are paused, there are already 2 other tests that do this: `browser_animation_toggle_button_toggles_animations.js` on the client, and `browser_animation_playPauseSeveral.js` on the server.

So what I suggest you do here is simulate an actual keyboard event with EventUtils, don't wait for anything, and just check, synchronously, the state of the pause/play button the global toolbar. If it's changed to its play state then fine.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/34903/#review31629

> Can you please split this into 2 small tests?
> One that only tests play/pause in the timeline, and the other that only tests play/payse globally, when no animations are displayed in the timeline.
> 
> For the second part, what we really want to test here is that when you press <space> and there is no timeline, then the global pause button is actioned.
> We don't really care that the animations are paused, there are already 2 other tests that do this: `browser_animation_toggle_button_toggles_animations.js` on the client, and `browser_animation_playPauseSeveral.js` on the server.
> 
> So what I suggest you do here is simulate an actual keyboard event with EventUtils, don't wait for anything, and just check, synchronously, the state of the pause/play button the global toolbar. If it's changed to its play state then fine.

Can't we do that for the first test too ? `browser_animation_timeline_pause_button.js` seems to check if animations are paused.

Comment 8

[Patrick Brosset <:pbro>]

Isn't the button state updated asynchronously in the timeline? I might be wrong, but I think you'd have to wait for the updated event to check this.

I just realized that, anyway, we need to make sure tests never end before protocols requests are done. So if you simulate something that triggers a protocol request (like pausing animations), then you should wait for that to be done, otherwise the test will end before the request is handled, and that can cause issues.
So, in the second test, it wouldn't be good to just check the button state and end the test immediately in fact. Sorry about the confusion.

I still think you should split them in 2, and that you should simulate the events with EventUtils in the first test, because that makes it simpler.

For the second test (the toggleAll one), maybe we don't care as much because we already have another test that calls toggleAll directly? And also because we don't have an event that tells us when that's done.
Not sure, please have a look at let me know what you think is the best solution.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

> Isn't the button state updated asynchronously in the timeline? I might be wrong, but I think you'd have to wait for the updated event to check this.

That's true, I now have a running test with Event.sendKey and yielding on UI_UPDATED_EVENT

> For the second test (the toggleAll one), maybe we don't care as much because we already have another test that calls toggleAll directly? And also because we don't have an event that tells us when that's done.

Ideally, I would add an event on the animation panel :

  toggleAll: Task.async(function*() {
    yield AnimationsController.toggleAll();
    // toggle the class on the button when server call's over,
    // to be consistent with how playResume button works
    this.toggleAllButtonEl.classList.toggle("paused");
    this.emit(this.GLOBAL_STAGE_CHANGES_EVENT); 
  });

I don't know what is your stance on this, and I'm not 100% convinced either.
For now, I managed to have a simplify version of the test, only checking the button state ( class toggling occurs before the call to AnimationsController.toggleAll ). 

If you think it would not be relevant to have such event, the only way I think of testing this is to have this simplest test, and let the actual toggleAll be tested in browser_animation_toggle_button_toggles_animations.js, which calls panel.toggleAll directly.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #9)
> > Isn't the button state updated asynchronously in the timeline? I might be wrong, but I think you'd have to wait for the updated event to check this.
> 
> That's true, I now have a running test with Event.sendKey and yielding on
> UI_UPDATED_EVENT
> 
> > For the second test (the toggleAll one), maybe we don't care as much because we already have another test that calls toggleAll directly? And also because we don't have an event that tells us when that's done.
> 
> Ideally, I would add an event on the animation panel :
> 
>   toggleAll: Task.async(function*() {
>     yield AnimationsController.toggleAll();
>     // toggle the class on the button when server call's over,
>     // to be consistent with how playResume button works
>     this.toggleAllButtonEl.classList.toggle("paused");
>     this.emit(this.GLOBAL_STAGE_CHANGES_EVENT); 
>   });
> 
> I don't know what is your stance on this, and I'm not 100% convinced either.
We do have some rare test-only events for cases where making the test was hard without an event. If we can do without, I'd rather. But I'm not against it if it's the best way to test.
> For now, I managed to have a simplify version of the test, only checking the
> button state ( class toggling occurs before the call to
> AnimationsController.toggleAll ). 
> 
> If you think it would not be relevant to have such event, the only way I
> think of testing this is to have this simplest test, and let the actual
> toggleAll be tested in
> browser_animation_toggle_button_toggles_animations.js, which calls
> panel.toggleAll directly.
Sounds good to me. Let's go with the simple test. Make sure there are no exceptions being thrown when the test ends though (which would happen if the request hasn't completed).

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34903/diff/2-3/

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

https://reviewboard.mozilla.org/r/34903/#review31991

The code changes look great, thanks!
However you must have forgotten to run `hg add` on the 2 new tests: browser_animation_spacebar_toggles_animations.js and browser_animation_spacebar_toggles_node_animations.js or something, because I don't see them in mozreview :(

::: devtools/client/animationinspector/animation-panel.js:56
(Diff revision 3)
> -      "refreshAnimationsUI", "toggleAll", "onTabNavigated",
> +      "onPickerStopped", "refreshAnimationsUI", "onToggleAllClick",

nit: change onToggleAllClick to onToggleAllClicked for better consistency with the other event callback names.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #12)
> Comment on attachment 8719175 [details]
> MozReview Request: Bug 1208204 - Pressing space in the animation inspector
> should toggle play/pause. r?pbrosset
> 
> https://reviewboard.mozilla.org/r/34903/#review31991
> 
> The code changes look great, thanks!
> However you must have forgotten to run `hg add` on the 2 new tests:
> browser_animation_spacebar_toggles_animations.js and
> browser_animation_spacebar_toggles_node_animations.js or something, because
> I don't see them in mozreview :(

Oh sorry about that, I should have check the review before submit it.

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34903/diff/3-4/

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

https://reviewboard.mozilla.org/r/34903/#review31995

This looks good. I think you should remove the requestLongerTimeout though, unless you have a good reason that you document in a comment.
Also, I'm a bit worried about the toggleAll test that doesn't wait for the request to actually be processed before ending. Isn't this test throwing exceptions in the logs when it ends?

::: devtools/client/animationinspector/test/browser_animation_spacebar_toggles_animations.js:7
(Diff revision 4)
> +requestLongerTimeout(2);

Why this? Did you already run this test on try? Did it timeout there?
Looking at the test, I don't think it should be that long, and so I think we should remove this requestLongerTimeout.

::: devtools/client/animationinspector/test/browser_animation_spacebar_toggles_node_animations.js:7
(Diff revision 4)
> +requestLongerTimeout(2);

Why this? Did you already run this test on try? Did it timeout there?
Looking at the test, I don't think it should be that long, and so I think we should remove this requestLongerTimeout.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/34903/#review31995

Something like this ? 

```
44 INFO checking window state
45 INFO Console message: [JavaScript Error: "TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram: "FX_PAGE_LOAD_MS", key: "null"" {file: "resource://gre/modules/TelemetryStopwatch.jsm" line: 297}]
```
or

`###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost`


I don't know how can I wait for toggleAll to end, there is no events I can listen to for this. Could you tell me more about this ?

Comment 17

[Patrick Brosset <:pbro>]

As discussed on IRC, let's add a new event at the end of toggleAll in animation-controller.js that the test can used to know when everything's done.
The TelemetryStopwatch error you're seeing doesn't seem related to your change however, this might be something else.
In any case, it's best to wait for the event anyway.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34903/diff/4-5/

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Pushed to try as the changes are pretty narrowed : https://treeherder.mozilla.org/#/jobs?repo=try&revision=40afa6334a4e

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34903/diff/5-6/

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe from comment #20)
> Comment on attachment 8719175 [details]
> MozReview Request: Bug 1208204 - Pressing space in the animation inspector
> should toggle play/pause. r?pbrosset
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/34903/diff/5-6/

nit ( s/all_animations_toggled/all-animations-toggled ) to be consistent with the other event name

See https://reviewboard.mozilla.org/r/34903/diff/4-6/ to see the proper changes

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

pulse went down during the test. Pushed again : https://treeherder.mozilla.org/#/jobs?repo=try&revision=b291c2485804

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

All the jobs are over, there are some errors but none seems related to this patch

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe from comment #23)
> All the jobs are over, there are some errors but none seems related to this
> patch

I'm going to fast here, you first need to review the patch

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8719175 [details]
MozReview Request: Bug 1208204 - Pressing space in the animation inspector should toggle play/pause. r?pbrosset

https://reviewboard.mozilla.org/r/34903/#review32823

Looks great. Ship it! Only a couple of very minor comments.

::: devtools/client/animationinspector/animation-controller.js:129
(Diff revision 6)
> -
> +  ALL_ANIMATIONS_TOGGLED_EVENT: "all-animations-toggled",

nit: please insert an empty line between the event and the init function.

::: devtools/client/animationinspector/test/browser_animation_spacebar_toggles_node_animations.js:7
(Diff revision 6)
> +// Test that the spacebar key press toggles the play/resume button state.
> +// This test doesn't need to test if animations actually pause/resume
> +// because there's an other test that does this.

Please also say something like:
"There are animations in the test page and since, by default, the <body> node is selected, animations will be displayed in the timeline, so the timeline play/resume button will be displayed"

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1208204.patch

Comment 27

[Patrick Brosset <:pbro>]

Comment on attachment 8722956 [details] [diff] [review]
Bug_1208204.patch

https://hg.mozilla.org/integration/fx-team/rev/9e99b8c9906a

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e99b8c9906a

Comment 29

[Nazir Ahmed Sabbir [:NaSb]]

I've managed to reproduce this bug on Nightly 44.0a1 (2015-09-24) (Build ID : 20150924030231) on Linux, 64 Bit.

This Bug is now verified as fixed on Latest Firefox Beta 47.0b3

Build ID: 20160505125249
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
OS: Linux 3.19.0-58-generic x86-64