1147981 - Cleanup doorhanger notification anchor ID & class duplication

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[Doug Turner (:dougt)]

In browser.css, there are a number of classes (fe, .geo-notification-icon) that are only used for the old notification bars.  We think we can remove these.

Comment 1

[Vaibhav Bhosale]

Hey Matt,
Can I work on this bug?
Thank You!

Comment 2

[Jesal]

Hi Matthew, could I please work on this bug? is this file located here: mozilla-source\mozilla-central\browser\themes\windows?

If this is correct, I can have a patch ready asap

Thanks

Jesal

Comment 3

[Jesal]

Hi Matthew,

I am guessing they need removing from windows. linux and osx files too?

Thanks 

Jesal

Comment 4

[Jesal]

Attachment: Bug 1147981 - Intial review patch for the removal of classes

Hi Matthew just removed the classes, from the directories. Just wondering if you could check and provide feedback to see if this is right?
Thanks

Jesal

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8585414 [details] [diff] [review]
Bug 1147981 - Intial review patch for the removal of classes

Review of attachment 8585414 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Jesal,

It's only the classes that should be removed for some rules that have both the class selector (starting with a period) and the ID selector starting with a `#`). You would need to look at the rules (not just geolocation) that have both versions for the same ruleset and figure out whether the class version is still used for a notification bar, instead of the newer PopupNotifications (aka. doorhangers) which used IDs.

You are correct about making this change on Mac, Linux and Windows but note that bug 1147281 is unifying this CSS so for now it makes sense to do the investigation into which class selectors for notification icons are still needed.

Comment 6

[Jesal]

Hi Matthew,

Just to confirm, when you say looking at the rules that have both versions for the same ruleset, does that mean repeating code, which isn't used anymore in other code not just geolocation?

Thanks

Jesal

Comment 7

[Matthew N. [:MattN]]

Yeah, I was thinking the problem applies to many of the notification icons but looking into this more, it seems like it's not as simple as I thought since our social chat feature (used by Firefox Hello) also uses the classes but only for notifications that it cares about. I think this isn't going to be that straightforward and will require changes to PopupNotifications and a few different places that I think it may be better to find a different mentored bug that interests you.

In the meantime bug 1147281 will make this a bit easier to fix later.

Sorry about that.

Comment 8

[Matthew N. [:MattN]]

Attachment: WIP MozReview Request: bz://1147981/MattN

/r/6511 - Bug 1147981 - Switch doorhanger anchor icons to use classes for icons to avoid ID conflicts and re-use by other iconboxes.

Pull down this commit:

hg pull -r 71abb21508e81d6053c8fcb1fab43ba0e2fe8178 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8587175 [details]
WIP MozReview Request: bz://1147981/MattN

This is a WIP to get rid of the ID & class CSS duplication. I know I need to change more of the CSS but I'm waiting on bug 1147281 and want to get feedback on the general idea and the PopupNotifications.jsm changes first. I'm going to do a try run to see which tests will need updating.

I kinda wonder if we should move to dynamically created <image> in the iconbox so other iconboxes (e.g. socialchat in the future) don't have to duplicate the <image>.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c67c6bc4f719

Comment 10

[Jesal]

Hi Matthew, 

thanks for your help, I will try to find another bug.

Jesal

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8587175 [details]
WIP MozReview Request: bz://1147981/MattN

LGTM

Comment 13

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1147981 - Switch doorhanger anchor icons to use classes for icons to avoid ID conflicts and re-use by other iconboxes.

Comment 14

[Johann Hofmann [:johannh]]

Matt, we'd love to finish this bug. Is there anything missing or any other reason you didn't get this landed yet? In that case I can take over and finish it if you like.

Thank you!

Comment 15

[Matthew N. [:MattN]]

Hey Johann,

I don't really remember off-hand and unfortunately didn't comment here. I suspect the main thing was just sanity checking this works with all of the consumers of the icons e.g. doorhanger anchors, notification bars and social chat permission anchors.

Comment 16

[Johann Hofmann [:johannh]]

Attachment: MozReview Request: Bug 1147981 - Switch doorhanger anchor icons to use classes for icons to avoid ID conflicts and re-use by other iconboxes.

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

Comment 17

[Johann Hofmann [:johannh]]

Ok! I rebased your patch onto central and tried to root out all failing tests and other consumers of these icons. Let's give it a try (hur hur) and see how that goes.

Comment 18

