Closed Bug 1253494 Opened 8 years ago Closed 8 years ago

Represent endDelay in the animation inspector

Categories

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

defect

Tracking

(firefox47 affected, firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: birtles, Assigned: ryo)

References

()

Details

(Whiteboard: [btpp-fix-later])

Attachments

(4 files)

Once endDelay is implemented (bug 1244635, due to land any day now) we should find a way of representing that in the animation inspector.

This probably doesn't really need to block shipping Element.animate but I'm marking it blocking for now in order to keep track of all the things we'd *like* to cover before we ship.
Thanks for filing this Brian. I'm assuming this is only ever going to be possible for script-generated animations, right?

In order to support this in devtools, at least these 2 files will need to be updated:

- devtools\client\animationinspector\utils.js
there's a helper in here: TimeScale that's responsible for calculating the width of an animation in the timeline and return the width of individual iterations and delays.

- devtools\client\animationinspector\components\animation-time-block.js
this is where the DOM elements for representing one animation in the timeline are created, right now just one delay element is created.
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Oh, and the AnimationPlayerActor in devtools\server\actors\animation.js will also need to be updated so endDelay is sent as part of the current state.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #1)
> Thanks for filing this Brian. I'm assuming this is only ever going to be
> possible for script-generated animations, right?

That's right.

Thanks for the detailed explanation of what needs to happen. Ryo here is going to take a look at this.

One tricky part is it's possible to have a negative endDelay, even one that is greater than the length of the animation itself. It's a pretty rare case (in fact, use of endDelay at all is expected to be very rare!) but it has the effect of cutting off part of the animation. I'm not sure how to represent that. We could just show the cut-off animation, but I think from a debugging point of view it would be useful to know *why* it got cut-off.

