661148 - Use PopupNotification.jsm in Fennec (doorhangers)

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: WIP

We have talked before about moving to doorhanger notifications in Fennec in order to fix a few bugs with our current notification system. Doorhanger notifications are easier to dismiss, but can be brought back up if the user wants to address them later. They also reduce the number of buttons we need to show (one default with hidden sub-options) which should be simpler for users and help with localization problems in notifications. They would also let us do things like putting the entire addon-installation flow inside one popup-notification rather than spreading it across system-notifications, toasters, dialogs, and notification boxes in the addon manager.

I tried last week to get this up and running and see where the trouble spots would be. My hope was to use the same PopupNotificaions.jsm file that desktop uses. Since there isn't really a ux-spec for this in Fennec, wanted to at least document what I have here for any future discussions.

Since we don't have a lot of space in the urlbar for what desktop calls an "iconbox", I settled for just showing them attached the favicon. Initially notifications are shown alone (or in groups if there are multiple notifications). Dismissed (but not removed) notifications are shown below the Identitiy area when when the site button is clicked.

Screenshots of what I have currently are at:

http://www.flickr.com/photos/digdug2k/sets/72157626695710681/

There were a few problems with PopupNotifications.jsm that made this hard. The only one I don't quite understand is that putting non-anonymous menuitems inside an anonymous menupopup isn't working. To fix that I just made them both non-anonymous, but I'd want a better solution. I've hacked around some other problems (we don't want to hide our "iconbox" and we don't have a tabbrowser element), but I think we can find easy solutions there that work for both us and desktop firefox.

Comment 1

[Wesley Johnston (:wesj)]

Build is at: http://dl.dropbox.com/u/72157/fennec-doorhangers.apk

Comment 2

[Wesley Johnston (:wesj)]

Attachment: WIP Part 1/2

Back on this (sorry for the delay).

Splitting this into changes needed in PopupNotifications.jsm and changes needed in mobile. This is the former.

Mostly we need 1.) to not worry about whether tabbrowser is a xul element (only that it implements the methods we need), and to not hide the  iconbox (just setting an attribute instead.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: WIP Part 2/2

This is part 2. I feel like I'm mostly putting in hacks all over the place and will need to come back and clean most of them up later. This handles locking the toolbars when notifations are shown, adds a little page curl to the site button, adds some methods on to arrowbox that we don't have on mobile, animates the menu appearance.... generally just makes this work.

Still finding bugs. Haven't even started testing on device. I'll keep digging at them. Screenshot of the pagecurl I made coming...

Comment 4

[Wesley Johnston (:wesj)]

Attachment: screenshot

Comment 5

[Wesley Johnston (:wesj)]

One more issue I've run into thats got me stumped has to do with the menulist buttons where I am hiding "secondary" actions. That is a menulist element, with a menupopup inside it, all inside the notification binding, so both elements are anonymous.

PopupNotifications.jsm adds menuitems as children of the notification binding, and they are moved (via xbl:children) into the menupopup. When the menulist button is clicked it checks for menupopup.childNodes.length > 0 and returns false.

I can see the childNodes attached in DOMi though, and I've checked and am detecting the correct menupopup. Me and vivien also played with turning ON native theming to see if it was interfering somehow, but nothing really changed.

So I'm not sure why these children aren't showing up. In the first WIP I put up here, I worked around it by building the entire menu in PopupNotifications.jsm, but we shouldn't HAVE to do that. Still trying to dig around figure out what is going on here...

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 546672 [details] [diff] [review]
WIP Part 1/2

Getting feedback on these changes from Gavin. These ideas look ok, or do you have something else you'd rather see?

One last change I need to make involves not requiring accesskeys on commands.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 546672 [details] [diff] [review]
WIP Part 1/2

Can you explain why you needed to make each of these changes? The hasNotifications change will impact other consumers, so it would be nice to avoid. You also shouldn't need to null check secondaryActions, given the fallback in the Notification constructor.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Oh, I read more of comments. Could you avoid needing to make the iconBox changes by using a dummy iconBox for mobile (a box that can be hidden/unhidden without, affecting the UI)? I guess that might be tricky.

Comment 9

[Wesley Johnston (:wesj)]

I initially did things by adding a box "next" to the urlbar-favicon, similar to desktop. That puts the arrowbox in the wrong position. We could work around that using strange margins or transforms...

Alternatively, I could potentially add a secret object to the favicon-stack... I can try it out, but it IS useful for our favicon box to know that we're showing a notification, or that we have hidden notifications that the user might need to deal with.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #8)
> Oh, I read more of comments. Could you avoid needing to make the iconBox
> changes by using a dummy iconBox for mobile (a box that can be
> hidden/unhidden without, affecting the UI)? I guess that might be tricky.

tricky and icky

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I guess you could leave the hasNotifications attribute setting as additional functionality, and add a parameter to the PopupNotifications constructor that controls whether it hides the iconBox (defaulting to the current behavior of "true").

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 546672 [details] [diff] [review]
WIP Part 1/2

The other changes look fine, aside from the secondaryActions thing mentioned in comment 7.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

I don't think this is useful in a Fennec Native world.