Closed
Bug 1294541
Opened 8 years ago
Closed 8 years ago
Rename @keyframes pulse in identity-block.inc.css and tabs.inc.css
Categories
(Firefox :: Theme, defect, P3)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 53
People
(Reporter: dao, Assigned: vladzuga)
References
Details
(Keywords: good-first-bug, Whiteboard: [fxprivacy][good first bug][lang=css])
Attachments
(1 file, 1 obsolete file)
@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•8 years ago
|
Priority: -- → P3
Updated•8 years ago
|
Whiteboard: [fxprivacy]
Reporter | ||
Comment 1•8 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)
Comment 2•8 years ago
|
||
(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 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•8 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]
Comment 5•8 years ago
|
||
(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.
I am currently working on it. Can someone assign it to me, please?
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•8 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•8 years ago
|
||
Could you please merge your two patches into one?
Assignee | ||
Comment 12•8 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•8 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•8 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) |
Attachment #8824377 -
Attachment is obsolete: true
Attachment #8824377 -
Flags: review?(dao+bmo)
Reporter | ||
Comment 16•8 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•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment 20•8 years ago
|
||
We've built 51 RC. Mark 51 won't fix.
You need to log in
before you can comment on or make changes to this bug.
Description
•