1197192 - Allow dragging the scrubber from anywhere on the element

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

In bug 1169563 a scrubber is being added to the animation-inspector panel.
It's possible to move around by dragging from the time graduation header only.

We should be able to move it by dragging from anywhere on the scrubber line.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug_1197192_-_Allow_dragging_the_scrubber_not_just.diff

This does the trick, and there's a new test.
This is however based on top of 2 other bugs that I'm going to land soon so I'll wait for that to happen before asking for review.

Comment 2

[Patrick Brosset <:pbro>]

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

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8671930 [details] [diff] [review]
Bug_1197192_-_Allow_dragging_the_scrubber_not_just.diff

This patch adds a new element in the scrubber, it's not visible but it's positioned on top of the red vertical 1px line and is thicker than it (6px). Hovering over it changes the cursor to col-resize so users know they can drag from it.
It's still possible to just click/drag in the header too.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8671930 [details] [diff] [review]
Bug_1197192_-_Allow_dragging_the_scrubber_not_just.diff

I just remember Mike is on PTO for a week, so I'm forwarding this to Alex. Feel free to ping me for more context if you need.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8671930 [details] [diff] [review]
Bug_1197192_-_Allow_dragging_the_scrubber_not_just.diff

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

We end up selecting text when dragging the vertical line...
It happens when you start/end dragging on top of a animation name.
We should either:
 * set -moz-user-select: none or equivalent trick to all text element (may be directly on the body?),
 * or prevent/cancel the events if we want to still allow user to copy paste these texts.

I know I'm not native, but "scrubber"? I don't see any meaningful match:
  https://translate.google.fr/#en/fr/scrubber

Also it is not directly related to this patch, but it may be even more discoverable if:
 * the scrubber was always visible, but I imagine it may have a performance impact?
 * the play button would actually work?!
   When you open the animation panel, nothing seems to be happenning when you hit this button?
   Does something actually happens? The UI blinks but nothing seems to be done.
   At least, if that was enabling the scrubber, it would help discovering it.

