1228978 - Add a drop-mark to the playback-rate selector in the animation-inspector's toolbar

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Attachment: no-dropmark.png

The playback rate selector in the toolbar doesn't look like something you can click on to choose a rate.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: dropmark.png

We could do something like this.

Comment 2

[Patrick Brosset <:pbro>]

In XUL toolbars, we have toolbar buttons that have drop markers already:

.devtools-toolbarbutton[type="menu"] > .toolbarbutton-menu-dropmarker, .devtools-toolbarbutton[type="menu-button"] > .toolbarbutton-menubutton-dropmarker {
	-moz-appearance: none !important;
	list-style-image: url("chrome://devtools/skin/images/dropmarker.svg");
	-moz-box-align: center;
	padding: 0 3px;
}

So we should use something similar.

Comment 3

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Hi Patrick,
Can I work on this one ? 

In order to make it even more discoverable, I would suggest : 
- adding a title attribute to the select element ( something like "Change rate selector" ), as we don't have any label for this
- adding a cursor : pointer . We could set it on the pause and playback button too

How do you feel about this ?

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #3)
> Hi Patrick,
> Can I work on this one ? 
> 
> In order to make it even more discoverable, I would suggest : 
> - adding a title attribute to the select element ( something like "Change
> rate selector" ), as we don't have any label for this
Yes, that sounds great. In fact, we should probably have tooltips for all buttons in the panel. It seems like we don't.
> - adding a cursor : pointer . We could set it on the pause and playback
> button too
So, none of the buttons in devtools have this, so for consistency reason I wouldn't do this here. cursor: pointer is usually reserved for links, and everywhere there are links in devtools, we do have the hand icon. So I'd prefer to keep it this way.

Comment 5

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug_1228978_v_01.patch

Comment 6

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug_1228978_v_01.patch

Hi,
I made some changes to display a drop marker and a tooltip when hovering.
I'm not 100% into convinced by this, it kinda looks weird when 1x is selected.
But in order to display 0.25x properly, I had to expand the width and remove the center alignment.

I have the feeling that we can use the default select element look, instead of having -moz-appearance: none . But I did not find a way to only find the triangle in the dropdown button.

I also changed the playbackRateLabel suffix, to use the proper multiplication glyph ( ✕ instead of "x" ), but I don't know if I should do this here, nor if I should do it at all.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8715010 [details] [diff] [review]
Bug_1228978_v_01.patch

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

This looks pretty good. I have a suggestion to help with the centering problems you mentioned.

::: devtools/client/locales/en-US/animationinspector.properties
@@ +68,5 @@
>  # This string is displayed as a tooltip for the icon that indicates that the
>  # animation is running on the compositor thread.
>  player.runningOnCompositorTooltip=This animation is running on compositor thread
>  
> +# LOCALIZATION NOTE (player.rateSelectorTooltip):

This should be timeline.rateSelectorTooltip instead.
The term "player" corresponds to one animation only (we used to have play/pause buttons per animation before we had a timeline).

@@ +69,5 @@
>  # animation is running on the compositor thread.
>  player.runningOnCompositorTooltip=This animation is running on compositor thread
>  
> +# LOCALIZATION NOTE (player.rateSelectorTooltip):
> +# This string is displayed in each animation player widget, as the tooltip of

Therefore, you need to change this too:
This string is displayed in the timeline toolbar, as the tooltip of the drop-down list that can be used to change the rate at which the animations run.

@@ +72,5 @@
> +# LOCALIZATION NOTE (player.rateSelectorTooltip):
> +# This string is displayed in each animation player widget, as the tooltip of
> +# drop-down list that can be used to change the rate at which the
> +# animation runs
> +player.rateSelectorTooltip=Change animation play rate

Change player.rateSelectorTooltip to timeline.rateSelectorTooltip here too.
Also, I think the following value would be better: Set the animations playback rates.

Finally, could you please also add tooltip labels to the play/pause and rewind buttons?

::: devtools/client/themes/animationinspector.css
@@ +177,5 @@
>    width: 100%;
>    height: 100%;
> +  background-image: url("chrome://devtools/skin/images/dropmarker.svg");
> +  background-repeat: no-repeat;
> +  background-position: calc(100% - 2px) center;

One way to make it look less weird is to keep text-align: center; but add a padding at the end like:
padding-inline-end: 1em;

Comment 8

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Thank you for the feedback Patrick.

> Finally, could you please also add tooltip labels to the play/pause and
> rewind buttons?

Yes, of course. Should I make the play/pause button one react to animation status ? i.e. "Pause" when animation is played, "Play" when animation is paused ?

> One way to make it look less weird is to keep text-align: center; but add a
> padding at the end like:
>   padding-inline-end: 1em;

I did not know about this property. After a quick look at the MDN page, I do not understand how it differs from padding-right ( apart taking into account text-direction, which for this bug doesn't seem to matter ). Am I missing something ?

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #8)
> Thank you for the feedback Patrick.
> 
> > Finally, could you please also add tooltip labels to the play/pause and
> > rewind buttons?
> 
> Yes, of course. Should I make the play/pause button one react to animation
> status ? i.e. "Pause" when animation is played, "Play" when animation is
> paused ?
If this is a straightforward change, then yes. But if it requires more code changes to keep track of the state, feel free to just add one label that doesn't change, like "Pause/Resume Animations". I wouldn't want to mix more unrelated code changes in this bug.
> > One way to make it look less weird is to keep text-align: center; but add a
> > padding at the end like:
> >   padding-inline-end: 1em;
> 
> I did not know about this property. After a quick look at the MDN page, I do
> not understand how it differs from padding-right ( apart taking into account
> text-direction, which for this bug doesn't seem to matter ). Am I missing
> something ?
Yeah, that's exactly what it is. It's part of a bunch of new "logical keywords" CSS properties that aim at making it easier to write CSS for various directions. 
I think it does matter in fact. For instance, when you have a <select> in a dir=rtl page, then the drop marker is on the opposite side of the select.
Maybe it doesn't matter for now because I doubt that the animation inspector works well with rtl. It's never been tested much and there might be a lot of other things that don't work.

Comment 10

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Dark-theme - Drop-mark alignement

Comment 11

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug_1228978_v_01.patch

Hi,

I added the title attributes for the rewind and resumePause buttons ( taking care of the animation state was straightforward ).
I ended up not adding any padding, and only widening the button to keep the label center-aligned and to prevent overlapping.

In the screenshot attached ( https://bugzilla.mozilla.org/show_bug.cgi?id=1228978 ), you can see that : 
 - it has the same space between the drop-marker and the button border than the webconsole toolbar buttons
 - it looks alright with both light and dark theme

Hope everything is fine, feel free to tell me if not !

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8715899 [details] [diff] [review]
Bug_1228978_v_01.patch

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

Looks good. Almost there. I would like to revisit one last time the select width, see my comment below.
Also, sorry about the trailing whitespace comments, but the code review tool shows them automatically, and we try and never commit trailing whitespaces.
I personally have set up my code editor to get rid of them all automatically on save.

::: devtools/client/locales/en-US/animationinspector.properties
@@ +69,5 @@
>  # animation is running on the compositor thread.
>  player.runningOnCompositorTooltip=This animation is running on compositor thread
>  
> +# LOCALIZATION NOTE (timeline.rateSelectorTooltip):
> +# This string is displayed in the timeline toolbar, as the tooltip of the 

Please remove the trailing space at the end of this line.

@@ +70,5 @@
>  player.runningOnCompositorTooltip=This animation is running on compositor thread
>  
> +# LOCALIZATION NOTE (timeline.rateSelectorTooltip):
> +# This string is displayed in the timeline toolbar, as the tooltip of the 
> +# drop-down list that can be used to change the rate at which the animations 

and here

@@ +75,5 @@
> +# run.
> +timeline.rateSelectorTooltip=Set the animations playback rates
> +
> +# LOCALIZATION NOTE (timeline.pauseResumeButtonTooltip):
> +# This string is displayed in the timeline toolbar, as the tooltip of the 

and here

@@ +76,5 @@
> +timeline.rateSelectorTooltip=Set the animations playback rates
> +
> +# LOCALIZATION NOTE (timeline.pauseResumeButtonTooltip):
> +# This string is displayed in the timeline toolbar, as the tooltip of the 
> +# pause/resume button that can be used to pause or resume the animations 

and here

@@ +80,5 @@
> +# pause/resume button that can be used to pause or resume the animations 
> +timeline.pausedButtonTooltip=Resume the animations
> +
> +# LOCALIZATION NOTE (timeline.pauseResumeButtonTooltip):
> +# This string is displayed in the timeline toolbar, as the tooltip of the 

and here

@@ +81,5 @@
> +timeline.pausedButtonTooltip=Resume the animations
> +
> +# LOCALIZATION NOTE (timeline.pauseResumeButtonTooltip):
> +# This string is displayed in the timeline toolbar, as the tooltip of the 
> +# pause/resume button that can be used to pause or resume the animations 

and here

@@ +85,5 @@
> +# pause/resume button that can be used to pause or resume the animations 
> +timeline.resumedButtonTooltip=Pause the animations
> +
> +# LOCALIZATION NOTE (timeline.rewindButtonTooltip):
> +# This string is displayed in the timeline toolbar, as the tooltip of the 

and here

@@ +86,5 @@
> +timeline.resumedButtonTooltip=Pause the animations
> +
> +# LOCALIZATION NOTE (timeline.rewindButtonTooltip):
> +# This string is displayed in the timeline toolbar, as the tooltip of the 
> +# rewind button that can be used to rewind the animations 

and here

::: devtools/client/themes/animationinspector.css
@@ +184,5 @@
>  }
>  
>  #timeline-rate {
>    position: relative;
> +  width: 6em;

I does look a little too big now. I think we need to make it smaller. What about 4.5em but adding a small padding-right on the <select> ? This seems to work fine for me locally in all cases. I'll attach a screen capture of what I think would be a good size for this select.

Comment 13

[Patrick Brosset <:pbro>]

Attachment: rate.gif

Comment 14

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug_1228978_v_02.patch

> Also, sorry about the trailing whitespace comments, but the code review tool shows them automatically, and we try and never commit trailing whitespaces.
> I personally have set up my code editor to get rid of them all automatically on save.

Oh, I'm the one who's sorry. I used to see them in JS files thanks to ESLint plugin in Sublime, but as it's a properties file here, I missed it.
Put "trim_trailing_whitespace_on_save" to true, it should not be a problem anymore.

> I does look a little too big now. I think we need to make it smaller. What about 4.5em but adding a small padding-right on the <select> ? This seems to work fine for me locally in all cases. I'll attach a screen capture of what I think would be a good size for this select.

Yes, looks better now !

Comment 15

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

As I think I'm very close to what you wanted, I pushed to TRY : 

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

Pushed with 

hg push --bookmark 1228978_animation_rate_drop_mark try 

I'm using the --bookmark again, because I have an unrelated local commit ( in another bookmark ) which appears in my outgoing changes. I omitted the -f too in order to not push it.

Comment 16

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

(In reply to Nicolas Chevobbe from comment #15)
> As I think I'm very close to what you wanted, I pushed to TRY : 
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=551656172965
> 
> Pushed with 
> 
> hg push --bookmark 1228978_animation_rate_drop_mark try 
> 
> I'm using the --bookmark again, because I have an unrelated local commit (
> in another bookmark ) which appears in my outgoing changes. I omitted the -f
> too in order to not push it.

I made some mistakes ( no pull before push ), and cancelled the job. But now I can't push anymore ( getting no changes found ).

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8716042 [details] [diff] [review]
Bug_1228978_v_02.patch

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

Looks perfect to me. I know you didn't flag me for review on this one yet, but I took a look and I don't think there's anything to change (apart from a small nitpicking comment).
Do you need help to push to try?
If we do find test failures on try, they're unlikely to require a fix that will require a new review. So I'll R+ this now.

::: devtools/client/animationinspector/animation-panel.js
@@ +215,5 @@
>      let {isMoving, isUserDrag, time} = data;
>  
>      this.playTimelineButtonEl.classList.toggle("paused", !isMoving);
>  
> +    let l10nPlayProperty = !isMoving ? "timeline.pausedButtonTooltip" :

nit: maybe invert the condition: !isMoving -> isMoving
This might avoid confusion later.

Comment 18

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

Attachment: Bug_1228978_v_03.patch

Pushed a new patch with the change you asked for.

> Do you need help to push to try?

it would be great indeed. I have a problem with my repo and not much time to investigate for now. Thank you

Comment 19

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6042566cc2f1&group_state=expanded

Comment 20

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

The job is over and everything looks good, except 2 errors that don't seems to be related to this patch.

Comment 21

[Pulsebot]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/411bf60720dc