[Johann Hofmann [:johannh]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bfdf0ba3ae4

Comment 19

[Johann Hofmann [:johannh]]

Comment on attachment 8746559 [details]
MozReview Request: Bug 1147981 - Switch doorhanger anchor icons to use classes for icons to avoid ID conflicts and re-use by other iconboxes.

Hey Matt, we'd like to know what you think of the patch, to see roughly how much additional work needs to go into it, assuming bug 1268619 eventually lands. I know this is a big change, sorry, but your feedback would be very useful!

Thank you!

Comment 20

[Matthew N. [:MattN]]

Adding addon-compat since this may break extensions creating PopupNotifications… perhaps that's a problem.

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8746559 [details]
MozReview Request: Bug 1147981 - Switch doorhanger anchor icons to use classes for icons to avoid ID conflicts and re-use by other iconboxes.

Bug 1193006 is going to use these classes now for the icons in the control center section so that solves the initial issue of comment 0 (unused rules).

Since I think it makes sense for the doorhanger anchors to remain IDs (since you can only anchor at one place) and changing the IDs would break addon-compat, I think the thing remaining could be to remove the IDs from the CSS and use the classes for the <image> too. The social chat stuff could probably still be simplified but I still don't fully understand the complication there after reading bug 1130356 comment 12 and that can be done in its own bug.

Here's my proposal in list form:
A) Copy the <image> IDs to its class attribute.
B) Remove #*-notification-icon references from CSS, replacing them with .*-notification-icon, if necessary. Make sure this doesn't cause issues with the change in specificity.
C) Leave socialchat.xml as-is.

Basically, the ID is used for anchoring but the class is used for styling.

Comment 22

[Johann Hofmann [:johannh]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #21)
> Basically, the ID is used for anchoring but the class is used for styling.

Yeah, I think that makes most sense right now, especially with addon-compat and other work going on. It's also conveniently easy to do.

Thanks for looking into this :)

Comment 23

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #21)
> A) Copy the <image> IDs to its class attribute.

To align with bug 1193006, I suggest using preferentially a class name like "geo-icon" for permissions, which are aligned with the IDs used with nsIPermissions. For example:

<image id="geo-notification-icon" class="notification-anchor-icon geo-icon" .../>

In the Control Center we will generate images to put near permission rows, like:

<image class="identity-popup-permission-icon geo-icon"/>

So our CSS would normally style both icons using ".geo-icon", but if we need the glyph or the color to be different in some cases we could still use more specific styles like ".notification-anchor-icon.geo-icon" or add SVG filters.

Since this bug would affect custom themes anyways, it's a good opportunity to do the renaming.

Comment 24

[:Paolo Amadini]

And we could use for example "identity-popup-permission-icon geo-icon blocked" for crossed out icons.

Comment 25

[Matthew N. [:MattN]]

Good idea, I was originally avoiding changing the ID in the other proposals since it's used in API calls for the anchor and I didn't want to break extensions but changing the class should break them for anchors, only the small change the class version was being used to style something else. Sounds good to me.

Comment 26

[:Paolo Amadini]

I have a patch that I still have to test, but I'm having trouble doing that due to the crash in bug 1274282, so I'll post the current version for review even if it may require adjustments after testing.

Comment 27

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1147981 - Cleanup doorhanger notification anchor ID & class duplication. r=MattN,mikedeboer

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

Comment 28

[:Paolo Amadini]

Note that I've kept the styles that are specific to the notification area (like first time animations) linked to the URL bar anchor IDs instead of the generic icon class. Also, I've kept all the hover and active effects for now, even if they may be removed or made consistent in bug 1193006.

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8754395 [details]
MozReview Request: Bug 1147981 - Cleanup doorhanger notification anchor ID & class duplication. r=MattN,mikedeboer

https://reviewboard.mozilla.org/r/53946/#review50722

Can you check if you need to update `PopupNotifications._updateAnchorIcons` and please test with Hello to make sure the icons work the same there.

Can you do a try push of a WIP bug 1268619 before and after this patch then we can compare with https://screenshots.mattn.ca/compare/ to avoid obvious regressions.

::: browser/base/content/browser.xul:762
(Diff revision 1)
>                         aria-label="&urlbar.loginFillNotificationAnchor.label;"/>
> -                <image id="password-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="password-notification-icon" class="notification-anchor-icon login-icon" role="button"
>                         aria-label="&urlbar.passwordNotificationAnchor.label;"/>
> -                <image id="plugins-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="plugins-notification-icon" class="notification-anchor-icon plugin-icon" role="button"
>                         aria-label="&urlbar.pluginsNotificationAnchor.label;"/>
> -                <image id="web-notifications-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="web-notifications-notification-icon" class="notification-anchor-icon desktop-notification-icon" role="button"

please use "web-notifications-icon" as "desktop-notification" is the legacy name and only used for the permission entry. There is nothing "desktop"-specific about them.

