1260912 - Instead of generating background image in JS in drawGraphElementBackground, use CSS styling

Status: RESOLVED FIXED

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

Description

[Alfred Kayser]

In drawGraphElementBackground, a RGB image is composed to make a graph background consisting of a series of vertical lines, with the color "hardcoded" in the JS file.

All this can be replaced by a simple border-inline-start on the .time-tick class and using absolute positioning to extend the .time-tick to the full view:

.time-tick{
	position:absolute;
	top:0;
	bottom:0;
	padding-top:3px;
	border-left:1px dotted threedshadow;
	color:graytext}

This will also dynamically size with changing view port size, without having to recalculate/render the graph image background.

Comment 1

[Alfred Kayser]

Also when the graph view is changed, during the change the lines themselves will stay 1px wide, instead growing bigger or smaller, which makes the change more smooth.

Comment 2

[Patrick Brosset <:pbro>]

You're absolutely right, this would simplify the code a lot, and make the resizing much smoother. Thanks for filing!
Would you like to take this on? Setting myself as mentor in case this helps.

Comment 3

[Patrick Brosset <:pbro>]

While I think of it, I haven't tested this, but I'm guessing we'll need pointer-events:none on the thing to avoid it conflicting with being able to grab the scrubber element and clicking on animations.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

my two cents: especially now that we're using SVG icons in devtools, and they looks sharper than ever, we should take in account displays with higher dpi than 1 (at least 2, as we're doing in other part of Firefox); so we probably want to have the pixels halved in such cases (e.g. border-left: 0.5px).

Comment 5

[Phil [:philr]]

Hi! Can I work on this? I already managed to make it work, but need to clean up everything and prepare patch. Thanks!

Comment 6

[Patrick Brosset <:pbro>]

Sure, especially if you have already started working on it.
Bugzilla tells me you're new here, so you might be interested in our contribution guide: https://developer.mozilla.org/en-US/docs/Tools/Contributing
I'm assigning the bug to you now. Feel free to reach out here, or on IRC (#devtools : https://wiki.mozilla.org/DevTools/GetInvolved#Communication), this is often a much better way to get quick help when you're having problems.

Comment 7

[Phil [:philr]]

Attachment: Bug1260912.patch

Here is the patch!

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8739776 [details] [diff] [review]
Bug1260912.patch

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

Generally looks really good. Thanks for the code cleanup.
I just have a couple of comments about the CSS changes.

::: devtools/client/themes/animationinspector.css
@@ +26,3 @@
>    --time-graduation-border-color: rgba(128, 136, 144, .5);
> +  /* The color of the time tick borders */
> +  --time-tick-border-color: rgba(128, 136, 144, .375);

Let's use just one variable for these border colors:

--time-graduation-border-color: rgba(128, 136, 144, .5);

And use it for both the time-tick's left border and the animation's top border (which is already the case). These 2 colors are very close, and I don't think we need 2 separate variables for them.

@@ +208,5 @@
>    left: var(--timeline-sidebar-width);
>    /* Leave the width of a marker right of a track so the 100% markers can be
>       selected easily */
>    right: var(--keyframes-marker-size);
> +  height: 100%;

I think it might be safer to keep .track-container elements at var(--timeline-animation-height) but then use a different way to make the ticks be full height (see below).

@@ +258,5 @@
>  
>  .animation-timeline .time-header .time-tick {
>    position: absolute;
> +  top: 0;
> +  bottom: 0;

Instead of bottom: 0, you could use height: 100vh;

Comment 9

[Phil [:philr]]

Attachment: Bug1260912.patch

I've updated the patch, according to the comments.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8744712 [details] [diff] [review]
Bug1260912.patch

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

Thanks this looks good.
Please add "; r=pbro" at the end of your commit message.
I have pushed this patch to the TRY server so we can make sure all tests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a13269d25a6
Let's wait until it turns out green and then land this change.

Comment 11

[Phil [:philr]]

Attachment: Bug1260912.patch

Added reviewer to the commit, also all checks are green!

Comment 12

[Patrick Brosset <:pbro>]

Thanks Phil! Please let me know if you're looking for other bugs to fix.

Comment 13

[Pulsebot]

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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/078fd576aba0