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.

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

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

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.

/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 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

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

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

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!

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.

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

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.

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

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!

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

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.

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

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

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

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.

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.

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

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

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

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

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

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

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

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

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

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

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