1229340 - Overflow scrollbar causes an offset between animations and the time header in the animation timeline

Status: RESOLVED FIXED

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

Description

[Patrick Brosset <:pbro>]

STR:
- open a page with a lot of animations, or with a lot of animated properties,
- make sure the toolbox is small enough that there's a scrollbar in the animation-inspector
- you can then slowly make the toolbox higher until you see the scrollbar disappear.

Notice how when the scrollbar appears/disappears the animations in the timeline adapt to the removed/added horizontal space.
However the header (which contains time graduations) does not adapt. That's because the scroll bar doesn't apply to it.

If we move the scrollbar to be on the whole timeline element, then the problem goes away, but scrolling down makes the header disappear while we'd like to make sure it's always visible.

This only occurs with the patches for bug 1228005 that I'm working on, but I decided to move this to another bug because it's not a blocker and the fix is sufficiently independent that I'd like to do it later.

Comment 1

[Alfred Kayser]

A couple of css tricks to make the timeline not impacted by scrollbar:
:root{--scrollbar-width:16px}
.animation{width:100vw}
.time-block,
.track-container{right:calc(var(--keyframes-marker-size) + var(--scrollbar-width))}
.animated-properties{width:calc(100vw - var(--scrollbar-width))}

100vw is the whole view with (same value for with/without scrollbar)
substraction of --scrollbar-width makes sure that the animation lines don't get overlapped by the scrollbar.

Comment 2

[Patrick Brosset <:pbro>]

Triaging (filter on CLIMBING SHOES).

Comment 3

[Ricky Chien [:rickychien] (inactive)]

