Status

For bugs in Firefox DevTools, the developer tools within the Firefox web browser. This includes issues about the user interface of the toolbox, special pages such as about:debugging and about:devtools, and developer-related APIs.

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`.

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 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.

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.

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.

> 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.

(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).

(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 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.

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 ?

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.