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

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dao, Assigned: vladzuga)

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])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years 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

3 years ago
Priority: -- → P3

Updated

3 years ago
Whiteboard: [fxprivacy]
(Reporter)

Comment 1

3 years 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

3 years 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

3 years 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

2 years 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

2 years 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

2 years ago
Could you please merge your two patches into one?
(Assignee)

Comment 12

2 years 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

2 years 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

2 years 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

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

Comment 16

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab220a16bda2
Status: NEW → RESOLVED
Last Resolved: 2 years 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.