Closed Bug 1431573 Opened 2 years ago Closed 2 years ago

React-ify animation controllers

Categories

(DevTools :: Inspector: Animations, enhancement, P2)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 1 obsolete file)

59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
In this bug, will address following controllers.

* Rewind button
* Pause / Resume button
* Pause / Resume by keyboard
* Playback rate chooser
* Scrubber for animation timeline

Also, related components.
* Play time label
* Progress bar for keyframes
Severity: normal → enhancement
Priority: -- → P2
Blocks: 1437730
Comment on attachment 8950409 [details]
Bug 1431573 - Part 0: Make functions in animation.js alphabetize.

https://reviewboard.mozilla.org/r/219636/#review225932
Attachment #8950409 - Flags: review?(gl) → review+
Comment on attachment 8950410 [details]
Bug 1431573 - Part 1: Implement base of animation toolbar.

https://reviewboard.mozilla.org/r/219638/#review225934

::: devtools/client/themes/animation.css:46
(Diff revision 1)
>  #animation-container:not(.animation-detail-visible) .controlled {
>    display: none;
>  }
>  
> +#animation-container .animation-container-splitter {
> +  height: calc(100% - 24px);

All these calc in this stylesheet makes me nervous. I don't think we are doing things right with how we are setting up our layout or handling our overflows if we need to use calc().
Attachment #8950410 - Flags: review?(gl) → review+
Comment on attachment 8950411 [details]
Bug 1431573 - Part 2: Implement pause/resume button.

https://reviewboard.mozilla.org/r/219640/#review225936