::: devtools/client/themes/animationinspector.css
@@ +202,5 @@
>  }
>  
> +/* The scrubber handle is a transparent element displayed on top of the scrubber
> +   line that allows users to drag it */
> +.animation-timeline .scrubber-handle {

I don't know the CSS best practices,
but we could help grok en node hierarchy by using:
  .animation-timeline .scrubber .scrubber-handle {

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8671930 [details] [diff] [review]
> Bug_1197192_-_Allow_dragging_the_scrubber_not_just.diff
> 
> Review of attachment 8671930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We end up selecting text when dragging the vertical line...
> It happens when you start/end dragging on top of a animation name.
> We should either:
>  * set -moz-user-select: none or equivalent trick to all text element (may
> be directly on the body?),
>  * or prevent/cancel the events if we want to still allow user to copy paste
> these texts.
Very good point, I'll get this fixed. Thanks.

> I know I'm not native, but "scrubber"? I don't see any meaningful match:
>   https://translate.google.fr/#en/fr/scrubber
I'm not native either, but the expression "to scrub the timeline" is a common way in animation tools (like AfterEffect and such) to talk about moving the vertical line that represents the current time back and forth. Hence, scrubber.
This was introduced in bug 1169563 in part reviewed by Mike and Tom who are both native English speakers, they did not seem to think this was weird, so I went with it.

> Also it is not directly related to this patch, but it may be even more
> discoverable if:
>  * the scrubber was always visible, but I imagine it may have a performance
> impact?
Yes, good point. When animations have finished playing, the scrubber sort of disappears to the right. I need to make sure it's always visible in the panel. Filed bug 1214664 for this since this is not directly related to this bug and more of a polish type of things.
>  * the play button would actually work?!
>    When you open the animation panel, nothing seems to be happenning when
> you hit this button?
>    Does something actually happens? The UI blinks but nothing seems to be
> done.
>    At least, if that was enabling the scrubber, it would help discovering it.
Odd, the button is supposed to work ... can you describe what you did? What test page did you use?
Maybe you bumped into bug 1211886 which I have a fix for but it hasn't yet gone in.
This isn't related to this bug either in fact, the goal here is really just to make the existing scrubber easier to drag around the panel.

> ::: devtools/client/themes/animationinspector.css
> @@ +202,5 @@
> >  }
> >  
> > +/* The scrubber handle is a transparent element displayed on top of the scrubber
> > +   line that allows users to drag it */
> > +.animation-timeline .scrubber-handle {
> 
> I don't know the CSS best practices,
> but we could help grok en node hierarchy by using:
>   .animation-timeline .scrubber .scrubber-handle {
I don't have a strong opinion about this. One of the best practices is to avoid nesting too much because that makes selectors have worse performance (the engine needs to do more checks), so I always try to simplify selectors while keeping at least some context.
But here the performance difference would be really minor, so if you think this helps reading the code, I'll add it.

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8671930 [details] [diff] [review]
> Bug_1197192_-_Allow_dragging_the_scrubber_not_just.diff
> 
> Review of attachment 8671930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We end up selecting text when dragging the vertical line...
> It happens when you start/end dragging on top of a animation name.
> We should either:
>  * set -moz-user-select: none or equivalent trick to all text element (may
> be directly on the body?),
>  * or prevent/cancel the events if we want to still allow user to copy paste
> these texts.
I ended up using -moz-user-select because I think it's important we allow to copy text from the panel (especially animation names, but there will be more text later when we introduce keyframes).

Comment 8

[Patrick Brosset <:pbro>]

Attachment: Bug_1197192_-_Allow_dragging_the_scrubber_not_just.diff

Comment 9

[Patrick Brosset <:pbro>]

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8671930&action=interdiff&newid=8673670&headers=1

Comment 10

[Patrick Brosset <:pbro>]

New try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3053c4adcd6

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8673670 [details] [diff] [review]
Bug_1197192_-_Allow_dragging_the_scrubber_not_just.diff

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

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6)
> > I know I'm not native, but "scrubber"? I don't see any meaningful match:
> >   https://translate.google.fr/#en/fr/scrubber
> I'm not native either, but the expression "to scrub the timeline" is a
> common way in animation tools (like AfterEffect and such) to talk about
> moving the vertical line that represents the current time back and forth.
> Hence, scrubber.

okok, just wanted to ensure it wasn't some crazy name we come up with ;)

> > Also it is not directly related to this patch, but it may be even more
> > discoverable if:
> >  * the scrubber was always visible, but I imagine it may have a performance
> > impact?
> Yes, good point. When animations have finished playing, the scrubber sort of
> disappears to the right. I need to make sure it's always visible in the
> panel. Filed bug 1214664 for this since this is not directly related to this
> bug and more of a polish type of things.
> >  * the play button would actually work?!
> >    When you open the animation panel, nothing seems to be happenning when
> > you hit this button?
> >    Does something actually happens? The UI blinks but nothing seems to be
> > done.
> >    At least, if that was enabling the scrubber, it would help discovering it.
> Odd, the button is supposed to work ... can you describe what you did? What
> test page did you use?
> Maybe you bumped into bug 1211886 which I have a fix for but it hasn't yet
> gone in.
> This isn't related to this bug either in fact, the goal here is really just
> to make the existing scrubber easier to drag around the panel.

google homepage for ex. Yes that can be bug 1211886.
As you just said, the scrubber is on the right and doesn't come back on the left and repeat the animation. Once the animation played once, the play button is basically useless, it doesn't revive the scrubber nor seems to do anything?

> I ended up using -moz-user-select because I think it's important we allow to
> copy text from the panel (especially animation names, but there will be more
> text later when we introduce keyframes).

Please use preventDefault then. You just have to call event.preventDefault from onScrubberMouseDown.
No need to modify animation-panel.js, introduce new event and CSS.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> Please use preventDefault then. You just have to call event.preventDefault
> from onScrubberMouseDown.
> No need to modify animation-panel.js, introduce new event and CSS.
Ah yes, that sounds simpler. I'll do that. Thanks.

Comment 13

[Pulsebot]

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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/36ade4d3ed36