1174060 - Handle negative delays in the animation inspector UI

Status: VERIFIED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

They make the timeline look weird right now.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: negative-delay.html

STR: 
- With a recent nightly-build, make sure devtools.inspector.animationInspectorV3 is true
- open the attached html page
- open the animation inspector
- click on the big button in the page

==> You'll see 3 animations run, one by one (2 of them are added after some setTimeout delay).
==> https://dl.dropboxusercontent.com/u/714210/negative-delay-before.gif

2 problems:
- the animations added later replace aren't added in the UI, the replace the one already there. That's because they share the same name and the logic we have to determine unique animations in the actor isn't good,
- the animation with a negative delay overflows out of the panel, and the delay UI that we normally use isn't visible.

Now, with the patches I'll attach, you should see this: https://dl.dropboxusercontent.com/u/714210/negative-delay-after.gif

All animations are now visible individually, and the one with the negative delay is fully visible inside the panel.
The way delays look has changed a little bit. There's nothing that visually shows that one delay is negative and the other is positive, but looking at the time, that should be pretty straightforward, also I've added a tooltip with this information in bug 1173761, so I don't think it's important to visually differentiate them.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: 5 - Reverts typo introduced by bug 1180134 that overrided the background-image

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug_1174060_-_1_-_Fix_auto-removed_animations_that.diff

Fix auto-removed animations that have the same name but different targets. Just a quick fix to avoid animations with the same name to replace already displayed animations.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug_1174060_-_2_-_Display_negative_delays_like_nor.diff

This ensure animations with negative delays are fully displayed
in the animation-inspector panel, and that negative and positive
delays look the same.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: Bug_1174060_-_3_-_Place_the_scrubber_correctly_whe.diff

Place the scrubber correctly when new animations are added.
Animations can be added later (after a setTimeout for example), we get events when that happens, and information about the animation that was just added.
We need to use that information to know what is the current documentTimeline time (this data comes with each animation front), in order to position the scrubber correctly again.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: Bug_1174060_-_4_-_Minor_eslint_changes_to_animatio.diff

Some minor eslint cleanup while I was working on these files.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: Bug_1174060_-_5_-_Reverts_typo_introduced_by_bug_1.diff

Very quick review for a one line css change. This is to fix a regression I introduced in bug 1180134 that caused animations with multiple iterations to now display individual iterations, but just one big time-block.
Not a huge deal, the UI is hidden behind a pref, and I realized this while working on this bug, that's why I decided to put the patch here. I don't think this needs to go in with another dedicated bug.

Comment 8

[Patrick Brosset <:pbro>]

Note that parts 1 and 3 need tests, I'm working on those in a separate patch that'll come later.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8656457 [details] [diff] [review]
Bug_1174060_-_2_-_Display_negative_delays_like_nor.diff

Review of attachment 8656457 [details] [diff] [review]:
-----------------------------------------------------------------

Are there tests for this?