(In reply to Patrick Brosset <:pbro> from comment #0)
> Notice how when the scrollbar appears/disappears the animations in the
> timeline adapt to the removed/added horizontal space.
> However the header (which contains time graduations) does not adapt. That's
> because the scroll bar doesn't apply to it.

I'm curious what's problem / unexpected behaviors on this bug? I tried STR but can't see any weird things around the header. What's the expected behavior that header should adapt? If possible, could you upload a picture or video to explain what you want? thanks!

Comment 4

[Patrick Brosset <:pbro>]

Attachment: keyframes.html

- open the attached test case,
- open the toolbox (on the inspector tab),
- open the "animations" tab in the inspector,
- click on the orange bar (the animation named wow), so that animated properties and keyframes are displayed,
- make the toolbox small until a vertical scrollbar appears in the animation panel,

==> you should see that when the scrollbar appears, it pushes the animation orange bar and keyframes to be shorter, because the scrollbar takes some space. However, the header (where the time graduations are displayed) and the scrubber (the 1px red vertical line indicating the current time) *do not* move. Therefore, an offset is introduced and the header and scrubber do not indicate the right time anymore.

I should note that I'm testing on windows, with the light devtools theme, where scrollbars aren't floating. If you have floating scrollbars it might not be a problem. I think on mac where there are floating scrollbars there is a system setting to force them on.

I'll attach a gif in a second.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: scrollbar-problem.gif

Comment 6

[Alfred Kayser]

The issue is real (even if Mac people are not able to reproduce...)
The solution is provided (see comment #1) (note, the scrollbar size doesn't have to be precise, as it is only about making sure the animation bars don't disappear behind the (floating) scrollbar...

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Mac are able to reproduce by setting a concrete scrollbar in System Preferences -> General -> Show scroll bars and select Always.

Comment 8

[Ricky Chien [:rickychien] (inactive)]

The concrete scroll bar doesn't appear in dark theme on Mac. Switching to light theme can reproduce this problem as expected.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1229340 - Move animation inspector scrollbar to timeline container

Review commit: https://reviewboard.mozilla.org/r/55786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55786/

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Attachment: Screen Shot 2016-05-27 at 6.11.11 PM.png

I only check my patch on Mac and it looks good to me for 16px of scroll bar width (you can see new effect in attachment).

I haven't applied all suggestions from comment 1 and var(--scrollbar-width) for track-container seems good to me.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

https://reviewboard.mozilla.org/r/55786/#review53084

::: devtools/client/themes/animationinspector.css:52
(Diff revision 1)
> +  /* The wide of the scrollbar */
> +  --scrollbar-width: 16px;

The problem with this is scrollbars can have various widths.
By default, scrollbars look different under different OSes.
On top of this, I'm fairly certain that users can customize their scrollbars on Linux (maybe even in Windows, but it doesn't seem to be an easy to find setting).
So hard-coding 16px here isn't going to do the trick in all situations.

The other problem I can see with this is that in many cases, there isn't a need for a scrollbar. There are sufficiently few animations displayed that the scrollbar doesn't appear.
In those cases, we're loosing 16px horizontally.

There might be a different solution, using CSS sticky positioning. Here's a quick test I made on jsbin: http://jsbin.com/digohorumu/edit?html,css,output
Notice how the header (yellow) and items (red) are aligned (I'v used 50% elements to make sure they were aligned), and notice how if you remove items, the scrollbar disappears, both header and items take the full width and are still aligned.

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/1-2/

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Hi Patrick, I haven't find out final a solution for scrubber to apply "position: sticky", so current scrubber doesn't display correctly when you scroll down the panel. Could you give me feedback of current implementation?

Any possible routes to make scrubber sticky doesn't work for me. I don't have clue how to achieve that. If you know how to do that, any suggestions are welcome. :)

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/2-3/

Comment 15

[Ricky Chien [:rickychien] (inactive)]

I'm still get stuck with the scrubber head position which is unable to stick at the top of time-header.

Applied "position: sticky" in ".animation-timeline .scrubber::before" can achieve the effect that leads to scrubber head to be fixed at the top of time-header (top: 0 works!) but it won't apply "right: -6px" and so looks weird.

Is pseudo element will apply "position: sticky" as well?

Comment 16

[Patrick Brosset <:pbro>]

Ah, you're right, it's not as easy as I thought it would be. I just spent 30 minutes playing around with this some more and couldn't figure out a way to deal the time-ticks and the scrubber nicely.
So I guess you're first approach is our only solution. However I'd really like the scrollbar width to be computed dynamically. To do this we have 2 options:
- first, the bad option: using nsIDOMWindowUtils.getScrollbarSize. It's bad because it's a privileged API that only code running in Firefox chrome-scope can call. But we want to get rid of this type of code to make the devtools run as a web page.
- Second, compute the size in Javascript dynamically. Can I suggest that you investigate this solution?
This requires having either a dummy DOM element in the panel (visibility:hidden and position:absolute so it doesn't impact the rest) that you can use for this, or calculating it with the animation-timeline element directly.
In any case, you need 2 elements:
<div scroller>
  <div inner />
</div>

and you need to do something like this:
scroller.style.overflowY = "hidden";
let w1 = inner.getBoxQuads()[0].bounds.width;
scroller.style.overflowY = "scroll";
let w2 = inner.getBoxQuads()[0].bounds.width;
scroller.style.overflowY = "";
let scrollbarWidth = w1 - w2;

This needs to be done only once, when the panel initializes for example.
If you're able to do this, then you could set this as a css variable:

document.documentElement.style.setProperty("--scrollbar-width", scrollbarWidth  + "px");

Then, in your first patch, just remove --scrollbar-width from the CSS, and the rest will be OK.
Hope this helps!

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Clearing as per previous comment.

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/3-4/

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Thank you Patrick! I haven't thought it needs so many works to achieve this effect! However, I figured out a good CSS solution after playing with it for a while.
I moved pseudo element of triangle indicator to scrubber-handle and tweaked the right position a little bit and it looks great to me!
There is one thing I've concerned that hover area (col-resize) of cursor on scrubber-handle is a little bit partial left but it's not pretty noticeable.

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Try tests failed on Mac OS due to our system preference is default to "always" for show scroll bars in try server's Mac machine. After trying ./mach test devtools/client/animationinspector/test/browser_animation_timeline_header.js along with a "when scrolling" that works good as expected and can passed the tests.

Is there any way to enable "when scrolling" on all Mac machines? I hope that we can find out a good way that doesn't have to modify our tests in order to avoid breaking tests on other platforms.

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Root cause of failures are found.

Unfortunately, our case ran into a boundary of time-tick changes. That means time ticks will be expanded as there is enough space around it. We default set "Always" show scroll bar on our Mac CI machine to run tests so that scroll bar forced timeHeaderEl to shrink its width and coincidentally hit the boundary and decrease the amount of .time-tick too.

There is nothing we can do at offsetWidth [1] but it causes [2] nb to be incorrect. Maybe we could modify findOptimalTimeInterval()[3]. I don't understand it well but any helps would be appreciated!

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/test/browser_animation_timeline_header.js#25
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/test/browser_animation_timeline_header.js#33
[3] https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/utils.js#59-86

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

https://reviewboard.mozilla.org/r/55786/#review55038

This is definitely getting better. However I did find a couple of bugs:
- if you switch devtools to the dark theme, the time header remains white, it should get a dark color like the rest of the UI
- if you make the toolbox taller than the content displayed in the animation tool (so that there's no scrollbar and there is empty content at the bottom), then you can see that the scrubber doesn't extend all the way to the bottom anymore
- also, related, the time tick separators (the 1px gray vertical bars) don't extend to the bottom anymore. In fact they only cover the time-header, making them way less useful.

::: devtools/client/animationinspector/components/animation-timeline.js:95
(Diff revision 4)
>        }
>      });
>      this.scrubberHandleEl.addEventListener("mousedown",
>        this.onScrubberMouseDown);
>  
> -    this.timeHeaderEl = createNode({
> +    let header = createNode({

`headerWrapper` would be a better name for this.
Also its class should be `header-wrapper`.
This way it's clearer to people that this element is just here to wrap some CSS logic.

::: devtools/client/animationinspector/components/animation-timeline.js:98
(Diff revision 4)
>        this.onScrubberMouseDown);
>  
> -    this.timeHeaderEl = createNode({
> +    let header = createNode({
>        parent: this.rootWrapperEl,
>        attributes: {
> +        "class": "header track-container"

Here you're creating a new parent element with the "track-container" class, but later you're setting this element as sticky. 
The "track-container" wasn't made for this.
You should remove the class here and in the CSS just add `height: var(--timeline-animation-height);` since this is the only css property you need from the class.

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/4-5/

Comment 24

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/5-6/

Comment 25

[Ricky Chien [:rickychien] (inactive)]

Patch updated!
I cannot find out a better solution for adjusting scrubber and time-tick elements. My solution is that calculate their height manually by removing extra padding or margin. However, now it works great to me. If you have a better CSS tips to achieve to the same effect please tell me :)

Comment 26

[Ricky Chien [:rickychien] (inactive)]

Test still failed for the same reason mentioned on comment 21.
My guess that implementation of findOptimalTimeInterval() was correct but actual result (width) of drawHeaderAndBackground [1] was wrong. 

Number of time-ticks should be 2 instead of 4 since there are some label overlay in time-ticks panel. However, I cannot get correct width for timeHeaderEl if scrollbar appears.

I think when drawHeaderAndBackground() first time invoked, scrollbar doesn't show up immediately. As a result, the width of animationsEl is incorrect and drawHeaderAndBackground() doesn't update itself again after scrollbar comes out so that we cannot set up the width (minus scrollbar width) by using the width of animationsEl.

Patrick, do you have a better idea? thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/components/animation-timeline.js#448

Comment 27

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/6-7/

Comment 28

[Ricky Chien [:rickychien] (inactive)]

Finally, I found out a way to workaround test failures by expanding min-width from 250px to 350px so that we can avoid this edge cases of time-ticks. I'm excited that we just need to modify one line CSS to fix all these failures. However, I'm not sure it's appropriate solution for you since the min-width of inspector might be defined in a spec that we shouldn't touch.

Comment 29

[Patrick Brosset <:pbro>]

(In reply to Ricky Chien [:rickychien] from comment #28)
> Finally, I found out a way to workaround test failures by expanding
> min-width from 250px to 350px so that we can avoid this edge cases of
> time-ticks. I'm excited that we just need to modify one line CSS to fix all
> these failures. However, I'm not sure it's appropriate solution for you
> since the min-width of inspector might be defined in a spec that we
> shouldn't touch.
You're right, we don't want to change the min-width of the sidebar, since that has a direct impact on the UI of the inspector, and people are used to being able to resize the sidebar to 250px.
If this is causing problems in tests only, it would be easy enough to resize the sidebar just in tests by doing this when the test starts:

yield pushPref("devtools.toolsidebar-width.inspector", 350);

With this, the size of the sidebar will be 350px just for the duration of the test.

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

https://reviewboard.mozilla.org/r/55786/#review59002

I found one more weird CSS issue. Sorry this is taking so long, but the CSS for this is getting complex and so there are edge cases:
- make the toolbox small so there is a scrollbar in the animation panel
- scroll down to the bottom of the animation panel
- now make the toolbox tall again
- notice that the scrollbar doesn't go away as it should, it only goes away when you scroll back up again.
This looks like an issue with using position sticky. I don't know how much you can do about this.

See my comments below. I let you decide if you want to push more in the current direction, knowing that it's getting more complex, or if you're willing to go back to the previous proposal.

::: devtools/client/inspector/inspector.css:7
(Diff revision 7)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #inspector-sidebar {
> -  min-width: 250px;
> +  min-width: 350px;

See comment 29, this should be removed.

::: devtools/client/themes/animationinspector.css:251
(Diff revision 7)
>    height: 100%;
>  }
>  
>  .animation-timeline .scrubber {
>    position: absolute;
> -  height: 100%;
> +  height: calc(100vh - 22px);

22px is a "magic number" here, and we should avoid this.
I guess you cannot use 100% here otherwise the scrubber won't expand all the way to the bottom when the toolbox is tall.

So you need a way to avoid hard-coding this number.
My understanding is that 22px is the sum of the --toolbar-height and 2 1px borders.

So, at the very least, this should be:

```
/* Make the scrubber as tall as the viewport minus the
  toolbar height and the borders */
height: calc(100vh - var(--toolbar-height) - 2px);
```

::: devtools/client/themes/animationinspector.css:292
(Diff revision 7)
> +.animation-timeline .header-wrapper {
> +  position: sticky;
> +  top: 0;
> +  background-color: var(--theme-body-background);
> +  border-bottom: 1px solid var(--time-graduation-border-color);
> +  z-index: 3;

Why 3 here? I see the time-tick borders above the animations, they should be in the background instead.

Oh, I see, if you don't use this, then when you scroll, the animation blocks are displayed above the header, which we don't want.

I don't know how to solve this problem off the top of my head.
On one hand, we want the time blocks to be below the header, but on the other hand, we want the time-tick borders to be below the time blocks. But the borders are actually part of the header, making this really hard.

Maybe this will require to move the time-ticks out of the header div, and into another track-container next to it that has a lower z-index?

I don't know, this is getting so complex that I'm starting to wonder if we shouldn't go back to previous proposal: auto-detect scrollbar width in JS, set it as a CSS variable, and then use this to just offset the whole timeline all the time, even if there is no scrollbar needed. At least the amount of CSS changes required for this would be minimal.

Comment 31

[Ricky Chien [:rickychien] (inactive)]

I tried to separate out time-tick span into header part and without header part, but it undoubtedly introduced other issues that aren't easy to fix.
Is it acceptable that we just fix this issue by a transparent time-tick borders?

Comment 32

[Patrick Brosset <:pbro>]

(In reply to Ricky Chien [:rickychien] from comment #31)
> I tried to separate out time-tick span into header part and without header
> part, but it undoubtedly introduced other issues that aren't easy to fix.
> Is it acceptable that we just fix this issue by a transparent time-tick
> borders?
What do you mean by transparent? We still want to see the vertical time-tick lines. If you make them semi-transparent, they might be really hard to see, but also, they'd still be displayed on top of the animation blocks.
Either you need to find something smart to position them at the bottom, while the header is at the top, or you need to go back to the other solution as I said in my previous comment:

(In reply to Patrick Brosset <:pbro> from comment #30)
> I'm starting to wonder if we
> shouldn't go back to previous proposal: auto-detect scrollbar width in JS,
> set it as a CSS variable, and then use this to just offset the whole
> timeline all the time, even if there is no scrollbar needed. At least the
> amount of CSS changes required for this would be minimal.

Comment 33

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/7-8/

Comment 34

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/8-9/

Comment 35

[Ricky Chien [:rickychien] (inactive)]

(In reply to Patrick Brosset <:pbro> from comment #30)
> I found one more weird CSS issue. Sorry this is taking so long, but the CSS
> for this is getting complex and so there are edge cases:
> - make the toolbox small so there is a scrollbar in the animation panel
> - scroll down to the bottom of the animation panel
> - now make the toolbox tall again
> - notice that the scrollbar doesn't go away as it should, it only goes away
> when you scroll back up again.
> This looks like an issue with using position sticky. I don't know how much
> you can do about this.

Patch has updated to address above issue and I finally found out a way to make vertical time-tick lines to be displayed behind the animations.

Comment 36

[Patrick Brosset <:pbro>]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

https://reviewboard.mozilla.org/r/55786/#review60570

A few final comments, but I believe we're there now!
So please do address the comments below and re-upload a new version without requesting a new review.
Can you push to TRY too?
Oh, also, it's best practice to explain what you fixed in the commit message, not just re-iterate the problem or bug title. So something like "move scrollbars to timeline container and use sticky position on header" or something like this, maybe shorter. You can always have a very short first line, and then explain what you changed in more details in following lines of the commit message. This is useful when going through the change history of a file in Mercurial.

::: devtools/client/animationinspector/components/animation-timeline.js:477
(Diff revision 9)
> +    this.timeTickEl.innerHTML = "";
>  
>      for (let i = 0; i <= width / intervalWidth; i++) {
>        let pos = 100 * i * intervalWidth / width;
>  
>        createNode({

Can you please add a comment here explaining where header-item elements are displayed.

::: devtools/client/animationinspector/components/animation-timeline.js:487
(Diff revision 9)
>            "style": `left:${pos}%`
>          },
>          textContent: TimeScale.formatTime(TimeScale.distanceToRelativeTime(pos))
>        });
> +
> +      createNode({

Can you please add a comment here explaining where time-tick elements are displayed.

::: devtools/client/animationinspector/test/browser_animation_timeline_header.js:19
(Diff revision 9)
>  // animation-timeline.js
>  const TIME_GRADUATION_MIN_SPACING = 40;
>  
>  add_task(function* () {
>    yield addTab(URL_ROOT + "doc_simple_animation.html");
> +  yield pushPref("devtools.toolsidebar-width.inspector", 350);

This requires a comment. People looking at the test in the future may wonder why we're making the sidebar 350px here. Please add a comment that explains why this is required in this test.

::: devtools/client/themes/animationinspector.css:315
(Diff revision 9)
> +.animation-timeline .time-body {
> +  height: 100%;
> +}
> +
> +.animation-timeline .time-body .time-tick {
> +  cursor: col-resize;

This should be removed. We don't want any specific cursor on time-ticks.

Comment 37

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/9-10/

Comment 38

[Ricky Chien [:rickychien] (inactive)]

Updated patch and wait for try!

Comment 39

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8757247 [details]
Bug 1229340 - Move animation inspector scrollbar to timeline container

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55786/diff/10-11/

Comment 40

[Ricky Chien [:rickychien] (inactive)]

Everything looks great! Thank you Patrick!

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/17ba38d33d7b
Move animation inspector scrollbar to timeline container r=pbro

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17ba38d33d7b