Rename @keyframes pulse in identity-block.inc.css and tabs.inc.css

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Theme
P3
normal
RESOLVED FIXED
10 months ago
4 months ago

People

(Reporter: dao, Assigned: Vlad Zuga)

Tracking

({good-first-bug})

Trunk
Firefox 53
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox51 wontfix, firefox53 fixed)

Details

(Whiteboard: [fxprivacy][good first bug][lang=css])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 months ago
@keyframes definitions share one namespace in any given document (in this case the browser.xul document). Therefore each definition needs an unambiguous name to avoid collisions. "pulse" doesn't meet that criteria.

Straw man name proposal: sharing-icon-attention

(I also think "sharing" is too generic. It wasn't immediately clear to me that this was about media devices when I first spotted this code.)
(Reporter)

Updated

10 months ago
Priority: -- → P3

Updated

10 months ago
Whiteboard: [fxprivacy]
(Reporter)

Comment 1

8 months ago
Turns out there's another @keyframes pulse definition in tabs.inc.css. This also ends up in browser.css. One definition overriding the other doesn't matter in this case since they're the same, but this is still bad practice.
Flags: needinfo?(florian)
(In reply to Dão Gottwald [:dao] from comment #1)
> Turns out there's another @keyframes pulse definition in tabs.inc.css. This
> also ends up in browser.css. One definition overriding the other doesn't
> matter in this case since they're the same, but this is still bad practice.

Ideally I would have wanted these 2 definitions to be the same, but I didn't find a good place to make it shared between tabs and notifications. Do you have an idea of where it could go?
Flags: needinfo?(florian)
(Reporter)

Comment 3

8 months ago
We don't have a shared file for CSS that doesn't belong to some specific feature, so for the time being I'd suggest just changing the two definitions to use different names.
Summary: Rename @keyframes pulse in identity-block.inc.css → Rename @keyframes pulse in identity-block.inc.css and tabs.inc.css
(Reporter)

Comment 4

8 months ago
For lack of a better suggestion, let's go with identity-box-sharing-icon-pulse in identity-block.inc.css and tab-sharing-icon-pulse in tabs.inc.css.

Here's the relevant code:

https://dxr.mozilla.org/mozilla-central/rev/b1d60f2f68c7cccc96fcf9a2075bb430a500a0f2/browser/themes/shared/identity-block/identity-block.inc.css#109-121

https://dxr.mozilla.org/mozilla-central/rev/b1d60f2f68c7cccc96fcf9a2075bb430a500a0f2/browser/themes/shared/tabs.inc.css#92-104
Keywords: good-first-bug
Whiteboard: [fxprivacy] → [fxprivacy][good first bug][lang=css]
(In reply to Dão Gottwald [:dao] from comment #4)
> For lack of a better suggestion, let's go with
> identity-box-sharing-icon-pulse in identity-block.inc.css and
> tab-sharing-icon-pulse in tabs.inc.css.

I would suggest adding a comment above each of them giving the name of the other and saying they should remain identical.
(Assignee)

Comment 6

5 months ago
I am currently working on it. Can someone assign it to me, please?
Done!
Assignee: nobody → vladzuga
Comment hidden (mozreview-request)
(Reporter)

Comment 9

5 months ago
mozreview-review
Comment on attachment 8823626 [details]
Bug 1294541 - Renamed pulse in identity-block.inc.css and tabs.inc.css.

https://reviewboard.mozilla.org/r/102162/#review103006

::: browser/themes/shared/identity-block/identity-block.inc.css:118
(Diff revision 1)
>    animation-delay: -1.5s;
>  }
>  
>  #identity-box[sharing] > #identity-icon,
>  #sharing-icon {
>    animation: 3s linear pulse infinite;

You also need to rename the animation here.

::: browser/themes/shared/tabs.inc.css:94
(Diff revision 1)
>    list-style-image: url("chrome://mozapps/skin/places/defaultFavicon.png");
>  }
>  
>  .tab-icon-image[sharing]:not([selected]),
>  .tab-sharing-icon-overlay {
>    animation: 3s linear pulse infinite;

Same here.
Attachment #8823626 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 11

5 months ago
Could you please merge your two patches into one?
(Assignee)

Comment 12

5 months ago
(In reply to Dão Gottwald [:dao] from comment #11)
> Could you please merge your two patches into one?

Sure!

As I am new with mercurial and the general workflow, can you please tell me how can I do that? Do I have to use hg merge (former changeset ID) while the working directory is the latest changeset?
I know it's probably trivial, but this my first bug, so thanks for your patience!
(Reporter)

Comment 13

5 months ago
(In reply to Vlad Zuga from comment #12)
> (In reply to Dão Gottwald [:dao] from comment #11)
> > Could you please merge your two patches into one?
> 
> Sure!
> 
> As I am new with mercurial and the general workflow, can you please tell me
> how can I do that? Do I have to use hg merge (former changeset ID) while the
> working directory is the latest changeset?
> I know it's probably trivial, but this my first bug, so thanks for your
> patience!

It depends on how exactly you produced your patches. Mercurial allows many different workflows and unfortunately I'm only familiar with some of them. Are you on IRC (https://wiki.mozilla.org/IRC)? If you raise this question in the #introduction channel, I think you have a very good chance of getting an answer.
(Assignee)

Comment 14

5 months ago
(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Vlad Zuga from comment #12)
> > (In reply to Dão Gottwald [:dao] from comment #11)
> > > Could you please merge your two patches into one?
> > 
> > Sure!
> > 
> > As I am new with mercurial and the general workflow, can you please tell me
> > how can I do that? Do I have to use hg merge (former changeset ID) while the
> > working directory is the latest changeset?
> > I know it's probably trivial, but this my first bug, so thanks for your
> > patience!
> 
> It depends on how exactly you produced your patches. Mercurial allows many
> different workflows and unfortunately I'm only familiar with some of them.
> Are you on IRC (https://wiki.mozilla.org/IRC)? If you raise this question in
> the #introduction channel, I think you have a very good chance of getting an
> answer.

Yes, I am and I will ask the guys over there. Thanks!
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8824377 - Attachment is obsolete: true
Attachment #8824377 - Flags: review?(dao+bmo)
(Reporter)

Comment 16

5 months ago
mozreview-review
Comment on attachment 8823626 [details]
Bug 1294541 - Renamed pulse in identity-block.inc.css and tabs.inc.css.

https://reviewboard.mozilla.org/r/102162/#review103514

Perfect, thanks!
Attachment #8823626 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)

Comment 18

5 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab220a16bda2
Rename @keyframes pulse in identity-block.inc.css and tabs.inc.css. r=dao

Comment 19

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab220a16bda2
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
We've built 51 RC. Mark 51 won't fix.
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.