1105825 - Create a simple animation panel in the inspector

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

This should be a simple prototype, to exercise the animation actors added in bug 1096044.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1105825-simple-animation-panel WIP.patch

Really ugly, broken in many ways, but this was put together in a couple of hours merely as a way to test the actors in bug 1096044.

Comment 2

[Patrick Brosset <:pbro>]

Little preview: https://www.youtube.com/watch?v=7Sfkbcuk94s&feature=youtu.be

Comment 3

[Patrick Brosset <:pbro>]

I was about to cc Gabriel (:gl) right when you cc'd yourself :)
You started to work on this last summer, please do provide feedback. As you can see in the video, this is really a draft.

Once we have a good idea of what we want to put in place, we should start creating child bugs to implement the UI in shareable widgets. Right now, everything is in the panel javascript file.

Also, we can get some inspiration from the quick preview Paul Lewis gave of the chromedevtools animation inspector at the chrome dev summit last week: https://www.youtube.com/watch?v=RCFQu0hK6bU (starts around 20:30).

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1105825-simple-animation-panel WIP.patch

Still WIP, but with more tests.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1105825-simple-animation-panel WIP.patch

Refactored the whole UI so it makes more sense, is more manageable and is more in line with how other devtools panels are built.
The UI is still really rough though, but I wanted to get the architecture right first (note that I haven't yet updated the tests so they will fail).

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8530345 [details] [diff] [review]
bug1105825-simple-animation-panel WIP.patch

Marking obsolete as I'm about to attach new patches instead.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: bug1105825-1-autorefresh-players v1.patch

Hey Brian, in order to make a timeline widget for animations in the inspector, I need the AnimationPlayerActor to give me its state at regular intervals.
So this patch is about adding an auto-refresh mechanism to the Front, so that's easy for consumers of the actor to just turn auto-refresh on and off using the front.
This way, consumers can receive update events at regular intervals.

My next will contain the UI changes adding the new panel with timeline widgets in the inspector. For info, in order to make the timeline progress smoothly during the animation, I'll be using a requestAnimationFrame loop, and will just use the front's auto-refresh mechanism to re-sync the timeline. This will allow not generating too much RPC traffic.

What do you think of this approach?

Pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4be7f93ba83b

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug1105825-1-autorefresh-players v2.patch

Fixed a typo in the new test.
New try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=455f2a15e419

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug1105825-2-toolbox-inspector-sidebar-destroy-sequence v1.patch

In order to cleanly destroy the animation UI and fronts, I needed the sidebar panel's destroy method to be async. However, the sidebar widget doesn't allow for this. So I had to modify it.
Doing so, I found and fixed a few issues:
- the inspector panel now waits for the sidebar to be destroyed
- the toolbox now waits for all panels to be destroyed before destroying the host
- the inspector tests now wait for the toolbox to be destroyed and closed before removing the tab.

This got rid of a leak I saw while working a new test for the animations panel.

Panos: could you take a look at the destroy sequence changes I made? I can ask someone else to review the inspector test changes if you would like me to.

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> Created attachment 8536553 [details] [diff] [review]
> bug1105825-1-autorefresh-players v2.patch
> 
> Fixed a typo in the new test.
> New try build:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=455f2a15e419
There are still failures with the new test I added. I'll try and take a look at these in the afternoon.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug1105825-3-simple-animation-panel v1.patch

Not quite ready for review, but almost there.
I'm in the process of adding tests for this panel. I think I've covered all the basic cases, now I need to add more tests for the special UI cases.
After this, I'll ask for reviews.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug1105825-1-autorefresh-players v3.patch

I think I found the issues that were making the test fail.

Comment 13

[Patrick Brosset <:pbro>]

Pending try build for patch 1: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=44805de42c60

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug1105825-3-simple-animation-panel v2.patch

Part 3, now with more tests. I think this is getting ready for a first round of review.

I'm not too sure about the CSS for this new panel, so Brian, it would be great if you could take a look at this part at least, and let me know your thoughts.

Victor, I somewhat designed the controler/panel part based on the way you did some of the recent panels, a review from you on this part would help a lot.

The lack of features in this new panel is intentional. I'd rather try and ship something simple soon.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> Created attachment 8536530 [details] [diff] [review]
> bug1105825-1-autorefresh-players v1.patch
> 
> Hey Brian, in order to make a timeline widget for animations in the
> inspector, I need the AnimationPlayerActor to give me its state at regular
> intervals.
> So this patch is about adding an auto-refresh mechanism to the Front, so
> that's easy for consumers of the actor to just turn auto-refresh on and off
> using the front.
> This way, consumers can receive update events at regular intervals.
> 
> My next will contain the UI changes adding the new panel with timeline
> widgets in the inspector. For info, in order to make the timeline progress
> smoothly during the animation, I'll be using a requestAnimationFrame loop,
> and will just use the front's auto-refresh mechanism to re-sync the
> timeline. This will allow not generating too much RPC traffic.

Hi Patrick, sorry, I wasn't CC'ed on this bug so I never saw this.

We're planning to add an animation observer mechanism specifically so devtools can detect new / removed / updated animations. Cameron McCormack will probably be in touch to ask you what you need here.

With regards to changes, I suspect we'll dispatch a notice when, say, the animation duration changes but not when its current time changes. So you'll still need to poll for changes to current time etc. I suspect we'd only report out-of-band / user-or-author-triggered-changes not changes that occur during the regular running of an animation but we could do that if you need it.

So I think the scheme you outlined makes sense.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> My next will contain the UI changes adding the new panel with timeline
> widgets in the inspector. For info, in order to make the timeline progress
> smoothly during the animation, I'll be using a requestAnimationFrame loop,
> and will just use the front's auto-refresh mechanism to re-sync the
> timeline. This will allow not generating too much RPC traffic.

One point to mention is that animations typically only update their time once per requestAnimationFrame tick. So you don't need to check any more often than that.

The one exception is Bug 927349 which means we can actually update an animation's timeline and start time a second time within the one requestAnimationFrame tick. We're currently discussing if that's a good idea though.

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> I'm not too sure about the CSS for this new panel, so Brian, it would be
> great if you could take a look at this part at least, and let me know your
> thoughts.

I'm not sure exactly what about the CSS you want me to check?

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Birtles (:birtles) from comment #17)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> > I'm not too sure about the CSS for this new panel, so Brian, it would be
> > great if you could take a look at this part at least, and let me know your
> > thoughts.
> 
> I'm not sure exactly what about the CSS you want me to check?

I think Patrick is referring to Brian Grinstead here.

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #15)
> We're planning to add an animation observer mechanism specifically so
> devtools can detect new / removed / updated animations. Cameron McCormack
> will probably be in touch to ask you what you need here.
Cool, this will help a lot.
> With regards to changes, I suspect we'll dispatch a notice when, say, the
> animation duration changes but not when its current time changes. So you'll
> still need to poll for changes to current time etc. I suspect we'd only
> report out-of-band / user-or-author-triggered-changes not changes that occur
> during the regular running of an animation but we could do that if you need
> it.
> 
> So I think the scheme you outlined makes sense.
Perfect. The new observers you intend to add will help me get the animation panel UI refreshed when a new animation is created, or removed, or when an animation's duration changes.
What about if the playState changes? Anyway, I'll discuss the details with Cameron later.

The auto-refresh mechanism I'm putting in place here is also required because the actor (the thing that lives in the content process and has access to the waapi) may be running on a remote device, while the UI that consumes the data and displays it may be running somewhere else. So, we somehow need the UI to contact the server at regular intervals to display the new state of the animation (mostly the currentTime for now).

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #16)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> > My next will contain the UI changes adding the new panel with timeline
> > widgets in the inspector. For info, in order to make the timeline progress
> > smoothly during the animation, I'll be using a requestAnimationFrame loop,
> > and will just use the front's auto-refresh mechanism to re-sync the
> > timeline. This will allow not generating too much RPC traffic.
> 
> One point to mention is that animations typically only update their time
> once per requestAnimationFrame tick. So you don't need to check any more
> often than that.
> 
> The one exception is Bug 927349 which means we can actually update an
> animation's timeline and start time a second time within the one
> requestAnimationFrame tick. We're currently discussing if that's a good idea
> though.
So, the requestAnimationFrame loop I was mentioning here was for the UI only. As I said in my previous comment, the UI and the actors are running in different processes, maybe different devices, and the UI that displays the current time of the animation will look better if it progresses smoothly. Hence my plan to get the currentTime only every once in a while, and in between use a requestAnimationFrame loop to avoid making the UI "jump" from update to update.  

(In reply to Brian Birtles (:birtles) from comment #17)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> > I'm not too sure about the CSS for this new panel, so Brian, it would be
> > great if you could take a look at this part at least, and let me know your
> > thoughts.
> 
> I'm not sure exactly what about the CSS you want me to check?
As jryans said, this was intended at Brian Grinstead :)

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8536619 [details] [diff] [review]
bug1105825-2-toolbox-inspector-sidebar-destroy-sequence v1.patch

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

::: browser/devtools/framework/toolbox.js
@@ +1656,4 @@
>      // target.
> +    this._destroyer = promise.all(outstanding).then(() => {
> +      return this.destroyHost();
> +    }, console.error).then(() => {

This form doesn't catch errors from destroyHost, like the previous one. A better form would be:

.then(() => this.destroyHost()).then(null, console.error)

Comment 22

[Patrick Brosset <:pbro>]

Attachment: bug1105825-2-toolbox-inspector-sidebar-destroy-sequence v2.patch

Thanks Panos!
Fixed review comment.

Comment 23

[Patrick Brosset <:pbro>]

I'm having some difficulties making part 2 stable in try. It seems to be making some random tests fail intermittently.
I'm going to make this bug depend on bug 1113552, and bug 1113584 as the test refactorings I've made in these bugs seem to help make things more stable.

Comment 24

[Patrick Brosset <:pbro>]

Attachment: bug1105825-1-autorefresh-players v4.patch

The test that I added for this change was failing intermittently on try. And that's because it was using ... TIMEOUTS!!! And we all know that timeouts in test are bad.

So I refactored the test to just wait for events to be fired, which means that if the auto-refresh mechanism is broken in the future, the test will time out, but that's okay, it's still better than having it fail randomly because a machine is under heavy load or whatever.

Comment 25

[Patrick Brosset <:pbro>]

New try builds:
- for part 1: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7dce509526eb
- for part 2: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5365ace78e99
(these 2 parts are in fact independent, hence the separate builds).

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8537248 [details] [diff] [review]
bug1105825-3-simple-animation-panel v2.patch

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

f+ for the styling changes.  I like the direction and use of flexbox, but just needs a few changes.  Probably the biggest thing is adding all the appropriate states for the html buttons so that we have focus/hover/active etc styled.  I'd recommend generally copying the styles used for devtools-toolbarbutton (although this should be a lot simpler than that in general since that's used for a bunch of different types of buttons)

::: browser/themes/shared/devtools/animationinspector.css
@@ +62,5 @@
> +.animation-title .meta-data {
> +  float: right;
> +}
> +
> +.animation-title em {

Maybe use a strong tag instead (unless if there is some semantic reason for using an em)

@@ +73,5 @@
> +
> +.timeline {
> +  height: 20px;
> +  width: 100%;
> +  display : flex;

Nit extra space before :

@@ +79,5 @@
> +  border-bottom: 1px solid var(--theme-splitter-color);
> +}
> +
> +.timeline .playback-controls {
> +  height: 100%;

Don't need height: 100%

@@ +81,5 @@
> +
> +.timeline .playback-controls {
> +  height: 100%;
> +  width: 50px;
> +  flex-grow: 0;

Don't need flex-grow: 0 here, the fact that it's set to 1 on sliders-container takes care of this

@@ +86,5 @@
> +  display: flex;
> +  flex-direction: row;
> +  /* hack to get rid of the extra whitespace at the beginning of each input
> +     range */
> +  margin-right: -2px;

Removing this margin-right rule doesn't seem to change anything for me.  Is it still needed with the display: flex?

@@ +91,5 @@
> +}
> +
> +/* Playback control buttons */
> +
> +.timeline .playback-controls button {

I'd like to see this button have similar styling to .devtools-toolbarbutton (including focus/hover/active styles).

Ideally we could add a .devtools-button class to toolbars.inc.css that works for HTML elements.  In this case we could also update the filter.svg rule for theme-light into the big selector list that does that in toolbarbuttons.inc.css.

This class could also be reused for the #element-picker above.

@@ +151,5 @@
> +.timeline .sliders-container {
> +  flex-grow: 1;
> +  height: 100%;
> +  position: relative;
> +  

Nit: whitespace

@@ +157,5 @@
> +}
> +
> +.timeline .sliders-container .current-time {
> +  position: absolute;
> +  top: -1px; /* hack to vertically align nicely */

You could set padding: 0 instead of doing this hack

@@ +159,5 @@
> +.timeline .sliders-container .current-time {
> +  position: absolute;
> +  top: -1px; /* hack to vertically align nicely */
> +  left: 0;
> +  

Nit: whitespace

@@ +181,5 @@
> +}
> +
> +/* Current time label */
> +
> +.timeline .time-display {

This CSS is simpler but should achieve the same effect:

  display: flex;
  align-items: center;
  justify-content: center;
  width: 50px;
  border-left: 1px solid var(--theme-splitter-color);
  background: var(--theme-toolbar-background);

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8539284 [details] [diff] [review]
bug1105825-1-autorefresh-players v4.patch

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

Looks reasonable

::: toolkit/devtools/server/actors/animation.js
@@ +277,5 @@
> +
> +    // Check if something has changed
> +    let hasChanged = false;
> +    for (let key in data) {
> +      if (this.state[key] !== data[key]) {

Is `this.state` always guaranteed to be set?  I notice it's set in the form() and not initialize.

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #26)
> Comment on attachment 8537248 [details] [diff] [review]
> bug1105825-3-simple-animation-panel v2.patch
> 
> Review of attachment 8537248 [details] [diff] [review]:

What is the Bug # for having a way to open additional sidebar tabs when the sidebar width becomes to small to display all of them at once?  I think that should block this bug since it's really likely now for that to happen.

Comment 29

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #28)
> (In reply to Brian Grinstead [:bgrins] from comment #26)
> > Comment on attachment 8537248 [details] [diff] [review]
> > bug1105825-3-simple-animation-panel v2.patch
> > 
> > Review of attachment 8537248 [details] [diff] [review]:
> 
> What is the Bug # for having a way to open additional sidebar tabs when the
> sidebar width becomes to small to display all of them at once?  I think that
> should block this bug since it's really likely now for that to happen.
Bug 1101569. I haven't had the chance to work that much on it for a while though. It shouldn't take a huge amount of work. If I recall correctly, I was working on adding new tests and the main code changes were done and F+'d by Victor.

Comment 30

[Patrick Brosset <:pbro>]

Attachment: bug1105825-2-toolbox-inspector-sidebar-destroy-sequence v3.patch

Made a minor test update and changed the commit message.
Also, since bug 1113584 and bug 1113552 have landed on fx-team, I can now push this one too:

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

Comment 31

[Patrick Brosset <:pbro>]

Attachment: bug1105825-1-autorefresh-players v5.patch

Thanks Brian for the review.
Turns out that form() is called right after the front is initialized, so this.state will always be defined. Having said that, it's probably better to make this obvious by adding |this.state = {};| in initialize. This will make the property easier to understand.
I've done this changed and carried over R+ to this new patch.

Comment 32

[Patrick Brosset <:pbro>]

Pushed part 1 to fx-team: https://hg.mozilla.org/integration/fx-team/rev/b77aade1e4f8

Comment 33

[Patrick Brosset <:pbro>]

Attachment: bug1105825-disable-actor-tests.patch

I didn't realize, but Brian Birtles had made this bug blocked with bug 1112422 because the Web Animations API isn't enabled in beta and release.

It's not a huge deal because the biggest part of this bug hasn't landed, but some of it has, and enough that the browser mochitests are going to permafail when they reach beta if the API isn't pref'd on then.
So, to avoid this, either we get bug 1112422 landed in nightly before the next release, or we simply disable the tests for now and re-enable them when possible.

I prefer to be on the safe side and disable all the tests for now.
It's just 5 skip-if lines to add and it's very simple to re-enable when we can (I filed bug 1114780 for this).

Since this is just disabling tests, I have R+'d it myself.

Comment 34

[Patrick Brosset <:pbro>]

Comment on attachment 8540382 [details] [diff] [review]
bug1105825-disable-actor-tests.patch

https://hg.mozilla.org/integration/fx-team/rev/32b3c028d71b

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c994439deec4
https://hg.mozilla.org/mozilla-central/rev/b77aade1e4f8

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32b3c028d71b

Comment 37

[Victor Porof [:vporof][:vp]]

Comment on attachment 8537248 [details] [diff] [review]
bug1105825-3-simple-animation-panel v2.patch

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

I didn't look at this in too much depth, because I don't know how the underlying actor works, but the code is nice and very easy to read, and the panel is fun to play with. Ship it.

Comment 38

[Patrick Brosset <:pbro>]

Attachment: bug1105825-3-simple-animation-panel v3.patch

Brian, I moved the generic part of the button CSS code to toolbars.inc.css into a new .devtools-button class. Let me know if my changes were what you had in mind?

Comment 39

[Brian Grinstead [:bgrins]]

Comment on attachment 8546022 [details] [diff] [review]
bug1105825-3-simple-animation-panel v3.patch

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

I was going to add more notes, but I'm just going to attach a patch with CSS / HTML changes since I think that'll be easier than listing stuff here

::: browser/devtools/animationinspector/animation-inspector.xhtml
@@ +14,5 @@
> +    <link rel="stylesheet" href="chrome://browser/skin/devtools/animationinspector.css" type="text/css"/>
> +    <script type="application/javascript;version=1.8" src="chrome://browser/content/devtools/theme-switching.js"/>
> +  </head>
> +  <body class="theme-sidebar devtools-monospace" role="application">
> +    <div id="players"></div>

Adding class="theme-toolbar" will make the play/pause button look better

@@ +18,5 @@
> +    <div id="players"></div>
> +    <div id="error-message">
> +      <p>&invalidElement;</p>
> +      <p>&selectElement;</p>
> +      <button id="element-picker" class="devtools-toolbarbutton"></button>

Use .devtools-button class here since it's an HTML element

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +254,5 @@
> +  /* The icon is absolutely positioned in the button using ::before */
> +  position: relative;
> +}
> +
> +.theme-dark .devtools-button:hover,

Here's what I'd suggest instead to match existing styles (with the .theme-toolbar class added to a parent element).

/* Button States */
.theme-dark .devtools-button:not([disabled]):hover {
  background: rgba(0, 0, 0, .3); /* Splitters */
}
.theme-light .devtools-button:not([disabled]):hover {
  background: rgba(170, 170, 170, .3); /* Splitters */
}

.theme-dark .devtools-button:not([disabled]):hover:active {
  background: rgba(0, 0, 0, .4); /* Splitters */
}
.theme-light .devtools-button:not([disabled]):hover:active {
  background: rgba(170, 170, 170, .4); /* Splitters */
}

.theme-dark .devtools-button[checked] {
  background: rgba(29, 79, 115, .7); /* Select highlight blue */
  color: var(--theme-selection-color);
}
.theme-light .devtools-button[checked] {
  background: rgba(76, 158, 217, .2); /* Select highlight blue */
}

Comment 40

[Brian Grinstead [:bgrins]]

Attachment: animation-styling-feedback.patch

Uses devtools-button for element picker, updates devtools-button styling to more closely match devtools-toolbarbutton, and adds a [standalone] attribute to the class for buttons like element-picker

Comment 41

[Patrick Brosset <:pbro>]

Comment on attachment 8546056 [details] [diff] [review]
animation-styling-feedback.patch

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

Wow thanks Brian for sending this patch. I'm not too comfortable with the CSS of our common widgets, so this helps a lot.
One minor question below:

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +263,5 @@
>  }
>  
> +/* Button States */
> +.theme-dark .devtools-button:not([disabled]):hover {
> +  background: rgba(0, 0, 0, .3); /* Splitters */

What do the /* Splitters */ comments mean in these new rules?
And why don't we have theme variables for these other colors?

Comment 42

[Patrick Brosset <:pbro>]

Attachment: bug1105825-3-simple-animation-panel v4.patch

Folded with your styling feedback patch. Thanks!

Comment 43

[Patrick Brosset <:pbro>]

Pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aacf025c772e

For info, everything is ready now in fx-team/m-c to get this landed. The web animations api is now enabled for chrome code by default, and my last preliminary patches have gone in a minute ago.
So if test pass on try, we could potentially land this before the next release on monday.

Comment 44

[Patrick Brosset <:pbro>]

Ah, one blocking bug is missing though: bug 1101569.
That's the one that adds a "..." dropdown menu to the sidebar in case there isn't enough space to display all tabs. I have a review request pending for this one. I'm not sure it should block this bug though. I know it would be better, but the panel is just a very simple first step anyway and I think we can live for a while with people have to resize the sidebar to access it.

Comment 45

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #41)
> Comment on attachment 8546056 [details] [diff] [review]
> animation-styling-feedback.patch
> 
> Review of attachment 8546056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wow thanks Brian for sending this patch. I'm not too comfortable with the
> CSS of our common widgets, so this helps a lot.
> One minor question below:
> 
> ::: browser/themes/shared/devtools/toolbars.inc.css
> @@ +263,5 @@
> >  }
> >  
> > +/* Button States */
> > +.theme-dark .devtools-button:not([disabled]):hover {
> > +  background: rgba(0, 0, 0, .3); /* Splitters */
> 
> What do the /* Splitters */ comments mean in these new rules?
> And why don't we have theme variables for these other colors?

Unfortunately we don't yet have a color() function in CSS that will let us adjust a single channel in an rgb color, and the splitters are defined without any alpha (https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/light-theme.css#19).  So for now we have to use the old style way of declaring a color per-theme.  The alternative would be to add new variables like --theme-button-hover-background and --theme-button-hover-active-background.

Comment 46

[Brian Grinstead [:bgrins]]

Comment on attachment 8546524 [details] [diff] [review]
bug1105825-3-simple-animation-panel v4.patch

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

CSS changes look good

::: browser/themes/shared/devtools/animationinspector.css
@@ +110,5 @@
> +.timeline .sliders-container {
> +  flex-grow: 1;
> +  height: 100%;
> +  position: relative;
> +

Nit: blank line

Comment 47

[Patrick Brosset <:pbro>]

Attachment: bug1105825-3-simple-animation-panel v5.patch

Thanks Victor and Brian for these reviews.
I fixed the latest review comment in this patch and also did a minor change to the panel startup/shutdown mechanism because some /browser/devtools/shared tests where failing earlier (this was due to tests creating/destroying the toolbox quickly, and the panel shutdown was called before startup could be finished).

Comment 48

[Patrick Brosset <:pbro>]

Attachment: bug1105825-sidebar-destroy-checks v1.patch

My latest attempt at pushing to try (https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09833d5ab072) has some remaining failures.

Opening and closing the toolbox (on the inspector panel) quickly makes some of the tilt tests fail.

My understanding of these failures is that the new animations panel is the first sidebar-panel that has an async destroy function, and recently, the sidebar widget's destroy sequence was refactored to yield on panel's destroy promises.

The thing is, since some tests abruptly close the window without waiting for the toolbox to be properly destroyed, by the time the destroy promise of the animations panel is fulfilled, the window doesn't exist anymore and the rest of the sidebar destroy method fails.

So, here is a separate patch that makes the sidebar's destroy method less fragile to this use case, allowing it to complete even if the tab/tabpanel DOM elements don't exist anymore.

Comment 49

[Patrick Brosset <:pbro>]

Pending try build with last 2 patches: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee6ad5e45993

Comment 50

[Patrick Brosset <:pbro>]

Attachment: bug1105825-3-simple-animation-panel v6.patch

And a minor test-only change after a couple or intermittents showed up in the last try push in one of the new mochitests I added.
This test was using a timeout, and of course when the test VM got slower, the test failed.

New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c599cd5c3b12

Comment 51

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8547029 [details] [diff] [review]
bug1105825-sidebar-destroy-checks v1.patch

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

Looks good to me.

Comment 52

[Patrick Brosset <:pbro>]

Comment on attachment 8547029 [details] [diff] [review]
bug1105825-sidebar-destroy-checks v1.patch

Thanks Panos for the review!
Last try is green.
This patch is now in fx-team: https://hg.mozilla.org/integration/fx-team/rev/814e685b77e1

Comment 53

[Patrick Brosset <:pbro>]

Comment on attachment 8547064 [details] [diff] [review]
bug1105825-3-simple-animation-panel v6.patch

And the last part is now also in fx-team: https://hg.mozilla.org/integration/fx-team/rev/35144551829c

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/814e685b77e1
https://hg.mozilla.org/mozilla-central/rev/35144551829c

Comment 55

[Lawrence Mandel [:lmandel] (use needinfo)]

relnoted as "New Inspector animations panel to control element animations"