Closed Bug 1320314 Opened 5 years ago Closed 5 years ago

Move #notification-popup > .panel-arrowcontainer > .panel-arrowcontent rule away from browser/themes/shared/notification-icons.inc.css

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: dao, Assigned: johannh)

References

Details

Attachments

(1 file, 1 obsolete file)

https://hg.mozilla.org/mozilla-central/diff/d04cc86f4313/browser/themes/shared/notification-icons.inc.css

notification-icons.inc.css is, as the name says, for notification icons. The #notification-popup > .panel-arrowcontainer > .panel-arrowcontent rule belongs somewhere else, presumably toolkit/themes/shared/popupnotification.inc.css?
Flags: needinfo?(jkt)
I remember this at the time I couldn't find a better place to put this, the file you mention was my first port of call however it isn't applied here despite the similar name. The other files are not shared so I have created a shared include however I'm not really sure if this is right.

Submitted for your review anyway, thanks for reminding me about this!
Flags: needinfo?(jkt)
Assignee: nobody → jkt
Comment on attachment 8814483 [details]
Bug 1320314 - Moving notification popup layout overrides for panel arrows into a shared file

AFAIK notification-popup lives completely in browser land, and toolkit should be agnostic to the ID used there.

Here's what I think we should do:

1. Stop removing the padding. Instead, reuse --arrowpanel-padding as a negative margin on the action buttons.

2. Remove the overflow:hidden thing. Instead, reuse --arrowpanel-border-radius on the action buttons.
Attachment #8814483 - Flags: review?(dao+bmo) → review-
Any update on this?
Flags: needinfo?(jkt)
(In reply to Dão Gottwald [:dao] from comment #3)
> 1. Stop removing the padding. Instead, reuse --arrowpanel-padding as a
> negative margin on the action buttons.
> 2. Remove the overflow:hidden thing. Instead, reuse
> --arrowpanel-border-radius on the action buttons.

Using specific rounded borders and negative margins would make the CSS for the buttons much more complicated, because we have to do things like re-ordering the buttons depending on the platform, hiding or showing buttons depending on the situation, and of course we have to support LTR more. Each situation requires a specific rule for which corner needs to become round. Moreover, we use the same technique in various panels, and we would need to add these rules controlling the button shapes to each panel.

This is what we used to do in some panels, and by using the much simpler "overflow: hidden;" technique I fixed several existing visual bugs caused by missing rules in some cases.

The need for "display: block;" associated to "overflow:hidden;" is quite likely related to bug 1293242, so when fixing that bug we can remove this property.
To solve the issue in comment 0, can we just move the code to "browser.css", or alternatively take the approach that you suggested of moving this to the Toolkit arrow panel styling, similar to Jonathan's patch but with a class or attribute instead of an ID?
Flags: needinfo?(jkt) → needinfo?(dao+bmo)
(In reply to :Paolo Amadini from comment #6)
> To solve the issue in comment 0, can we just move the code to "browser.css",

It doesn't really belong in browser/ at all since popup notifications are implemented in toolkit and shouldn't depend on some obscure CSS in browser/ in order to be displayed correctly.

> or alternatively take the approach that you suggested of moving this to the
> Toolkit arrow panel styling, similar to Jonathan's patch but with a class or
> attribute instead of an ID?

Yes, PopupNotifications.jsm could set a class on the panel and we could use that in global.css (I don't think it belongs in popup.css).
Flags: needinfo?(dao+bmo)
As discussed on IRC, I'm taking this because I'd like to resolve bug 1319732. :)
Assignee: jkt → jhofmann
Status: NEW → ASSIGNED
Attachment #8814483 - Attachment is obsolete: true
Comment on attachment 8824371 [details]
Bug 1320314 - Move popup notification layout overrides for arrow panels into a shared file.

https://reviewboard.mozilla.org/r/102896/#review103374

It looks like we currently don't preprocess global.css on Linux, so you'll need to change that.
Attachment #8824371 - Flags: review?(dao+bmo) → review-
Good catch! I'll also do a mozscreenshots push to make sure all platforms look fine.
Blocks: 1319732
Comment on attachment 8824371 [details]
Bug 1320314 - Move popup notification layout overrides for arrow panels into a shared file.

https://reviewboard.mozilla.org/r/102896/#review103390
Attachment #8824371 - Flags: review?(dao+bmo) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1eb8dcc1427f
Move popup notification layout overrides for arrow panels into a shared file. r=dao
https://hg.mozilla.org/mozilla-central/rev/1eb8dcc1427f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.