::: browser/base/content/browser.xul:766
(Diff revision 1)
>                         aria-label="&urlbar.pluginsNotificationAnchor.label;"/>
> -                <image id="web-notifications-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="web-notifications-notification-icon" class="notification-anchor-icon desktop-notification-icon" role="button"
>                         aria-label="&urlbar.webNotsNotificationAnchor3.label;"/>
> -                <image id="webRTC-shareDevices-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="webRTC-shareDevices-notification-icon" class="notification-anchor-icon camera-icon" role="button"
>                         aria-label="&urlbar.webRTCShareDevicesNotificationAnchor.label;"/>
> -                <image id="webRTC-sharingDevices-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="webRTC-sharingDevices-notification-icon" class="notification-anchor-icon camera-icon inuse" role="button"

Nit: it seems like most of our classes related to this use hyphens so "in-use" would make this match.

::: browser/base/content/browser.xul:776
(Diff revision 1)
>                         aria-label="&urlbar.webRTCSharingMicrophoneNotificationAnchor.label;"/>
> -                <image id="webRTC-shareScreen-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="webRTC-shareScreen-notification-icon" class="notification-anchor-icon screen-icon" role="button"
>                         aria-label="&urlbar.webRTCShareScreenNotificationAnchor.label;"/>
> -                <image id="webRTC-sharingScreen-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="webRTC-sharingScreen-notification-icon" class="notification-anchor-icon screen-icon inuse" role="button"
>                         aria-label="&urlbar.webRTCSharingScreenNotificationAnchor.label;"/>
> -                <image id="pointerLock-notification-icon" class="notification-anchor-icon" role="button"
> +                <image id="pointerLock-notification-icon" class="notification-anchor-icon pointerLock-icon" role="button"

"pointer-lock-icon" icon while your cleaing it up?

::: browser/themes/shared/notification-icons.inc.css:118
(Diff revision 1)
> +/* This class can be used alone or in combination with the class defining the
> +   type of icon displayed. This rule must be defined before the others in order
> +   for its list-style-image to be overridden. */

Thanks for the comment

Comment 30

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #29)
> Can you check if you need to update `PopupNotifications._updateAnchorIcons`

Filed bug 1275558 to remove this logic, doing this in a separate bug is better because it might affect add-ons using their own CSS. I also wonder if the logic is there in the first place only for add-ons, since we used to style using the ID.

> "pointer-lock-icon" icon while your cleaing it up?

Went for the simpler "pointer-icon" since we're using a name different from the permission ID anyways.

Comment 31

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #29)
> Can you do a try push of a WIP bug 1268619 before and after this patch then
> we can compare with https://screenshots.mattn.ca/compare/ to avoid obvious
> regressions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=febd1c350f89
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92056508d480

Comment 32

[:Paolo Amadini]

Comment on attachment 8754395 [details]
MozReview Request: Bug 1147981 - Cleanup doorhanger notification anchor ID & class duplication. r=MattN,mikedeboer

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

Comment 33

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #29)
> and please test with Hello to make sure the icons work the same there.

Mike, can you take a look at the socialchat.xml changes, and tell me how I can test them?

Comment 34

[Mike de Boer [:mikedeboer]]

Comment on attachment 8754395 [details]
MozReview Request: Bug 1147981 - Cleanup doorhanger notification anchor ID & class duplication. r=MattN,mikedeboer

https://reviewboard.mozilla.org/r/53946/#review51758

::: browser/base/content/socialchat.xml:19
(Diff revision 2)
>          </xul:hbox>
>          <xul:toolbarbutton anonid="webRTC-shareScreen-icon"
> -                           class="notification-anchor-icon chat-toolbarbutton"
> +                           class="notification-anchor-icon chat-toolbarbutton screen-icon"
>                             oncommand="document.getBindingParent(this).showNotifications(this); event.stopPropagation();"/>
>          <xul:toolbarbutton anonid="webRTC-sharingScreen-icon"
> -                           class="notification-anchor-icon chat-toolbarbutton"
> +                           class="notification-anchor-icon chat-toolbarbutton screen-icon in-use"

The only way you can test this is manually by opening a chat window from the Hello panel. It starts sharing your screen right away, so that should work.

The interesting part is the 'notification-icon' below, which is a combined fallback anchor that combines the icons for mic and webcam in the case of Hello, because no separate notification anchors are specified for it.
None of your changes should impact that behavior, though.

IOW, I don't see any issues.

Comment 35

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #30)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #29)
> > Can you check if you need to update `PopupNotifications._updateAnchorIcons`
> 
> Filed bug 1275558 to remove this logic, doing this in a separate bug is
> better because it might affect add-ons using their own CSS. I also wonder if
> the logic is there in the first place only for add-ons, since we used to
> style using the ID.

