Cleanup doorhanger notification anchor ID & class duplication

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dougt, Assigned: Paolo)

Tracking

(Blocks: 1 bug, {addon-compat})

unspecified
Firefox 49
addon-compat
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Mentor: MattN+bmo@mozilla.com
Component: General → Theme
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: good first bug → [good first bug][lang=css]
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)

Comment 4

3 years ago
Created attachment 8585414 [details] [diff] [review]
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
Attachment #8585414 - Flags: review?(MattN+bmo)
Comment hidden (obsolete)
Assignee: nobody → kjjcrm1
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)

Comment 6

3 years ago
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
Flags: needinfo?(MattN+bmo)
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.
Assignee: kjjcrm1 → nobody
Mentor: MattN+bmo@mozilla.com
Status: ASSIGNED → NEW
Depends on: 1147281
Flags: needinfo?(MattN+bmo)
Summary: Remove old style notification classes in browser.css → Cleanup doorhanger notification anchor ID & class duplication
Whiteboard: [good first bug][lang=css]
Created attachment 8587175 [details]
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 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
Attachment #8587175 - Attachment description: MozReview Request: bz://1147981/MattN → WIP MozReview Request: bz://1147981/MattN
Attachment #8587175 - Flags: feedback?(mixedpuppy)

Comment 10

3 years ago
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
Attachment #8587175 - Flags: feedback?(mixedpuppy) → feedback+
Comment on attachment 8587175 [details]
WIP MozReview Request: bz://1147981/MattN
Attachment #8587175 - Attachment is obsolete: true
Created attachment 8619881 [details]
MozReview Request: Bug 1147981 - Switch doorhanger anchor icons to use classes for icons to avoid ID conflicts and re-use by other iconboxes.

Updated

3 years ago
Blocks: 1188472
Priority: -- → P3
Whiteboard: [fxprivacy]

Updated

3 years ago
Priority: P3 → P2

Updated

2 years ago
Priority: P2 → P3
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!
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)

Updated

2 years ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify?
Priority: P3 → P1
Created 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.

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

Updated

2 years ago
Flags: qe-verify? → qe-verify-
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!
Attachment #8746559 - Flags: feedback?(MattN+bmo)

Updated

2 years ago
Iteration: 49.1 - May 9 → 49.2 - May 23
Adding addon-compat since this may break extensions creating PopupNotifications… perhaps that's a problem.
Keywords: addon-compat
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.
Attachment #8746559 - Flags: feedback?(MattN+bmo)
(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 :)
(Assignee)

Comment 23

2 years ago
(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.
(Assignee)

Comment 24

2 years ago
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.
(Assignee)

Comment 26

2 years ago
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.
(Assignee)

Comment 27

2 years ago
Created attachment 8754395 [details]
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/
Attachment #8754395 - Flags: review?(MattN+bmo)

Updated

2 years ago
Assignee: jhofmann → paolo.mozmail
(Assignee)

Comment 28

2 years ago
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
Attachment #8754395 - Flags: review?(MattN+bmo)

Updated

2 years ago
Depends on: 1274480

Updated

2 years ago
Iteration: 49.2 - May 23 → 49.3 - Jun 6

Updated

2 years ago
Blocks: 1193006
(Assignee)

Updated

2 years ago
Blocks: 1275558
(Assignee)

Comment 30

2 years ago
(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.
(Assignee)

Comment 31

2 years ago
(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
(Assignee)

Comment 32

2 years ago
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/
Attachment #8754395 - Attachment description: MozReview Request: Bug 1147981 - Cleanup doorhanger notification anchor ID & class duplication. r=MattN → MozReview Request: Bug 1147981 - Cleanup doorhanger notification anchor ID & class duplication. r=MattN,mikedeboer
Attachment #8754395 - Flags: review?(mdeboer)
Attachment #8754395 - Flags: review?(MattN+bmo)
(Assignee)

Comment 33

2 years ago
(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?
Flags: needinfo?(mdeboer)
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.
Attachment #8754395 - Flags: review?(mdeboer) → review+
(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.
Attachment #8754395 - Flags: review?(MattN+bmo) → review+
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.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 38

2 years ago
(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.
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 40

2 years ago
(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.
(Assignee)

Comment 41

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8619881 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8746559 - Attachment is obsolete: true
(Assignee)

Comment 43

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

Comment 46

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6cfd5ff960f7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.