Closed Bug 1031021 Opened 6 years ago Closed 6 years ago

Change popup notification removeOnDismissal to false

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently, the update popup notification has removeOnDismissal: true.

I didn't realize what this feature did when I set that flag. Benjamin says we want it to be false so the icon persists in the location bar.

Open issues:

1) Do we want a custom icon? Currently, it's the default blue "I" AFAICT

2) The notification doesn't reappear on Firefox 10 but it does on Firefox 28. I suspect our custom XBL for the notification (necessary to backport some PopupNotification features to Firefox 10) doesn't like older Firefox.
I'm looking into this.
Assignee: nobody → gps
Status: NEW → ASSIGNED
The notification unbreaks in Firefox 15. Now, to find what changed in that release...
I'm pretty sure https://hg.mozilla.org/mozilla-central/rev/aeea5b83cf89 is the relevant commit. Reading it now.
I played around with things for a few hours and have nothing to show for it. I just don't grok all the XBL/DOM/CSS magic happening with popup notifications.

I suspect getting this to work before and after aeea5b83cf89 is possible, but I'm not sure what that solution entails. Furthermore, I doubt my ability to efficiently arrive at a solution.

I need help with this one.

Matt: Do you have any ideas? Existing hotfix notification code can be found at https://hg.mozilla.org/releases/firefox-hotfixes/file/de957c193904/v20140527.01/content/notification.css and https://hg.mozilla.org/releases/firefox-hotfixes/file/de957c193904/v20140527.01/content/notification.xml. I'm highly confident aeea5b83cf89 is what needs worked around in Firefox 10 through 14. Reading the code paths, I'm not sure how to do this without monkeypatching PopupNotifications.jsm. And I consider that a solution of last resort.
Flags: needinfo?(MattN+bmo)
Boriss: I'd like your UX opinion on this.

Current:

https://people.mozilla.org/~gszorc/hotfix-win7-notification-removed.webm

When the user clicks outside the pop-up (this is called "dismissing" the notification), the notification disappears completely. There is no icon in the notifications tray.

Proposed:

https://people.mozilla.org/~gszorc/hotfix-win7-notification-dismissed.webm

When the notification is dismissed, we see an icon in the notifications tray. That icon can be clicked to re-show the notification. The icon doesn't disappear until Firefox is upgraded.

Buggy Behavior:

https://people.mozilla.org/~gszorc/hotfix-win7-notification-busted.webm

On Firefox 10 through 14, we are yet unable to make the icon in the tray work. The icon appears, but clicking on it results in the entire tray collapsing. We can't make the notification re-appear.

Unless MattN or someone else provides a workaround, we'll mitigate this obvious UX badness by not enabling the proposed dismissal/tray behavior on Firefox <15.

Open Issues:

1) Is the proposed behavior of dismissing the notification to a tray icon acceptable?
2) If so, is the default "blue I" icon sufficient? Do you want to provide a custom icon?
Flags: needinfo?(jboriss)
(In reply to Gregory Szorc [:gps] from comment #5)
> 1) Is the proposed behavior of dismissing the notification to a tray icon
> acceptable?

You're right: leaving the tray icon visible and clickable is preferable.  It's how we treat most other permission dialogs, and without doing so the user has no way to return to the dialog if they accidentally dismiss it with a click.

> 2) If so, is the default "blue I" icon sufficient? Do you want to provide a
> custom icon?

Yes, that "i" icon needs an improvement.  Ideally we could use the Firefox logo favicon, since this is 1. a request from Firefox and 2. that's the icon used in the dialog itself.

The Firefox icon would be preferable.  The only poor case would be if we show another firefox favicon in about:home, displaying two.  Gps, would that ever be the case in these older builds?
Flags: needinfo?(jboriss) → needinfo?(gps)
I'm not sure what, if anything, would produce a Firefox icon notification. I suspect Gavin could answer this off the top of his head though!
Flags: needinfo?(gps) → needinfo?(gavin.sharp)
Gavin is on vacation. Nothing else currently uses a "Firefox icon notification" but it's not hard to make that happen. I believe about:home has the Firefox favicon for a while so there would be two of the same/similar beside each other.

I haven't looked into the issue yet but the videos help me understand the problem.
Flags: needinfo?(gavin.sharp)
Here's a video with the Firefox logo as the tray icon:

https://people.mozilla.org/~gszorc/hotfix-win7-notification-tray-icon.webm

I'm spot testing this on various versions before I publish a patch for review.
Attachment #8448975 - Flags: review?(MattN+bmo)
Attachment #8448975 - Flags: feedback?(benjamin)
Attachment #8448921 - Attachment is obsolete: true
Attachment #8448975 - Flags: feedback?(benjamin) → feedback+
(In reply to Gregory Szorc [:gps] from comment #9)
> Here's a video with the Firefox logo as the tray icon:
> 
> https://people.mozilla.org/~gszorc/hotfix-win7-notification-tray-icon.webm
> 
> I'm spot testing this on various versions before I publish a patch for
> review.

Redundant, and not great, but not the end of the world.  Greg, how hard would it be to have that notification spawn from the Firefox identity panel (http://cl.ly/image/0M1m280A3y2U) instead, such that users clicking it would see the install notification rather than the site identity panel?
Flags: needinfo?(gps)
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #12)
> (In reply to Gregory Szorc [:gps] from comment #9)
> > Here's a video with the Firefox logo as the tray icon:
> > 
> > https://people.mozilla.org/~gszorc/hotfix-win7-notification-tray-icon.webm
> > 
> > I'm spot testing this on various versions before I publish a patch for
> > review.
> 
> Redundant, and not great, but not the end of the world.  Greg, how hard
> would it be to have that notification spawn from the Firefox identity panel
> (http://cl.ly/image/0M1m280A3y2U) instead, such that users clicking it would
> see the install notification rather than the site identity panel?

I, uh, am not sure. I didn't know you could click on that panel! Considering that panel was introduced in Firefox 20-something, we're talking about more version-conditional code. My opinion is we should avoid that type of behavior in a hotfix unless it is absolutely necessary (things are already pretty complicated the way they are).

Is this request absolutely necessary to avoid user anarchy?
Flags: needinfo?(gps)
Comment on attachment 8448975 [details] [diff] [review]
Do not remove popup on dismissal (if it works)

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

r=me but I'd prefer we didn't have adjacent icons that are the same.

::: v20140527.01/content/notification.css
@@ +12,5 @@
>    -moz-margin-end: 10px;
>  }
> +
> +#upgrade-notification-tray-icon {
> +  list-style-image: url(chrome://firefox-hotfix/content/firefox16.png);

As I said on IRC, I think having adjacent icons that are the same are worse than the default icon but that's just my opinion.

I also don't think we should change the behaviour of the identity panel in the hotfix.
Attachment #8448975 - Flags: review?(MattN+bmo) → review+
Please reopen if you want to change the UI.

Clearing MattN needinfo since we worked around the problem on Firefox <15 and we presumably don't care enough to make these versions consistent with 15+. We can always revisit later if it turns out Firefox 10-14 users aren't upgrading.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(MattN+bmo)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.