::: browser/devtools/animationinspector/components.js
@@ +566,5 @@
>     */
>    addAnimation: function(state) {
>      let {startTime, delay, duration, iterationCount, playbackRate} = state;
>  
> +    this.minStartTime = Math.min(this.minStartTime, startTime + (delay < 0 ? delay / playbackRate : 0));

Nit: 80 chars.  May be good to assign `(delay < 0 ? delay / playbackRate : 0)` or `startTime + (delay < 0 ? delay / playbackRate : 0)` to a variable

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Comment on attachment 8656457 [details] [diff] [review]
> Bug_1174060_-_2_-_Display_negative_delays_like_nor.diff
> 
> Review of attachment 8656457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are there tests for this?
> 
> ::: browser/devtools/animationinspector/components.js
> @@ +566,5 @@
> >     */
> >    addAnimation: function(state) {
> >      let {startTime, delay, duration, iterationCount, playbackRate} = state;
> >  
> > +    this.minStartTime = Math.min(this.minStartTime, startTime + (delay < 0 ? delay / playbackRate : 0));
Not yet, I'm working on tests now. Oh yeah, I mentioned parts 1 and 3 in comment 8, but I'm also adding tests for part 2.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: Bug_1174060_-_6_-_Tests_for_how_delays_are_display.diff

Added tests to ensure negative and positive delays are shown correctly
and that the timeScale window is computed correctly.
Also added a test to ensure that animations with the same name but
different nodes don't override each others in the UI.

This patch also cleans up a lot of exceptions that were thrown while
tests were running. These exceptions were due to pending protocol requests
when tests ended.

Alex: you worked a little bit on cleaning up some animation-inspector test, so I thought you'd be a good person to review this. I've simply generalized the used of the waitForAllAnimationTargets function at 2 key places which got rid of all exceptions I was seeing.

Comment 12

[Patrick Brosset <:pbro>]

Try build for parts 1 to 5: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0addc4359be5

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8656457 [details] [diff] [review]
Bug_1174060_-_2_-_Display_negative_delays_like_nor.diff

Review of attachment 8656457 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/shared/devtools/animationinspector.css
@@ +292,4 @@
>    height: 100%;
> +  background-image: repeating-linear-gradient(45deg,
> +                                              transparent, transparent 1px,
> +                                              white 1px, white 4px);

May want to use var(--theme-selection-color) instead of white to match up with other colors in the UI

Comment 14

[Patrick Brosset <:pbro>]

Attachment: Bug_1174060_-_2_-_Display_negative_delays_like_nor.diff

Thanks Brian. I replaced the css color with a variable.
I also made a minor test changed after detecting failures in earlier try builds.

Comment 15

[Patrick Brosset <:pbro>]

Landing parts 1 to 5.

remote:   https://hg.mozilla.org/integration/fx-team/rev/10a7429d7ed5
remote:   https://hg.mozilla.org/integration/fx-team/rev/688b108cdfe0
remote:   https://hg.mozilla.org/integration/fx-team/rev/ba641c7faa12
remote:   https://hg.mozilla.org/integration/fx-team/rev/0ff170e2d376
remote:   https://hg.mozilla.org/integration/fx-team/rev/ff0093c81879

Part 6 still needs review and a green try build.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8657063 [details] [diff] [review]
Bug_1174060_-_6_-_Tests_for_how_delays_are_display.diff

Review of attachment 8657063 [details] [diff] [review]:
-----------------------------------------------------------------

Some tweaks can be done but looks good.

::: browser/devtools/animationinspector/test/browser_animation_mutations_with_same_names.js
@@ +14,5 @@
> +  let {controller, panel} = yield openAnimationInspectorNewUI();
> +
> +  info("Wait until all animations have been added " +
> +       "(they're added with setTimeout)");
> +  yield new Promise(r => setTimeout(r, 2000));

Isn't there some animation inspector events we could wait?
Like event that is dispatched every time a new animation is added/displayed?

::: browser/devtools/animationinspector/test/browser_animation_playerWidgets_scrubber_enabled.js
@@ -36,5 @@
>  
>    ok(timeline.hasAttribute("disabled"), "The timeline input[range] is disabled");
> -
> -  yield selectNode("body", inspector);
> -  controller.traits.hasSetCurrentTime = true;

Just wondering, why don't you reset the traits anymore?
Is that becuase controller is a new instance for each new inspector panel being opened?

::: browser/devtools/animationinspector/test/browser_animation_timeline_shows_delay.js
@@ +29,5 @@
>  
> +function checkDelayAndName(timelineEl, hasDelay) {
> +  let delay = timelineEl.querySelector(".delay");
> +  let name = timelineEl.querySelector(".name");
> +  let targetNode = timelineEl.querySelector(".target");

You can move `let name` and `let targetNode` within `if (hasDelay)`.

@@ +46,5 @@
> +    // Check that the delay is not displayed on top of the name.
> +    let nameLeft = name.getBoundingClientRect().left;
> +    // Subtracting 1 to account for the border-right width.
> +    ok(delayRect.x + delayRect.width - 1 <= nameLeft,
> +       "The delay element does not span over the name element");

First time I'm seeing a layout assertion!
I get the first assertion, even if the message doesn't really match what it actually asserts.
The first assertion message would be about asserting:
ok(delayRect.x >= delay.parentNode.getBoundingClientReact().x, "...");
I would suggest modifying the assertion message or the check.

The second assertion is weird with this border thing.
getBoundingClientRect includes borders.
I think it highlights that delay do overlap the name.
Actually we can see a very small margin on the left of name if there is no delay. Whereas the is no margin at all and name starts right after the orange border.
It would be great to have the same margin on the left of name.
Also note that delayRect.x+delayRect.width equals delayRect.right

::: browser/devtools/animationinspector/test/doc_negative_animation.html
@@ +13,5 @@
> +    background-image: linear-gradient(to right, #eee 0px, transparent .5px),
> +                      linear-gradient(to right, #aaa 0px, transparent .5px),
> +                      linear-gradient(to right, #000 0px, transparent .5px);
> +    background-repeat: repeat;
> +    background-size: 10px 1px, 50px 1px, 100px 1px;

Do you really need this weird style on the <html> node?

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10a7429d7ed5
https://hg.mozilla.org/mozilla-central/rev/688b108cdfe0
https://hg.mozilla.org/mozilla-central/rev/ba641c7faa12
https://hg.mozilla.org/mozilla-central/rev/0ff170e2d376

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #16)
> browser/devtools/animationinspector/test/
> browser_animation_mutations_with_same_names.js
> @@ +14,5 @@
> > +  let {controller, panel} = yield openAnimationInspectorNewUI();
> > +
> > +  info("Wait until all animations have been added " +
> > +       "(they're added with setTimeout)");
> > +  yield new Promise(r => setTimeout(r, 2000));
> 
> Isn't there some animation inspector events we could wait?
> Like event that is dispatched every time a new animation is added/displayed?
Right. I initially didn't do this because by the time the panel is open, the animations might already have been added. But I could easily change this to only wait for controller.PLAYERS_UPDATED_EVENT events in a loop so long as the animations haven't been added yet.

> browser/devtools/animationinspector/test/
> browser_animation_playerWidgets_scrubber_enabled.js
> @@ -36,5 @@
> >  
> >    ok(timeline.hasAttribute("disabled"), "The timeline input[range] is disabled");
> > -
> > -  yield selectNode("body", inspector);
> > -  controller.traits.hasSetCurrentTime = true;
> 
> Just wondering, why don't you reset the traits anymore?
> Is that becuase controller is a new instance for each new inspector panel
> being opened?
Yes, these 2 lines were useless.

> browser/devtools/animationinspector/test/
> browser_animation_timeline_shows_delay.js
> @@ +29,5 @@
> >  
> > +function checkDelayAndName(timelineEl, hasDelay) {
> > +  let delay = timelineEl.querySelector(".delay");
> > +  let name = timelineEl.querySelector(".name");
> > +  let targetNode = timelineEl.querySelector(".target");
> 
> You can move `let name` and `let targetNode` within `if (hasDelay)`.
Done

> @@ +46,5 @@
> > +    // Check that the delay is not displayed on top of the name.
> > +    let nameLeft = name.getBoundingClientRect().left;
> > +    // Subtracting 1 to account for the border-right width.
> > +    ok(delayRect.x + delayRect.width - 1 <= nameLeft,
> > +       "The delay element does not span over the name element");
> 
> First time I'm seeing a layout assertion!
> I get the first assertion, even if the message doesn't really match what it
> actually asserts.
> The first assertion message would be about asserting:
> ok(delayRect.x >= delay.parentNode.getBoundingClientReact().x, "...");
> I would suggest modifying the assertion message or the check.
Done.

> The second assertion is weird with this border thing.
> getBoundingClientRect includes borders.
> I think it highlights that delay do overlap the name.
My initial though was to avoid being too strict with these tests, knowing that precisely testing the layout is hard, and makes it harder to do subtle css tweaks in the future. Also a first version of the test that was stricter was failing on linux while passing on other platforms. So I thought, ok, let's still have some assertion that things aren't totally misplaced, but let's account for some error margin.
> Actually we can see a very small margin on the left of name if there is no
> delay. Whereas the is no margin at all and name starts right after the
> orange border.
> It would be great to have the same margin on the left of name.
Yeah that's right, good catch. In fact there's 1px difference between the animations with delays and those without. I believe the delay covers up one pixel too much to the left of the name, and this comes from the delay's right border. Really, delays should be displayed with box-sizing:border-box;
> Also note that delayRect.x+delayRect.width equals delayRect.right
Right, somehow I didn't think of this when writing the test.

I'll tweak the way delay elements are positioned and sized so that the test looks less weird.
Thanks for reporting this.

> ::: browser/devtools/animationinspector/test/doc_negative_animation.html
> @@ +13,5 @@
> > +    background-image: linear-gradient(to right, #eee 0px, transparent .5px),
> > +                      linear-gradient(to right, #aaa 0px, transparent .5px),
> > +                      linear-gradient(to right, #000 0px, transparent .5px);
> > +    background-repeat: repeat;
> > +    background-size: 10px 1px, 50px 1px, 100px 1px;
> 
> Do you really need this weird style on the <html> node?
Oh that's a left-over from before when I was using the background as a grid to make sure the animated elements were moving as I expected them to. Will remove this.

Comment 19

[Patrick Brosset <:pbro>]

By the way, last try with part 6 was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=467193ab5721
I'm going to do the changes described in comment 18 and push to try again.

Comment 20

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e3adca7c2a1

Comment 21

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/076c51779626

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/076c51779626

Comment 23

[Vasilica Tamas, QA [:vtamas]]

Attachment: gif1.gif

Verified this issue on Firefox 44 (2015-10-21) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac Os X 10.10.4 and the expected results are accomplished. The animation graphs are displayed as in https://dl.dropboxusercontent.com/u/714210/negative-delay-after.gif


But, I’ve noticed a few potential issue: 

[1].Misplaced animation graphs:
STR: - Click on “Start” button.
     - Click on “Start” button again, before the animation reaches the end.
     - Repeat the previous step if this issue does not reproduce on first try

AR:  - See screenshot: http://i.imgur.com/D81BWul.jpg
     - The following errors are thrown in browser console: 
TypeError: this.player is null animation.js:399:5
<unavailable> protocol.js:907
Protocol error (unknownError): TypeError: player is null animation-panel.js:193



[2]. The timeline does not adapt according to the panel length
STR: - Click on “Start” button
     - Resize the timeline panel while the animation is played.

AR:  - See screenshot: http://i.imgur.com/EMfcAbg.jpg



[3].The animation graphs are changed for several times while hitting Play/Pause button 
STR: - Click on “Start” button.
     - Click on “Play” button.
     - Press “Stop” button.
     - Repeat the previous 2 steps a few times.

AR:  - See attached video(gif1).


Patrick, Should I file a new bugs for the mentioned issues?

Comment 24

[Patrick Brosset <:pbro>]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #23)
Thanks for testing and for the feedback.

> [1].Misplaced animation graphs:
> STR: - Click on “Start” button.
>      - Click on “Start” button again, before the animation reaches the end.
>      - Repeat the previous step if this issue does not reproduce on first try
> 
> AR:  - See screenshot: http://i.imgur.com/D81BWul.jpg
>      - The following errors are thrown in browser console: 
> TypeError: this.player is null animation.js:399:5
> <unavailable> protocol.js:907
> Protocol error (unknownError): TypeError: player is null
> animation-panel.js:193
I couldn't reproduce this error, even clicking multiple times really quickly. So I copied this to a separate bug: bug 1218338.

> [2]. The timeline does not adapt according to the panel length
> STR: - Click on “Start” button
>      - Resize the timeline panel while the animation is played.
> 
> AR:  - See screenshot: http://i.imgur.com/EMfcAbg.jpg
Yup, that's a known limitation, tracked in bug 1171863.

> [3].The animation graphs are changed for several times while hitting
> Play/Pause button 
> STR: - Click on “Start” button.
>      - Click on “Play” button.
>      - Press “Stop” button.
>      - Repeat the previous 2 steps a few times.
> 
> AR:  - See attached video(gif1).
When we play/pause animations, the tool refreshes their states, to make sure to display correct information, and as a result, it re-renders the UI. So we should make sure we don't alter the order of animations when this happens and that we don't alter the relative start times of each animations either, so they appear at the same place every time.
I think that bug 1213651 and bug bug 1202487 should basically fix this, but I'll file a bug with your above STR anyway, just to make sure: bug 1218342.

Comment 25

[Vasilica Tamas, QA [:vtamas]]

Thanks Patrick for your answers!

Based on Comment 23 and Comment 24, I am marking this bug as Verified since the other issues are tracked separately.