Closed Bug 1260912 Opened 8 years ago Closed 8 years ago

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


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



(firefox49 fixed)

Firefox 49
Tracking Status
firefox49 --- fixed


(Reporter: alfredkayser, Assigned: philr, Mentored)



(1 file, 2 obsolete files)

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:

	border-left:1px dotted threedshadow;

This will also dynamically size with changing view port size, without having to recalculate/render the graph image background.
Component: Developer Tools → Developer Tools: Animation Inspector
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.
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.
Mentor: pbrosset
Priority: -- → P3
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.
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).
Hi! Can I work on this? I already managed to make it work, but need to clean up everything and prepare patch. Thanks!
Flags: needinfo?(pbrosset)
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:
I'm assigning the bug to you now. Feel free to reach out here, or on IRC (#devtools :, this is often a much better way to get quick help when you're having problems.
Assignee: nobody → philipp
Flags: needinfo?(pbrosset)
Attached patch Bug1260912.patch (obsolete) — Splinter Review
Here is the patch!
Attachment #8739776 - Flags: review?(pbrosset)
Comment on attachment 8739776 [details] [diff] [review]

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;
Attachment #8739776 - Flags: review?(pbrosset)
Attached patch Bug1260912.patch (obsolete) — Splinter Review
I've updated the patch, according to the comments.
Attachment #8739776 - Attachment is obsolete: true
Attachment #8744712 - Flags: review?(pbrosset)
Comment on attachment 8744712 [details] [diff] [review]

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:
Let's wait until it turns out green and then land this change.
Attachment #8744712 - Flags: review?(pbrosset) → review+
Attached patch Bug1260912.patchSplinter Review
Added reviewer to the commit, also all checks are green!
Attachment #8744712 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Attachment #8744958 - Flags: review+
Thanks Phil! Please let me know if you're looking for other bugs to fix.
Flags: needinfo?(pbrosset)
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.