So, for example, I could imagine one way of representing it would be make the cut-off part translucent but I'm not sure if that really fits with DevTools style. Ryo is going to work on a couple of screenshots to see how we might show this. Perhaps we could overlay the cross-hatch pattern on top of the animation somehow?
I made some screenshots expressing negative endDelay.
Please give me an advice which pattern is suit to express negative endDelay.
Attached image Overlap example
(In reply to Ryo Motozawa [:ryo] from comment #4)
> Created attachment 8728265 [details]
> Screenshots for negative enddelay
> 
> I made some screenshots expressing negative endDelay.
> Please give me an advice which pattern is suit to express negative endDelay.

Motozawa-san, thank you for preparing these different examples. In future, please just attach the screenshots as a jpg/png (you can combine them together in one image to make it less to attach, and easier to compare).

Also, it is a little bit hard to compare these without seeing positive endDelay.

If it is negative endDelay, I would expect the lightning bolt icon to be at the end of the green bar?

I suggest the pattern we discussed this morning is probably the most clear:

* endDelay looks like normal delay but slants in the opposite direction
* When endDelay is negative is is translucent
* Because endDelay slants in the opposite direction to regular delay if the two ever overlap (very rare), it is clear what is happening

Something like this picture.
Assignee: nobody → motoryo1
Status: NEW → ASSIGNED
Attachment #8729345 - Flags: review?(pbrosset)
Attachment #8729346 - Flags: review?(pbrosset)
Sorry for the delay, I have been away on a work week last week. I will start reviewing these patches now.
Attachment #8729345 - Flags: review?(pbrosset)
Comment on attachment 8729345 [details]
MozReview Request: Bug 1253494 - Part1 Implement endDelay representation in the animation inspector r=pbro

https://reviewboard.mozilla.org/r/39391/#review37835

This seems to work really well, thanks!

I just noticed one thing that we may potentially want to fix:
if you have a 5s animation with a -2s endDelay, then after 3 seconds, the animation reaches its end state and stops, right?
In the timeline however, you can see the scrubber continues to move for the remaining 2seconds.
I'm not sure this is a bug btw, I think we just need to choose what should happen in this case. As Brian said this is going to be pretty rare anyway.
I like the fact that you can see the whole 5s animation in the timeline, and the endDelay covering it up partially. I'm just not sure if the scrubber should progress all the way through the displayed animation.

::: devtools/client/animationinspector/components/animation-time-block.js:106
(Diff revision 1)
> +    // endDelay
> +    if (state.endDelay) {
> +      createNode({
> +        parent: this.containerEl,
> +        attributes: {
> +          "class": "endDelay" + (state.endDelay < 0 ? " negative" : ""),

Please use `end-delay` instead of `endDelay` for the CSS classname on this element.

::: devtools/client/animationinspector/utils.js:202
(Diff revision 1)
> +    let arrangedDelay = delay / playbackRate;
> +    let arrangedDuration = (duration / playbackRate) *

Not sure about the word "arranged" here. Maybe "rateRelative" instead? But that's not great either.

Another option would be to make a small helper function for this instead:

```
let toRate = v => v / playbackRate;
```

This way, everywhere you need to divide by the playbackRate, you can use this:

```
let duration = toRate(duration * (!iterationCount ? 1 : iterationCount));
```

Or something like this anyway.
This will be useful when calculating `length` as well, since you're dividing the endDelay there too.

::: devtools/client/animationinspector/utils.js:211
(Diff revision 1)
> -    this.minStartTime = Math.min(this.minStartTime,
> -                                 previousStartTime + relevantDelay);
> -    let length = (delay / playbackRate) +
> -                 ((duration / playbackRate) *
> -                  (!iterationCount ? 1 : iterationCount));
> +    this.minStartTime =
> +      Math.min(this.minStartTime,
> +               previousStartTime +
> +               relevantDelay +
> +               Math.min(Math.max(arrangedDelay, 0) +
> +                        arrangedDuration +

It's getting a bit hard to read this piece of code now. I suggest creating local variables to make this simpler.

Also, didn't you forget to divite endDelay by playbackRate here? Since delay and duration are both being divided, I think endDelay should be too.

Also, maybe let's create another helper function like `toRate` to cap values to minimum 0, since we need this in a couple of places:

`let minZero = v => Math.max(v, 0);`

So you could end up with something that looks more like this:

```
let startTime = toRate(minZero(delay)) +
                toRate(duration) +
                toRate(endDelay);
this.minStartTime = Math.min(
  this.minStartTime,
  previousStartTime + 
  relevantDelay + 
  Math.min(startTime, 0)
);
```

::: devtools/client/locales/en-US/animationinspector.properties:49
(Diff revision 1)
>  player.animationDelayLabel=Delay:
>  
> +# LOCALIZATION NOTE (player.animationEndDelayLabel):
> +# This string is displayed in each animation player widget. It is the label
> +# displayed before the animation endDelay.
> +player.animationEndDelayLabel=EndDelay:

This string is displayed to users, I think it should be "End delay:" instead of "EndDelay:"

::: devtools/client/themes/animationinspector.css:433
(Diff revision 1)
>       animation, so there's already the animation's left border that serves as
>       a separation. */
>    border-width: 1px;
>  }
>  
> +.animation-timeline .animation .endDelay {

s/endDelay/end-delay

::: devtools/client/themes/animationinspector.css:434
(Diff revision 1)
> +  position: absolute;
> +  height: 100%;
> +
> +  border: 1px solid var(--timeline-border-color);
> +  box-sizing: border-box;

If you move this part in a new rule, you can remove these 4 lines from here *and* from the `.delay` rule:

```
.animation-timeline .animation .delay,
.animation-timeline .animation .end-delay {
  position: absolute;
  height: 100%;
  border: 1px solid var(--timeline-border-color);
  box-sizing: border-box;
}
```

::: devtools/client/themes/animationinspector.css:449
(Diff revision 1)
> +                      transparent 3px,
> +                      var(--timeline-border-color) 3px,
> +                      var(--timeline-border-color) 4px);
> +}
> +
> +.animation-timeline .animation .endDelay.negative {

s/endDelay/end-delay

::: devtools/client/themes/animationinspector.css:449
(Diff revision 1)
> +.animation-timeline .animation .endDelay.negative {
> +  border-width: 1px;
> +}

You could also put this in common with the `.delay` rule:

```
.animation-timeline .animation .delay.negative,
.animation-timeline .animation .end-delay.negative {
  /* Negative delays are displayed on top of the animation, so they need a
     right border. Whereas normal delays are displayed just before the
     animation, so there's already the animation's left border that serves as
     a separation. */
  border-width: 1px;
}
```
Comment on attachment 8729346 [details]
MozReview Request: Bug 1253494 - Part2 Add tests for endDelay representation r?pbro

https://reviewboard.mozilla.org/r/39393/#review37839

There are a few changes needed in these tests.

::: devtools/client/animationinspector/test/browser.ini:13
(Diff revision 1)
>    doc_keyframes.html
>    doc_modify_playbackRate.html
>    doc_negative_animation.html
>    doc_simple_animation.html
>    doc_multiple_animation_types.html
> +  doc_animation_endDelay.html

No need for the `_animation` part in this new file name. `doc_end_delay.html` would be fine.
Also, please make sure the list of `support-files` stays in alphabetical order.

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_endDelay.js:15
(Diff revision 1)
> +// animation is delayed.
> +// Also check that negative endDelays do not overflow the UI, and are shown
> +// like positive endDelays.
> +
> +add_task(function*() {
> +  yield addTab(TEST_URL_ROOT + "doc_animation_endDelay.html");

`TEST_URL_ROOT` does not exist anymore, please use `URL_ROOT` now.

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_endDelay.js:18
(Diff revision 1)
> +  info("Selecting a endDelay animation");
> +  yield selectNode("#target1", inspector);
> +  let timelineEl = panel.animationsTimelineComponent.rootWrapperEl;
> +  checkEndDelayAndName(timelineEl);
> +
> +  info("Selecting a endDelay animation");
> +  yield selectNode("#target2", inspector);
> +  checkEndDelayAndName(timelineEl);
> +
> +  info("Selecting a endDelay animation");
> +  yield selectNode("#target3", inspector);
> +  checkEndDelayAndName(timelineEl);
> +
> +  info("Selecting a endDelay animation");
> +  yield selectNode("#target4", inspector);
> +  checkEndDelayAndName(timelineEl);

You can replace all of this with a loop:

```
let timelineEl = panel.animationTimelineComponent.rootWrapperEl;

let selectors = ["#target1", "#target2", "#target3", "#target4"];
for (let selector of selectors) {
  yield selectNode(selector, inspector);
  checkEndDelayAndName(timelineEl);
}
```

Also, here there's a mistake, the element `timelineEl` is the same for each of the 4 test cases. So basically, `checkEndDelayAndName` tests 4 times the same thing.

Maybe use this for loop isntead then:

```
for (let i = 0; i < selectors.length; i ++) {
  let selector = selectors[i];
}
```

And get the actual time block element like this:

```
let animationEl = timelineEl.querySelectorAll(".animation")[i];
```

And pass this to `checkEndDelayAndName` instead.

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_time_info.js:33
(Diff revision 1)
>      if (controller.animationPlayers[i].state.iterationCount !== 1) {
>        ok(title.match(/Repeats: /), "The tooltip shows the iterations");
>      } else {
>        ok(!title.match(/Repeats: /), "The tooltip doesn't show the iterations");
>      }
> +    ok(title.match(/endDelay: [\d.-]+s/), "The tooltip shows the endDelay");

s/endDelay/End delay

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_time_info.js:34
(Diff revision 1)
>        ok(title.match(/Repeats: /), "The tooltip shows the iterations");
>      } else {
>        ok(!title.match(/Repeats: /), "The tooltip doesn't show the iterations");
>      }
> +    ok(title.match(/endDelay: [\d.-]+s/), "The tooltip shows the endDelay");
> +    }

This change causes a syntax error in the file. Also, why did you add this here? This test opens the test page `doc_simple_animation.html` which does *not* contain any animations with an endDelay defined, so this assertion shouldn't work.

We should indeed have a test that verifies that the tooltip content is fine. To do this, please add a new element in `doc_simple_animation.html` with a script-generated animation that defines a value for endDelay. And then wrap this line:
```
ok(title.match(/End delay: [\d.-]+s/), "The tooltip shows the endDelay");
```
Into an IF that checks if the animation does have an endDelay:
```
if (controller.animationPlayers[i].state.endDelay) {
```

(In fact, you can do what is already done for `iterationCount`).

::: devtools/client/animationinspector/test/doc_animation_endDelay.html:6
(Diff revision 1)
> +<!DOCTYPE html>
> +<html lang="en">
> +<head>
> +  <meta charset="UTF-8">
> +  <style>
> +  target {

This rule doesn't apply to any element on the page.
Please add `class="target"` to each of the 4 `div` elements below and change `target` to `.target`

::: devtools/client/animationinspector/test/doc_animation_endDelay.html:24
(Diff revision 1)
> +    "use strict";
> +
> +    var el = document.getElementById("target1");
> +    el.animate(
> +      { opacity: [ 0, 1 ] },
> +      { id: 'endDelay_animation1',

Please use only double quotes (throughout the file).
Attachment #8729346 - Flags: review?(pbrosset)
Attachment #8729345 - Flags: review?(pbrosset)
Comment on attachment 8729345 [details]
MozReview Request: Bug 1253494 - Part1 Implement endDelay representation in the animation inspector r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39391/diff/1-2/
Comment on attachment 8729346 [details]
MozReview Request: Bug 1253494 - Part2 Add tests for endDelay representation r?pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39393/diff/1-2/
Attachment #8729346 - Flags: review?(pbrosset)
Comment on attachment 8729346 [details]
MozReview Request: Bug 1253494 - Part2 Add tests for endDelay representation r?pbro

https://reviewboard.mozilla.org/r/39393/#review38093

Code changes look good. You just need to add the doc_end_delay.html to the commit and I will R+ this.

::: devtools/client/animationinspector/test/browser.ini:7
(Diff revision 2)
>  tags = devtools
>  subsuite = devtools
>  skip-if = e10s && debug # bug 1252283
>  support-files =
>    doc_body_animation.html
> +  doc_end_delay.html

Thanks for changing the name, however you forgot to add this new file to the commit, so it's missing on mozreview.

::: devtools/client/animationinspector/test/browser_animation_timeline_shows_endDelay.js:28
(Diff revision 2)
> +function checkEndDelayAndName(timelineEl) {
> +  let endDelay = timelineEl.querySelector(".end-delay");
> +  let name = timelineEl.querySelector(".name");
> +  let targetNode = timelineEl.querySelector(".target");

For consistency, rename `timelineEl` to `animationEl` here.
Attachment #8729346 - Flags: review?(pbrosset)
Attachment #8729345 - Flags: review?(pbrosset) → review+
Comment on attachment 8729345 [details]
MozReview Request: Bug 1253494 - Part1 Implement endDelay representation in the animation inspector r=pbro

https://reviewboard.mozilla.org/r/39391/#review38097
Comment on attachment 8729345 [details]
MozReview Request: Bug 1253494 - Part1 Implement endDelay representation in the animation inspector r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39391/diff/2-3/
Attachment #8729345 - Attachment description: MozReview Request: Bug 1253494 - Part1 Implement endDelay representation in the animation inspector r?pbro → MozReview Request: Bug 1253494 - Part1 Implement endDelay representation in the animation inspector r=pbro
Attachment #8729346 - Flags: review?(pbrosset)
Comment on attachment 8729346 [details]
MozReview Request: Bug 1253494 - Part2 Add tests for endDelay representation r?pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39393/diff/2-3/
Comment on attachment 8729346 [details]
MozReview Request: Bug 1253494 - Part2 Add tests for endDelay representation r?pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39393/diff/3-4/
Attachment #8729346 - Flags: review?(pbrosset) → review+
Comment on attachment 8729346 [details]
MozReview Request: Bug 1253494 - Part2 Add tests for endDelay representation r?pbro

https://reviewboard.mozilla.org/r/39393/#review38413

Looks good to me now. Thanks for making these last changes!
https://hg.mozilla.org/mozilla-central/rev/170088059bf7
https://hg.mozilla.org/mozilla-central/rev/214a78b9334d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1261561
Depends on: 1261651
Depends on: 1264101
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.