I think the social chat icons for Hello used the classes and that's what the code was added for. We should sanity check we don't regress that.

Comment 36

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #31)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #29)
> > Can you do a try push of a WIP bug 1268619 before and after this patch then
> > we can compare with https://screenshots.mattn.ca/compare/ to avoid obvious
> > regressions.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=febd1c350f89
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=92056508d480

Thanks. Comparison: https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=febd1c350f89b279313d869592b711103b38380a&newProject=try&newRev=92056508d480cf3715633684b3faea47b2269b47

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 8754395 [details]
MozReview Request: Bug 1147981 - Cleanup doorhanger notification anchor ID & class duplication. r=MattN,mikedeboer

https://reviewboard.mozilla.org/r/53946/#review52118

Thanks!

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #36)
> (In reply to :Paolo Amadini from comment #31)
> > (In reply to Matthew N. [:MattN] (behind on reviews) from comment #29)
> > > Can you do a try push of a WIP bug 1268619 before and after this patch then
> > > we can compare with https://screenshots.mattn.ca/compare/ to avoid obvious
> > > regressions.
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=febd1c350f89
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=92056508d480
> 
> Thanks. Comparison:
> https://screenshots.mattn.ca/compare/
> ?oldProject=try&oldRev=febd1c350f89b279313d869592b711103b38380a&newProject=tr
> y&newRev=92056508d480cf3715633684b3faea47b2269b47

I had to retrigger the OS X build since it was busted so didn't run the ss job but otherwise it looks fine. I didn't manually test.

Comment 38

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #35)
> I think the social chat icons for Hello used the classes and that's what the
> code was added for. We should sanity check we don't regress that.

Ah, so indeed there was a regression as Hello used the classes. I've added back the Hello-specific selectors for now.

Thanks Mike for the test instructions, I've now seen where this icons appear. When I stop sharing they simply disappear. So, the only cases we need to handle are .webRTC-sharingDevices-notification-icon and .webRTC-sharingMicrophone-notification-icon, in other words the orange "in use" cases, is this correct? Screen sharing has its own anchor icon, that I've already updated with the right classes in the code.

By the way, I'm not fond of this UI - it's a bit confusing because when you click the icons, it says "you're sharing your camera with this page", which is not correct. I suspect that a different UI than popup notifications might serve your use case better, if all you need to do is to inform the user they're sharing or allow the device to be changed.

Comment 39

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #38)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #35)
> > I think the social chat icons for Hello used the classes and that's what the
> > code was added for. We should sanity check we don't regress that.
> 
> Ah, so indeed there was a regression as Hello used the classes. I've added
> back the Hello-specific selectors for now.
> 
> Thanks Mike for the test instructions, I've now seen where this icons
> appear. When I stop sharing they simply disappear. So, the only cases we
> need to handle are .webRTC-sharingDevices-notification-icon and
> .webRTC-sharingMicrophone-notification-icon, in other words the orange "in
> use" cases, is this correct? Screen sharing has its own anchor icon, that
> I've already updated with the right classes in the code.

Correct.

> By the way, I'm not fond of this UI - it's a bit confusing because when you
> click the icons, it says "you're sharing your camera with this page", which
> is not correct. I suspect that a different UI than popup notifications might
> serve your use case better, if all you need to do is to inform the user
> they're sharing or allow the device to be changed.

Tell me about it! Since Hello is currently the only consumer of the socialchat.xml bindings, it should be fine to have a custom indicator instead.
But it's more interesting and perhaps more feasible in the shorter term, to get rid of the combined notification anchor first. So separate audio/ video and screen sharing notification anchors and forget about all the other types, because Hello never uses them. In that case we can clean up PopupNotifications.jsm a bit.

Comment 40

[:Paolo Amadini]

(In reply to Mike de Boer [:mikedeboer] from comment #39)
> Tell me about it! Since Hello is currently the only consumer of the
> socialchat.xml bindings, it should be fine to have a custom indicator
> instead.

Yeah, a specific UI might allow for a better interaction anyways.

> But it's more interesting and perhaps more feasible in the shorter term, to
> get rid of the combined notification anchor first.

I've copied this comment to bug 1275558, which I've also referenced in the source code.

Comment 41

[:Paolo Amadini]

Comment on attachment 8754395 [details]
MozReview Request: Bug 1147981 - Cleanup doorhanger notification anchor ID & class duplication. r=MattN,mikedeboer

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

Comment 42

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69aa899553ca

Comment 43

[:Paolo Amadini]

Ok, there are some tests to update here to ignore the extra classes.

Comment 44

[:Paolo Amadini]

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

Comment 45

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6cfd5ff960f7

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cfd5ff960f7