Use SVG context-fill instead of clip-path for animation-fast-track.svg

RESOLVED FIXED in Firefox 56

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: ntim, Assigned: Towkir, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 56

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
in https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/animationinspector.css#459 :
-moz-context-properties: fill;
fill: (...)

then in the SVG:

<!-- 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/. -->
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 9 12" width="16" height="16">
  <path d="M5.75 0l-1 5.5 2 .5-3.5 6 1-5-2-.5z" fill="context-fill"/>
</svg>
Mentor: ntim.bugs
Keywords: good-first-bug
Priority: -- → P3
(Assignee)

Updated

2 years ago
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Posted patch svg-context-fill.patch (obsolete) — Splinter Review
Looks good on my local build, hope it works ;)
Attachment #8880275 - Flags: review?(ntim.bugs)
(Reporter)

Comment 2

2 years ago
Comment on attachment 8880275 [details] [diff] [review]
svg-context-fill.patch

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

::: devtools/client/themes/animationinspector.css
@@ +475,3 @@
>  }
>  
>  .animation-timeline .some-properties .name::after {

I know it wasn't part of the bug initially, but the rules on .animation-timeline .some-properties .name::after and .animation-timeline .all-properties .name::after are identical.

Can you remove this block, and just share the selector ?

.animation-timeline .all-properties .name::after,
.animation-timeline .some-properties .name::after {
  -moz-context-properties: fill;
  ...
}
Attachment #8880275 - Flags: review?(ntim.bugs) → feedback+
(Reporter)

Comment 3

2 years ago
Thanks for working on this btw, the patch works great!
(Reporter)

Comment 4

2 years ago
Comment on attachment 8880275 [details] [diff] [review]
svg-context-fill.patch

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

::: devtools/client/themes/animationinspector.css
@@ +469,5 @@
>  
>  .animation-timeline .all-properties .name::after {
> +  -moz-context-properties: fill;
> +  fill: var(--theme-content-color3);
> +  background-image: url(images/animation-fast-track.svg#thunderbolt);

Oh, I almost forgot, can you remove #thunderbolt from the URL ? #thunderbolt references to the old clip path, that's now removed.
(Assignee)

Comment 5

2 years ago
Should I nit the code like this ?:
> background-image: url("images/animation-fast-track.svg");

I am talking about the quotation, currently there is no quotes.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 6

2 years ago
However, I have made the changes. Please have a look.
Attachment #8880275 - Attachment is obsolete: true
Attachment #8882161 - Flags: review?(ntim.bugs)
(Reporter)

Comment 7

2 years ago
(In reply to [:Towkir] Ahmed from comment #5)
> Should I nit the code like this ?:
> > background-image: url("images/animation-fast-track.svg");
> 
> I am talking about the quotation, currently there is no quotes.

I don't mind either way to be honest.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 8

2 years ago
Comment on attachment 8882161 [details] [diff] [review]
svg-context-fill.patch

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

Looks good to me!
Attachment #8882161 - Flags: review?(ntim.bugs) → review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab62d15c1ca
Use SVG context-fill instead of clip-path for animation-fast-track.svg now. r=ntim
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aab62d15c1ca
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.