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

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

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?

Comment 1

[Jonathan Kingston [:jkt] he/him]

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!

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1320314 - Moving notification popup layout overrides for panel arrows into a shared file

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

Comment 3

[Dão Gottwald [:dao]]

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.

Comment 4

[Dão Gottwald [:dao]]

Any update on this?

Comment 5

[:Paolo Amadini]

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

Comment 6

[:Paolo Amadini]

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?

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Johann Hofmann [:johannh]]

As discussed on IRC, I'm taking this because I'd like to resolve bug 1319732. :)

Comment 9

[Johann Hofmann [:johannh]]

Attachment: Bug 1320314 - Move popup notification layout overrides for arrow panels into a shared file.

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

Comment 10

[Dão Gottwald [:dao]]

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.

Comment 11

[Johann Hofmann [:johannh]]

Good catch! I'll also do a mozscreenshots push to make sure all platforms look fine.

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8824371 [details]
Bug 1320314 - Move popup notification layout overrides for arrow panels into a shared file.

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

Comment 13

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=883a2f71e658

Comment 14

[Dão Gottwald [:dao]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1eb8dcc1427f
Move popup notification layout overrides for arrow panels into a shared file. r=dao

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1eb8dcc1427f