Edge of animation graph is diagonal sometimes

RESOLVED FIXED in Firefox 53

Status

DevTools
Animation Inspector
P2
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: birtles, Assigned: daisuke)

Tracking

Trunk
Firefox 53
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 wontfix, firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Created attachment 8808043 [details]
Screenshot

In some cases I notice the edge of the animation graph is diagonal. For example, if I:

1. Login to trello.com
2. Open the Animation Inspector and select the <body> element.
3. Click the "Show Menu" button on the right

I see the graph from the attached screenshot. Notice the right-hand edge of the waves is diagonal but I believe it should be vertical.
(Reporter)

Updated

2 years ago
Component: Developer Tools → Developer Tools: Animation Inspector
Confirmed, I had mentioned this in bug 1210795 comment 78, but it got lost. Thanks for filing.
(Assignee)

Updated

2 years ago
Assignee: nobody → daisuke
(Assignee)

Comment 2

2 years ago
Created attachment 8808918 [details]
edge-diagonal-1315598.html

The bug reproduced with attached testcase.
The cause seems depending on animation duration.
If the duration is too short, it reproduces.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8809260 - Flags: review?(pbrosset)
(Assignee)

Updated

2 years ago
Attachment #8809261 - Flags: review?(pbrosset)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8809260 [details]
Bug 1315598 - Part 1: Edge of animation graph is diagonal sometimes.

https://reviewboard.mozilla.org/r/91840/#review93446

R+ with a few clarifications.

::: devtools/client/animationinspector/components/animation-time-block.js:23
(Diff revision 2)
> +// Also, narrow the distance which calculates by NARROW_AMOUNT
> +// in case of re-dividing.

I don't think these 2 extra lines add clarity to the overall comment.

::: devtools/client/animationinspector/components/animation-time-block.js:29
(Diff revision 2)
> +// in case of re-dividing.
>  // DURATION_RESOLUTION shoud be integer and more than 2.
>  const DURATION_RESOLUTION = 4;
>  // MIN_PROGRESS_THRESHOLD shoud be between more than 0 to 1.
>  const MIN_PROGRESS_THRESHOLD = 0.1;
> +// NARROW_AMOUNT shoud be less than 1.

Maybe rename NARROW_AMOUNT to BOUND_EXCLUDING_TIME so it's a bit clearer?
And maybe change the comment to say something like:
// BOUND_EXCLUDING_TIME should be less than 1ms and is used to exclude start and end bounds when dividing  duration in createPathSegments.

::: devtools/client/animationinspector/components/animation-time-block.js:683
(Diff revision 2)
>      // the previous segment and the Y coordinate of the current segment is too
>      // large, then recurse with a smaller duration to get more details
>      // in the graph.
>      if (Math.abs(currentSegment.y - previousSegment.y) > minProgressThreshold) {
>        // Divide the current interval (excluding start and end bounds
>        // by adding/subtracting 1ms).

"adding/subtracting 1ms" is an obsolete comment now, since we actually add and subtract NARROW_AMOUNT instead.
Attachment #8809260 - Flags: review?(pbrosset) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8809261 [details]
Bug 1315598 - Part 2: Add test for short duration animation.

https://reviewboard.mozilla.org/r/91842/#review93458

::: devtools/client/animationinspector/test/browser_animation_timeline_short_duration.js:24
(Diff revision 2)
> +    info(`Check the time block ${i}`);
> +    const {containerEl, animation: {state}} = timelineComponent.timeBlocks[i];
> +    checkSummaryGraph(containerEl, state);
> +  }
> +
> +  info("Check the time block one by one");

Why check the animations 1 by 1 again?
The test would be shorter if we didn't do this. Can you explain the reason why this is needed?

::: devtools/client/animationinspector/test/browser_animation_timeline_short_duration.js:71
(Diff revision 2)
> +    approximate(thirdLastPathSeg.y, 1, 0.001,
> +                "The y of third last segment should be approximate 1");

This assertion fails for me everytime when I run the test locally.

::: devtools/client/animationinspector/test/browser_animation_timeline_short_duration.js:95
(Diff revision 2)
> +}
> +
> +function approximate(value, expected, permissibleRange, message) {
> +  const min = expected - permissibleRange;
> +  const max = expected + permissibleRange;
> +  is(min <= value && value <= max, true, message);

This is the same as
`ok(min <= value && value <= max, message);`

::: devtools/client/animationinspector/test/doc_short_duration_animation.html:4
(Diff revision 2)
> +<!DOCTYPE html>
> +<html lang="en">
> +  <head>
> +    <style>

Please add <meta charset="UTF-8"> to avoid warnings.
Attachment #8809261 - Flags: review?(pbrosset)
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8809261 [details]
Bug 1315598 - Part 2: Add test for short duration animation.

https://reviewboard.mozilla.org/r/91842/#review93458

> Why check the animations 1 by 1 again?
> The test would be shorter if we didn't do this. Can you explain the reason why this is needed?

The reason is that I want to test it since the displayed area and iteration count are changed.
For example, the graphs change as,

in case of list,
| target | display area | iteration count |
| onetime | half of container | 1 |
| twotimes | whole of container | 2 |
| infinite | whole of container | 2 |
in case of detail,
| onetime | whole of container | 1 |
| twotimes | whole of container | 2 |
| infinite | whole of container | 1 |

In #onetime, I’d like to confirm that the path is same even if the displayed area is changed.
In #infinite, the iteration count is changed from 2 to 1. In animation-time-block.js logic, we use copied path of first path for after first path. I’d like to test that.
Also, it may be better that we add a test for iteration count.
In #twotimes, that is same state. So I drop. Thanks.

> This assertion fails for me everytime when I run the test locally.

Oh, what is the cause...
There are no error in try server...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4efad474e596
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 years ago
Status: NEW → ASSIGNED
Priority: -- → P2

Comment 17

2 years ago
mozreview-review
Comment on attachment 8809261 [details]
Bug 1315598 - Part 2: Add test for short duration animation.

https://reviewboard.mozilla.org/r/91842/#review94126

Ok if this passes on TRY then fine, but it still fails for me locally. It might be something with my environment, but I cloned mozilla-central this week only and don't have anything else applied locally, so I don't know what could be happening.
Did you test on Windows on TRY? 
I ran the test on Windows 10, and here's the failure report I get: https://pastebin.mozilla.org/8929230
Attachment #8809261 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 19

2 years ago
Hi Patrich,

We reproduced the failure in Windows 10.
So, we modified the test a bit.
Could you confirm in your environment?

Thanks!
Flags: needinfo?(pbrosset)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Daisuke Akatsuka (:daisuke) from comment #19)
> Hi Patrich,
> 
> We reproduced the failure in Windows 10.
> So, we modified the test a bit.
> Could you confirm in your environment?
> 
> Thanks!
Works for me now. Thank you Daisuke.
Flags: needinfo?(pbrosset)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 23

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eef68eeb10c6
Part 1: Edge of animation graph is diagonal sometimes. r=pbro
https://hg.mozilla.org/integration/autoland/rev/dc202d732d64
Part 2: Add test for short duration animation. r=pbro
Keywords: checkin-needed

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eef68eeb10c6
https://hg.mozilla.org/mozilla-central/rev/dc202d732d64
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I guess this will ride the train from 53.
status-firefox52: affected → wontfix

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.