::: devtools/client/inspector/animation/animation.js:127
(Diff revision 1)
>  
>      this.inspector = null;
>      this.win = null;
>    }
>  
> +  get state() {

This should be:
get animations

::: devtools/client/inspector/animation/animation.js:128
(Diff revision 1)
>      this.inspector = null;
>      this.win = null;
>    }
>  
> +  get state() {
> +    return this.inspector.store.getState().animations;

It also seems like we do this.inspector.store.getState().animations.animations

::: devtools/client/inspector/animation/components/PauseResumeButton.js:60
(Diff revision 1)
> +        className: "pause-resume-button devtools-button" +
> +                   (isPlaying ? "" : " paused"),
> +        onClick: this.onClick.bind(this),
> +        title: isPlaying ?
> +                 getStr("timeline.resumedButtonTooltip")
> +               :

Move this to the end of the previous line
Attachment #8950411 - Flags: review?(gl) → review+
Comment on attachment 8950412 [details]
Bug 1431573 - Part 3: Implement rewind button.

https://reviewboard.mozilla.org/r/219642/#review225938

::: devtools/client/inspector/animation/components/RewindButton.js:27
(Diff revision 1)
> +
> +    return dom.button(
> +      {
> +        className: "rewind-button devtools-button",
> +        onClick: rewindAnimationsCurrentTime,
> +        text: getStr("timeline.rewindButtonTooltip"),

I think you meant title instead of text
Attachment #8950412 - Flags: review?(gl) → review+
Comment on attachment 8950411 [details]
Bug 1431573 - Part 2: Implement pause/resume button.

https://reviewboard.mozilla.org/r/219640/#review225936

> This should be:
> get animations

Test refers other fields in the state.
Which is better do you think?
1. Leave as it is. 
2. Address your suggest and make another function (getStateForTest?) for testing?
Comment on attachment 8950413 [details]
Bug 1431573 - Part 4: Implement time label.

https://reviewboard.mozilla.org/r/219644/#review226342
Attachment #8950413 - Flags: review?(gl) → review+
Comment on attachment 8950414 [details]
Bug 1431573 - Part 5: Implement playback rate chooser.

https://reviewboard.mozilla.org/r/219646/#review226346
Attachment #8950414 - Flags: review?(gl) → review+
Comment on attachment 8950415 [details]
Bug 1431573 - Part 6: Make summary graph reflect to playback rate.

https://reviewboard.mozilla.org/r/219648/#review226348

::: commit-message-a3d6d:1
(Diff revision 2)
> +Bug 1431573 - Part 6: Make summery graph refrect to playback rate. r?gl

s/summery/summary
s/refrect/reflect
Attachment #8950415 - Flags: review?(gl) → review+
Sorry, I modified patch 5 a bit because sometimes try got fail for playback rate selector.

new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=404e16bb6b3cf9f7643f04d210b532e09d44a0ef
Comment on attachment 8950416 [details]
Bug 1431573 - Part 7: Implement scrubber.

https://reviewboard.mozilla.org/r/219650/#review230088

::: devtools/client/inspector/animation/animation.js:232
(Diff revision 4)
>      return this.inspector && this.inspector.toolbox && this.inspector.sidebar &&
>             this.inspector.toolbox.currentToolId === "inspector" &&
>             this.inspector.sidebar.getCurrentTabID() === "newanimationinspector";
>    }
>  
>    onAnimationsCurrentTimeUpdated(currentTime) {

Add JSDoc for this function.

::: devtools/client/inspector/animation/animation.js:276
(Diff revision 4)
>  
>    selectAnimation(animation) {
>      this.inspector.store.dispatch(updateSelectedAnimation(animation));
>    }
>  
> +  async setAnimationsCurrentTime(currentTime, needRefresh) {

s/needRefresh/shouldRefresh

::: devtools/client/inspector/animation/animation.js:289
(Diff revision 4)
> +      await this.updateAnimations(animations);
> +      await this.updateState([...animations]);
> +    } else if (!this.isCurrentTimeSetting) {
> +      this.isCurrentTimeSetting = true;
> +      await this.animationsFront.setCurrentTimes(animations, currentTime, true);
> +      await this.updateAnimations(animations);

Do we need to do await this.updateState([...animations]); after updateAnimations here?

::: devtools/client/inspector/animation/animation.js:292
(Diff revision 4)
> +      this.isCurrentTimeSetting = true;
> +      await this.animationsFront.setCurrentTimes(animations, currentTime, true);
> +      await this.updateAnimations(animations);
> +    }
> +
> +    this.isCurrentTimeSetting = false;

I think the intention was to say isCurrentTimeSet inside of setting.

::: devtools/client/inspector/animation/animation.js:292
(Diff revision 4)
> +      this.isCurrentTimeSetting = true;
> +      await this.animationsFront.setCurrentTimes(animations, currentTime, true);
> +      await this.updateAnimations(animations);
> +    }
> +
> +    this.isCurrentTimeSetting = false;

Should this be set to false here? If so, do we really need the this.isCurrentTimeSetting check since it will always be false.

::: devtools/client/inspector/animation/components/CurrentTimeScrubberController.js:35
(Diff revision 4)
> +    this.onMouseMove = this.onMouseMove.bind(this);
> +    this.onMouseOut = this.onMouseOut.bind(this);
> +    this.onMouseUp = this.onMouseUp.bind(this);
> +
> +    this.state = {
> +      offset: 0,

Add a comment for this state prop

::: devtools/client/inspector/animation/components/CurrentTimeScrubberController.js:110
(Diff revision 4)
> +    const {
> +      setAnimationsCurrentTime,
> +      timeScale,
> +    } = this.props;
> +
> +    const time = pageX - this.controllerArea.x < 0 ? 0 :

Move 0 : to the next line

::: devtools/client/themes/animation.css:129
(Diff revision 4)
>  }
>  
> +/* Current Time Scrubber */
> +.current-time-scrubber-controller {
> +  cursor: col-resize;
> +  grid-column: 2/3;

2 / 3
Attachment #8950416 - Flags: review?(gl) → review+
Comment on attachment 8950418 [details]
Bug 1431573 - Part 9: Implement toggle pause/resume function by key board.

https://reviewboard.mozilla.org/r/219654/#review231192

::: devtools/client/inspector/animation/components/PauseResumeButton.js:57
(Diff revision 4)
> +
> +  getKeyEventTarget() {
> +    return ReactDOM.findDOMNode(this).closest("#animation-container");
> +  }
> +
>    onClick() {

Rename this function to be onToggleAnimationsPlayState
Attachment #8950418 - Flags: review?(gl) → review+
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=243b5cb724f739b87f9970a3f6fd272c05049458

Also, I appended one more patch part 11 to address as same as bug 1420130, since the UI events came twice.
Comment on attachment 8950419 [details]
Bug 1431573 - Part 10: Reflect to stop animation.

https://reviewboard.mozilla.org/r/219656/#review232258

::: devtools/client/inspector/animation/animation.js:460
(Diff revision 5)
>        this.startAnimationsCurrentTimeTimer();
>      }
>    }
>  }
>  
>  class CurrentTimeTimer {

Move this to its own file.

::: devtools/client/inspector/animation/animation.js:460
(Diff revision 5)
>        this.startAnimationsCurrentTimeTimer();
>      }
>    }
>  }
>  
>  class CurrentTimeTimer {

Add some JSDoc for this class about what it does.

::: devtools/client/inspector/animation/animation.js:476
(Diff revision 5)
>    destroy() {
>      this.stop();
>      this.animationInspector = null;
>    }
>  
> -  next() {
> +  async next() {

Add a JSDoc for what this next() functions does.

::: devtools/client/inspector/animation/utils/utils.js:73
(Diff revision 5)
>  
>    return true;
>  }
>  
>  /**
> + * Check whether there is the animations whose iteration is infinity.

Check whether or not the given list of animations has  an iteration count of infinite.

::: devtools/client/inspector/animation/utils/utils.js:76
(Diff revision 5)
>  
>  /**
> + * Check whether there is the animations whose iteration is infinity.
> + *
> + * @param {Array} animations.
> + * @return {Boolean} true: playing

return {Boolean} true if there is an animation in the  list of animations whose animation iteration count is infinite.

::: devtools/client/inspector/animation/utils/utils.js:78
(Diff revision 5)
> + * Check whether there is the animations whose iteration is infinity.
> + *
> + * @param {Array} animations.
> + * @return {Boolean} true: playing
> + */
> +function hasInfinityAnimation(animations) {

Rename this to isAnimationIterationCountInfinite

::: devtools/client/inspector/animation/utils/utils.js:79
(Diff revision 5)
> + *
> + * @param {Array} animations.
> + * @return {Boolean} true: playing
> + */
> +function hasInfinityAnimation(animations) {
> +  return animations.some(({state}) => state.iterationCount === null);

s/state.iteractionCount === null/!state.iterationCount

::: devtools/client/inspector/animation/utils/utils.js:88
(Diff revision 5)
>   * Check wether the animations are running at least one.
>   *
>   * @param {Array} animations.
>   * @return {Boolean} true: playing
>   */
>  function hasPlayingAnimation(animations) {

Rename this to isAnimationPlaying while we are at it.
Attachment #8950419 - Flags: review?(gl) → review+
Comment on attachment 8956760 [details]
Bug 1431573 - Part 11: Stop UI event propagation.

https://reviewboard.mozilla.org/r/225730/#review232262

::: devtools/client/inspector/animation/components/AnimationDetailHeader.js:21
(Diff revision 1)
>        animation: PropTypes.object.isRequired,
>        setDetailVisibility: PropTypes.func.isRequired,
>      };
>    }
>  
> -  onClick() {
> +  onClick(e) {

Rename all e to event

::: devtools/client/inspector/animation/components/AnimationDetailHeader.js:24
(Diff revision 1)
>    }
>  
> -  onClick() {
> +  onClick(e) {
>      const { setDetailVisibility } = this.props;
>      setDetailVisibility(false);
> +    e.stopPropagation();

Move all event.stopPropagation() to be the first line.
Attachment #8956760 - Flags: review?(gl) → review+
Comment on attachment 8950419 [details]
Bug 1431573 - Part 10: Reflect to stop animation.

https://reviewboard.mozilla.org/r/219656/#review232362

::: devtools/client/inspector/animation/utils/utils.js:78
(Diff revision 5)
> + * Check whether there is the animations whose iteration is infinity.
> + *
> + * @param {Array} animations.
> + * @return {Boolean} true: playing
> + */
> +function hasInfinityAnimation(animations) {

I changed my mind about is vs has. We should rename this to hasAnimationIterationCountInfinite.

I just want us to be very clear about what these functions mean

::: devtools/client/inspector/animation/utils/utils.js:88
(Diff revision 5)
>   * Check wether the animations are running at least one.
>   *
>   * @param {Array} animations.
>   * @return {Boolean} true: playing
>   */
>  function hasPlayingAnimation(animations) {

Changed my mind here as well. Rename this to be hasRunningAnimation.
Comment on attachment 8950417 [details]
Bug 1431573 - Part 8: Implement progress bar in keyframes.

https://reviewboard.mozilla.org/r/219652/#review232264

::: devtools/client/inspector/animation/animation.js:381
(Diff revision 5)
>  
>      return this.simulatedAnimation;
>    }
>  
> +  /**
> +   * Returns simulatable animation for progress by given parameters.

Add that we are simulating the effect timing animation for the keyframes progress bar.

::: devtools/client/inspector/animation/animation.js:390
(Diff revision 5)
> +   * @param {Object} effectTiming
> +   *        e.g. { duration: 1000, fill: "both" }
> +   * @return {Animation}
> +   *         https://drafts.csswg.org/web-animations/#the-animation-interface
> +   */
> +  simulateAnimationForProgress(effectTiming) {

Rename this to be simulateAnimationForKeyframesProgressBar. This is a bit verbose of a name, but this looks like its only use case.

::: devtools/client/inspector/animation/components/KeyframesProgressBar.js:11
(Diff revision 5)
> +
> +const { PureComponent } = require("devtools/client/shared/vendor/react");
> +const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> +
> +class KeyframesProgressBar extends PureComponent {

I think we should just merge KeyframesProgressBar and KeyframesProgressBarController and just rename the entire component to KeyframesProgressBar

::: devtools/client/inspector/animation/components/KeyframesProgressBarController.js:32
(Diff revision 5)
> +    super(props);
> +
> +    this.onCurrentTimeUpdated = this.onCurrentTimeUpdated.bind(this);
> +
> +    this.state = {
> +      offset: 0,

Add a comment for this state offset variable

::: devtools/client/themes/animation.css:372
(Diff revision 5)
>    padding: 0;
>  }
>  
>  /* Keyframes Progress Tick List */
>  .keyframes-progress-tick-list {
> -  margin-right: var(--graph-right-offset);
> +  grid-column: 2/3;

2 / 3

::: devtools/client/themes/animation.css:392
(Diff revision 5)
>  }
>  
> +/* Keyframes Progress Bar */
> +.keyframes-progress-bar-controller {
> +  background: none;
> +  grid-column: 2/3;

2 / 3
Attachment #8950417 - Flags: review?(gl) → review+
Comment on attachment 8950420 [details]
Bug 1431573 - Part 12: Add tests.

https://reviewboard.mozilla.org/r/219658/#review232388

::: devtools/client/inspector/animation/test/browser_animation_animation-toolbar.js:10
(Diff revision 6)
> +
> +// Test for following AnimationToolbar component:
> +// * element existence
> +// * controller components existence in this toolbar
> +
> +add_task(async function () {

I am not quite sure such a test for existence is really useful. We can really do this when we test the  individual components like clicking the resume/pause button and seeing what it does. I would say we should remove this test.
Attachment #8950420 - Flags: review?(gl) → review+
Attachment #8956760 - Attachment is obsolete: true
Oh, I don't know why but the part 11 had been obsolete and replaced as new patch...
Gabriel, unfortunately, could you give r+ again?

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c556ba1a8f4614698673847e77ee972baff56b7

Now, one of tests ( browser_animation_pause-resume-button_spacebar.js ) is failing on Linux x64 debug.
I will re-check the test code and talos, then if not found any issues, consider to add requestLongerTimeout.
Flags: needinfo?(gl)
Comment on attachment 8958042 [details]
Bug 1431573 - Part 11: Stop UI event propagation.

https://reviewboard.mozilla.org/r/226978/#review232794
Attachment #8958042 - Flags: review?(gl) → review+
I changed the document from doc_multi_timing.html to doc_custom_playback_rate.html for browser_animation_pause-resume-button_spacebar.js. The different point is number of animations in the doc. custom_playback_rate has only 2 animations, but multi_timing has about 20 animations. It can reduce the time to draw.
This is the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ad2ac32e9f113879dd13252485c096079f7c3bf
There are no problem for new tests.

So, I replaced more 2 tests (browser_animation_pause-resume-button.js and browser_animation_rewind-button.js) to custom_playback_rate. Since those tests don't refer the content of doc in detail.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33f1bbfb0d486acd32436291b59f814f9922efbb

If no problem in the try, I'll land the patches.
Thanks.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58f0a60706bb
Part 0: Make functions in animation.js alphabetize. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/3289205e71d3
Part 1: Implement base of animation toolbar. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8451579b688
Part 2: Implement pause/resume button. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c4e18b5b6f
Part 3: Implement rewind button. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/81d305202ac8
Part 4: Implement time label. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/fed660a901cc
Part 5: Implement playback rate chooser. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0a73bb0cfc
Part 6: Make summary graph reflect to playback rate. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9608940304f
Part 7: Implement scrubber. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad8466e2be32
Part 8: Implement progress bar in keyframes. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/059e8cc0247c
Part 9: Implement toggle pause/resume function by key board. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03f28aca55e
Part 10: Reflect to stop animation. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/54cc969c883a
Part 11: Stop UI event propagation. r=gl
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf659bbe2d46
Part 12: Add tests. r=gl
Could you please have a look at this push for devtools failures on devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel.js?

Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cf659bbe2d465d02b3dc26e0960c4c85b76742c7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=167616682

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=167616682&repo=mozilla-inbound&lineNumber=1799

Brasstacks link: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1445291

Log snippet:

01:20:11     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel.js | Should have opened the editor. - 
01:20:11     INFO - Stack trace:
01:20:11     INFO -     chrome://mochitests/content/browser/devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel.js:testRefocusingOnClick:177
01:20:11     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:928:23
01:20:11     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
01:20:11     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:11
01:20:11     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
01:20:11     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:713:7
01:20:11     INFO -     onPacket/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1339:9
01:20:11     INFO -     DevTools RDP*request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1279:14
01:20:11     INFO -     generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1433:14
01:20:11     INFO -     updateBoxModel/lastRequest<@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/boxmodel/box-model.js:162:34
01:20:11     INFO -     _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
01:20:11     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:928:23
01:20:11     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
01:20:11     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:11
01:20:11     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
01:20:11     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:713:7
01:20:11     INFO -     onPacket/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1339:9
01:20:11     INFO -     DevTools RDP*request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1279:14
01:20:11     INFO -     generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1433:14
01:20:11     INFO -     updateBoxModel/lastRequest<@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/boxmodel/box-model.js:151:38
01:20:11     INFO -     _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:310:39
01:20:11     INFO -     promise callback*_handleResultValue@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:386:7
01:20:11     INFO -     _run@resource://devtools/shared/base-loader.js -> resource://devtools/shared/task.js:318:13
Flags: needinfo?(dakatsuka)
Hi Raul,
I think, this was fixed in bug 1445151.
Flags: needinfo?(dakatsuka)
:daisuke hi, can you land a fix for: https://bugzilla.mozilla.org/show_bug.cgi?id=1445452 ?
Started to become a permafailure on autoland. Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168042782&repo=autoland&lineNumber=4397
Flags: needinfo?(dakatsuka)
Andreea, I apologize for any inconvenience this may cause.
Now, I'm investigating about this.
If could not resolve the cause today, will disable this test once.
Thanks.
Flags: needinfo?(dakatsuka)
Daisuke: thank you!
Flags: needinfo?(gl)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.