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

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P3)

Description

[Dão Gottwald [:dao]]

@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.)

Comment 1

[Dão Gottwald [:dao]]

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.

Comment 2

[Florian Quèze [: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?

Comment 3

[Dão Gottwald [:dao]]

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.

Comment 4

[Dão Gottwald [:dao]]

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

Comment 5

[Florian Quèze [:florian]]

(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.

Comment 6

[Vlad Zuga]

I am currently working on it. Can someone assign it to me, please?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Done!

Comment 8

[Vlad Zuga]

Attachment: Bug 1294541 - Renamed pulse in identity-block.inc.css and tabs.inc.css.

Review commit: https://reviewboard.mozilla.org/r/102162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102162/

Comment 9

[Dão Gottwald [:dao]]

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.

Comment 10

[Vlad Zuga]

Attachment: Bug 1294541 - Changed animations names.

Review commit: https://reviewboard.mozilla.org/r/102694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102694/

Comment 11

[Dão Gottwald [:dao]]

Could you please merge your two patches into one?

Comment 12

[Vlad Zuga]

(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!

Comment 13

[Dão Gottwald [:dao]]

(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.

Comment 14

[Vlad Zuga]

(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 15

[Vlad Zuga]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102162/diff/1-2/

Comment 16

[Dão Gottwald [:dao]]

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!

Comment 17

[Vlad Zuga]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102162/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab220a16bda2

Comment 20

[Gerry Chang [:gchang]]

We've built 51 RC. Mark 51 won't fix.