Last Comment Bug 893505 - Simplify the application update UI
: Simplify the application update UI
Status: ASSIGNED
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: 24 Branch
: All All
-- normal with 5 votes (vote)
: ---
Assigned To: Doug Thayer [:dthayer]
:
: Robert Strong [:rstrong] (use needinfo to contact me)
Mentors:
: 347585 752996 952406 986969 1074721 1170436 (view as bug list)
Depends on:
Blocks: 476294 583064 589051 690494 971123 1137763 1182495 1237017
  Show dependency treegraph
 
Reported: 2013-07-13 19:41 PDT by Robert Strong [:rstrong] (use needinfo to contact me)
Modified: 2017-02-12 13:29 PST (History)
33 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
mockup (218.99 KB, image/png)
2013-07-13 19:41 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details
sc.png (80.06 KB, image/png)
2014-10-01 09:27 PDT, Sylvestre Ledru [:sylvestre]
no flags Details
Notifications work in progress (10.12 KB, patch)
2016-12-16 09:12 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Multiple notifications (117.65 KB, image/png)
2016-12-16 16:53 PST, Doug Thayer [:dthayer]
no flags Details
Draft of new update notifications (63.78 KB, patch)
2016-12-21 09:05 PST, Doug Thayer [:dthayer]
no flags Details | Diff | Splinter Review
Latest (63.35 KB, patch)
2016-12-21 15:13 PST, Doug Thayer [:dthayer]
no flags Details | Diff | Splinter Review
Bug 893505 - Simplify the application update UI (59 bytes, text/x-review-board-request)
2016-12-30 10:56 PST, Doug Thayer [:dthayer]
dothayer: review? (enndeakin)
gijskruitbosch+bugs: review+
dothayer: review? (robert.strong.bugs)
Details | Review
Bug 893505 - clean up js and simplify css for PanelUI notifications (59 bytes, text/x-review-board-request)
2017-01-04 12:38 PST, Doug Thayer [:dthayer]
no flags Details | Review

User Story
Primary goals of the redesign:

* warn users who are seriously out of date: if you are 12+ weeks out of date, we want to have semi-persistent warnings that you are running an insecure product
* provide a backstop for update orphaning: if you are 12+ weeks out of date or updates have failed, we want to prompt users to reinstall.
* reduce users noticing updates. In the average case we don't want users to ever notice that Firefox was updated. In the edge cases we want updates to interrupt users as little as possible
* get rid of the big ugly popup dialog.      
Description User image Robert Strong [:rstrong] (use needinfo to contact me) 2013-07-13 19:41:23 PDT
Created attachment 775278 [details]
mockup

It has been several years since we last used the remote billboard and the license pages so these should be removed. Also, the current UI, which can be seen in attachment #430909 [details], is much larger than it needs to be and was made this size to support a large billboard. I'd like to use panels if at all possible (see attached mockup).

This bug does not cover the about dialog. That is bug 617101

Basic requirements as I see them:
* inform users of the add-ons that aren't compatible with the update before downloading an update (this list can be long so it will need to be scrollable).
* inform users that an update is available.
* inform users that they should restart to apply an update (for users that don't restart often).
* inform users that can't apply an update that they should manually install the new version to upgrade via a link.
* inform users when there are repeated errors when trying to update and should manually install the new version to upgrade via a link.

Possible requirements:
* Ability to show UI equivalent to the remote billboard (open a web page?)
* Ability to survey users that don't update (open a web page?)

Note: I haven't included all of the UI in the mockup that is needed (e.g. errors, manual install, etc.)
Comment 1 User image Doug Turner (:dougt) 2014-03-25 20:55:35 PDT
*** Bug 986969 has been marked as a duplicate of this bug. ***
Comment 2 User image Brad Lassey [:blassey] (use needinfo?) 2014-09-30 13:23:47 PDT
*** Bug 1074721 has been marked as a duplicate of this bug. ***
Comment 3 User image Sylvestre Ledru [:sylvestre] 2014-10-01 09:27:31 PDT
Created attachment 8498253 [details]
sc.png

Maybe using the e10s windows could facilitate the work on this feature.
The current update window could need a refresh.
Comment 4 User image Robert Strong [:rstrong] (use needinfo to contact me) 2014-12-12 10:07:34 PST
It's unlikely that I will get to work on this in the next two weeks, so unassigning myself for now.
Comment 5 User image Gingerbread Man 2015-04-10 02:10:07 PDT
*** Bug 952406 has been marked as a duplicate of this bug. ***
Comment 6 User image Sylvestre Ledru [:sylvestre] 2016-03-23 14:29:04 PDT
Bram, could you please share your plans? Thanks
Comment 7 User image Bram Pitoyo [:bram] 2016-03-23 20:43:01 PDT
Hi Sylvestre,

My plan is to continue the design work that you and rstrong had done with the update UI. This will involve specifying its behaviour, doing some of the visual design, and maybe creating an interactive prototype of it.

Earlier, a few of us met and made a few decision related to this, that you might like to review and see how it will impact other parts of Firefox. You can find it under the heading “Decisions Needed”:

https://docs.google.com/document/d/1CGPMWaOxtBMDgKkAJCLg7FHn_Z-9wcFjt98wj9frEks/edit
Comment 8 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-20 17:56:43 PDT
*** Bug 752996 has been marked as a duplicate of this bug. ***
Comment 9 User image Bram Pitoyo [:bram] 2016-05-29 22:14:51 PDT
The latest user flow can be found here:
https://www.lucidchart.com/documents/view/0be491ba-ae0f-4c3f-adba-28d81c1d9356

It accounts for all of these conditions:

1. On update check disabled
2. On update check failure (and multiple failures)
3. On download failure
4. If user hasn’t restarted after x period of time
5 On user type ≠ administrator (I think we are working on this problem, so that you can still install an update even though you’re not an admin)
6. On admin password entry failure
7. On update apply failure

Robert is going to provide some feedback on this flow, then we can go ahead and design the notifications and write the messages.

(At the moment, my mockup only accounts for condition #3 and #4: https://invis.io/Y776FIBWS)
Comment 10 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-06-14 09:49:06 PDT
(In reply to Bram Pitoyo [:bram] from comment #9)
> The latest user flow can be found here:
> https://www.lucidchart.com/documents/view/0be491ba-ae0f-4c3f-adba-
> 28d81c1d9356
> 
> It accounts for all of these conditions:
> 
> 1. On update check disabled
> 2. On update check failure (and multiple failures)
> 3. On download failure
> 4. If user hasn’t restarted after x period of time
It might be a good thing to have a visual indicator like the green arrow when we are ready to restart and then after x period of time bring up ui as this states.

> 5 On user type ≠ administrator (I think we are working on this problem, so
> that you can still install an update even though you’re not an admin)
There will always be the case where the user does not have the privileges to install an update. This should be the Download a Fresh copy path.

> 6. On admin password entry failure
> 7. On update apply failure
Note: This notification is displayed after restart.
Comment 11 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-07-13 14:47:53 PDT
*** Bug 1170436 has been marked as a duplicate of this bug. ***
Comment 12 User image Bram Pitoyo [:bram] 2016-07-13 16:58:19 PDT
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)

I’ve updated the logic diagram to account for the situation you’ve mentioned:

1. Green arrow when update is available and when we’re ready to restart

2. There’s no more administrator check within Firefox itself, because we obviously can’t do that. Instead, we’ll ask user to input an admin password when Firefox has already quit, and when escalation is required before applying the update. If password input fails, the update can’t install and Firefox will still start, but user is then invited to Download a Fresh Copy

3. Elaborated the restart condition. I’ve split it into “Quit Firefox”, “Apply update” and “Start Firefox”, so it’s clear exactly when each notification is displayed. The “Download a Fresh Copy” notification is displayed after restart.

The latest user flow can always be found on comment 9, with the URL:
https://www.lucidchart.com/documents/view/0be491ba-ae0f-4c3f-adba-28d81c1d9356
Comment 13 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-31 13:25:55 PDT
Hi Bram, I finished some app update code removal that should make this easier to implement and I'm going to work on this as time permits.
Comment 14 User image Sylvestre Ledru [:sylvestre] 2016-09-08 00:42:38 PDT
Do we want a link to the release notes from the update windows?
Anyway, any progress on this would be great!
Comment 15 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-09-08 00:45:18 PDT
This will be a very simple user interface and the one in the about window might be a better place for it.
Comment 16 User image Neil Deakin 2016-12-16 09:12:33 PST
Created attachment 8819330 [details] [diff] [review]
Notifications work in progress

This is some code which shows the three new notifications. Some dummy buttons are added to browser.xul to trigger them.

Work that needs to happen:

1. The icons need to be added.
2. Buttons and links hooked up to actions
3. PopupNotifications.jsm assumes that all notifications will hang out of the icon on the url field, so this will need to be adjusted to support the 'PanelUI-button'
Comment 17 User image Doug Thayer [:dthayer] 2016-12-16 16:53:33 PST
Created attachment 8819471 [details]
Multiple notifications

(In reply to Neil Deakin from comment #16)
> Created attachment 8819330 [details] [diff] [review]
> Notifications work in progress
> 
> This is some code which shows the three new notifications. Some dummy
> buttons are added to browser.xul to trigger them.
> 
> Work that needs to happen:
> 
> 1. The icons need to be added.
> 2. Buttons and links hooked up to actions
> 3. PopupNotifications.jsm assumes that all notifications will hang out of
> the icon on the url field, so this will need to be adjusted to support the
> 'PanelUI-button'

There seem to be a few little bugs with the `PopupNotifications.jsm` stuff in general. It seems like in the event of multiple notifications coming in, it tries to just display them one-by-one FIFO style, but it doesn't quite succeed. Attaching a screenshot to highlight the issue. You can repro this by going to addons.mozilla.org and trying to install several addons in quick succession, not accepting anything but just letting them download. You'll see them pile up while they're downloading, then collapse down when they're done. At the end of it it behaves like a queue of dialogs.

I'm trying to figure out what the intended behavior is - this effect becomes a little weirder once we start allowing anchoring to the `PanelUI-menu-button`, because if we take the simple path there, it can cause the add-on notification to jump to be anchored there.
Comment 18 User image Bram Pitoyo [:bram] 2016-12-18 23:30:34 PST
In your interest, my invision mocks were updated last week and can be seen here:
http://mozilla.invisionapp.com/share/Y776FIBWS
Comment 19 User image Sylvestre Ledru [:sylvestre] 2016-12-19 01:57:47 PST
> In your interest, my invision mocks were updated last week and can be seen
> here.
Clap clap clap, I am a big fan. Much better. Bravo!
Comment 20 User image Doug Thayer [:dthayer] 2016-12-19 10:01:34 PST
I'm wondering if `PopupNotifications.jsm` should really be what we're using here. It doesn't seem like too big of a lift to make a different notifications service for the hamburger dropdown, and I think that might simplify things overall. A lot of the complexity of `PopupNotifications.jsm` seems to come from ideas like scoping notifications to a particular tab, and keeping a queue of notifications around, which we probably don't want to do. Additionally, this would let us keep it in a different <panel> element, which would let us change the properties around if we need to, and it's going to make it easier to change set the alignment to "bottomcenter topright", which we'll need in order to have the notifications display well in the top right of the window. Thoughts?

Regarding the UI, do we want this popup to autohide when the user clicks somewhere else in the screen, or do we want to require them to click "Not now"?
Comment 21 User image Neil Deakin 2016-12-20 10:13:17 PST
> Additionally, this would let us keep it in a different <panel> element,
> which would let us change the properties around if we need to, and it's
> going to make it easier to change set the alignment to "bottomcenter
> topright", which we'll need in order to have the notifications display well
> in the top right of the window. Thoughts?

Yes, it seems like it should just be a different panel and not use PopupNotifications. Perhaps break out just the pieces from PopupNotifications that are needed.
Comment 22 User image Doug Thayer [:dthayer] 2016-12-21 09:05:00 PST
Created attachment 8820783 [details] [diff] [review]
Draft of new update notifications

Let me know how this looks. I copied over `PopupNotifications.jsm` and stripped out things that didn't make any sense when applied to the hamburger dropdown (is PanelUI the canonical name for this?) I also stripped out telemetry, but we can add back in the telemetry that we want - I'm not super familiar with best practices in that regard. It's still more complex than it needs to be for doing just want we want it to do, but if we ever want to add more notifications hanging from the hamburger dropdown, this should make it simpler.

The tests aren't done yet, I brought over some of the tests for the `PopupNotifications.jsm` stuff, but I wanted to get feedback on if this general approach was good or not before diving too deep.

Two alternatives I can think of to this approach:
- Don't make a `PanelUINotifications.jsm` module - just write the minimum code required to display and dismiss our three notifications. This would make our notifications simpler, but it would make it more complicated down the line in the event that we wanted to add more notifications to the hamburger which could conceivably happen at the same time as our update notifications. Our current solution makes sure that we don't have multiple notifications stacked on top of each other.

- Just extend `PopupNotifications.jsm` with what we need. We'd have to add an option to the notification to specify it's alignment (`bottomcenter topright` in our case), and we'd have to add an option allowing us to specify that we want this notification to persist across tab changes, and we would have to grab elements by id outside of just the icon box. Ultimately it wouldn't be too crazy, but I think that these notifications will in general have different kinds of requirements than notifications that use the icon box, so it makes sense to split them out early.
Comment 23 User image Doug Thayer [:dthayer] 2016-12-21 15:13:08 PST
Created attachment 8820925 [details] [diff] [review]
Latest

I have a few questions about this:

On the technical side (:enn):

- At what level should I provide hooks to wire into, and what should those hooks look like? Should I add methods into `nsIUpdateService.idl`? Or should I just add observers and get notified (like the old system: `Services.obs.addObserver(this, "update-staged", false);`)?

- Right now the green menu item and the green arrow badge are separate from the doorhanger, and need to be wired up together. Should they instead just all be automatically worked into the new `PanelUINotifications.jsm` logic (assuming we stick with the `PanelUINotifications.jsm` approach)?

On the design side (:bram):

- The "Restyle doorhanger notifications" bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1267604) seems to overlap with this a bit, in that we're using those designs for the doorhangers for this. However, they don't _quite_ match up with the design you linked at https://mozilla.invisionapp.com/share/Y776FIBWS#/screens/155743946. For instance, the border on the top of the buttons is just grey, whereas in your design it's a dark blue above the blue button. I like your design better than what's actually in Nightly right now, because since the grey line is lighter than the blue CTA, but darker than the default button, it makes it look like the CTA is one pixel shorter than the default button. Should I update the styles for doorhangers to match your designs?

- Another way the design doesn't match up is that the top label ("Firefox can't update to the latest version"), isn't any larger than the bottom description ("Download a fresh copy..."). Again, if we were to align this with your design, we'd probably want to align all doorhangers. Should I also make the change to do that?

- Should we display the green arrow _before_ the doorhanger (maybe after a few days of not restarting rather than a whole week)? I wasn't totally sure of this based on the design. It might be nice because they have the opportunity to voluntarily engage with it and not have to be disrupted by the doorhanger when they didn't realize they even needed to restart, but it also could be worse because the ideal scenario is they never had to notice anything at all.
Comment 24 User image Neil Deakin 2016-12-23 10:21:09 PST
> - At what level should I provide hooks to wire into, and what should those
> hooks look like? Should I add methods into `nsIUpdateService.idl`? Or should
> I just add observers and get notified (like the old system:
> `Services.obs.addObserver(this, "update-staged", false);`)?

I think that would be a question for Rob Strong instead.

> - Right now the green menu item and the green arrow badge are separate from
> the doorhanger, and need to be wired up together. Should they instead just
> all be automatically worked into the new `PanelUINotifications.jsm` logic
> (assuming we stick with the `PanelUINotifications.jsm` approach)?

I think they should be specified separately in the browser's css, similarly to the ones for the existing notifications (those are in browser/themes/shared/notification-icons.inc.css). Either a default or no image should probably be used by default.

The image for the green menu item should go where the existing code for the update menuitem is.

As a tip, make sure to check the 'Need more information from' field if you want a question answered as bugzilla will track these for the people you're asking.
Comment 25 User image Doug Thayer [:dthayer] 2016-12-23 13:00:42 PST
(In reply to Neil Deakin from comment #24)
> > - At what level should I provide hooks to wire into, and what should those
> > hooks look like? Should I add methods into `nsIUpdateService.idl`? Or should
> > I just add observers and get notified (like the old system:
> > `Services.obs.addObserver(this, "update-staged", false);`)?
> 
> I think that would be a question for Rob Strong instead.

Ah, okay then. Adding him.

> As a tip, make sure to check the 'Need more information from' field if you
> want a question answered as bugzilla will track these for the people you're
> asking.

Thank you! Still learning the ropes. Adding in :rstrong and :bram.
Comment 26 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-12-23 13:06:59 PST
Go with the existing method and I'll evaluate a new method later.
Comment 27 User image Doug Thayer [:dthayer] 2016-12-29 08:45:05 PST
Just wanted to get a bit of clarification, Neil - my plan is to wrap the code for displaying the menu badge and green menu options (not the css or images) in the PanelUINotifications.jsm module, and migrate existing things that use this (firefox account sync is the only one that's not behind a disabled pref) to use the PanelUINotifications.jsm module. I think this will simplify things in the long term, as any questions about the priority of multiple badges and menu items and their interaction when they're both present will be answerable in one central place. It's also just going to make our code read much nicer if we colocate most of it.

Does that seem like the right approach to you?
Comment 28 User image Doug Thayer [:dthayer] 2016-12-30 10:56:03 PST Comment hidden (mozreview-request)
Comment 29 User image Doug Thayer [:dthayer] 2016-12-30 11:05:34 PST Comment hidden (mozreview-request)
Comment 30 User image Doug Thayer [:dthayer] 2016-12-30 11:28:54 PST
I'm just realizing this, sorry it's a bit late, but there's an infinite UI loop in this diagram if downloads consistently fail: https://pageshot.net/osmBeugOW0Mhmh32/www.lucidchart.com. What do we want to do about this? I also don't really understand the user flow in this loop. If the download failed, what reason do we have to believe that the user manually clicking something to trigger the same download is going to work any better? I understand if we were directing them to a manual download page, like in the "Download a fresh copy" flow, but in this case we're just re-triggering what we tried already. It seems like we should automatically retry, and if we fail enough times, then show them the "Download a fresh copy" flow. In this case, we don't need the "Update available" notification at all.

Thoughts, rstrong? I'm also including you on the code review - if we end up scrapping the "Update available" flow, then this should already be wired up correctly, but I'd still appreciate any feedback.

I'm not sure where to add overall comments in reviewboard, so I'll add them here for now:

- There aren't any tests yet - again I'd like to know that this isn't the completely wrong path before investing too much time in the tests. In the future I'll be trending toward a more TDD approach, but with this uncertainty I think it's simpler to test afterwards.

- I brought the menu badge and menu footer mechanisms together with our system, since they are interrelated. I was hoping I could modify the Firefox Accounts stuff to use the system the same way we do, but I realized their menu item was a bit more complicated than ours and didn't gel nicely with our badge/menu item system. Might be a TODO to consolidate that.

- I colocated the notifications stuff with all the regular panelUI logic. This is because we need to be aware of what's going on in the PanelUI (when it shows, hides, etc.) so that we hide our doorhangers and badges when the user opens up the menu. I thought it seemed the cleanest and most discoverable to keep it all in there. There were a few earlier comments in this bug about it, but using PopupNotifications.jsm seemed wrong, since our doorhangers/badges are complicated by things that existing doorhangers don't care about, and existing doorhangers are complicated in ways that our doorhangers/badges aren't.
Comment 31 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-12-30 11:33:37 PST
(In reply to Doug Thayer [:dthayer] from comment #30)
> I'm just realizing this, sorry it's a bit late, but there's an infinite UI
> loop in this diagram if downloads consistently fail:
> https://pageshot.net/osmBeugOW0Mhmh32/www.lucidchart.com. What do we want to
> do about this? I also don't really understand the user flow in this loop. If
> the download failed, what reason do we have to believe that the user
> manually clicking something to trigger the same download is going to work
> any better? I understand if we were directing them to a manual download
> page, like in the "Download a fresh copy" flow, but in this case we're just
> re-triggering what we tried already. It seems like we should automatically
> retry, and if we fail enough times, then show them the "Download a fresh
> copy" flow. In this case, we don't need the "Update available" notification
> at all.
If all mar downloads fail it should be the Download a fresh copy UI. Note: some updates have both a partial and complete mar and others will only have a complete mar.

> Thoughts, rstrong? I'm also including you on the code review - if we end up
> scrapping the "Update available" flow, then this should already be wired up
> correctly, but I'd still appreciate any feedback.
Sounds good
Comment 32 User image Doug Thayer [:dthayer] 2016-12-30 13:22:50 PST Comment hidden (mozreview-request)
Comment 33 User image Neil Deakin 2017-01-03 06:36:23 PST
I can take a look at reviewing some of this, but Gijs for the panelUI changes and Dao for style changes would be better reviewers for those portions.

I assume you mean to remove the debugging buttons from browser.xul
Comment 34 User image Doug Thayer [:dthayer] 2017-01-03 09:02:00 PST Comment hidden (mozreview-request)
Comment 35 User image Doug Thayer [:dthayer] 2017-01-03 09:07:02 PST Comment hidden (mozreview-request)
Comment 36 User image Doug Thayer [:dthayer] 2017-01-03 09:18:48 PST
Thanks Neil! (and yes, I was planning on removing the debugging buttons before landing this anywhere - just removed them in the latest review though anyway.)

Adding Gijs first per your suggestion.
Comment 37 User image Doug Thayer [:dthayer] 2017-01-03 11:28:51 PST Comment hidden (mozreview-request)
Comment 38 User image :Gijs 2017-01-04 05:27:37 PST
Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review102470

I haven't tested this patch - these are just code comments. I'll try to remember to look in more detail in the next round of reviews.

This generally looks pretty good already, so please don't be put off by the comments. :-)

Note though that the current badging system is preffed off except on nightly/devedition. Do we want to use it everywhere? And if so do we want to remove / turn off the dialog stuff?

::: browser/base/content/browser.js:2667
(Diff revision 6)
> -      this.displayBadge(false);
> +      let openManualUpdateUrl = this.openManualUpdateUrl;
> +      let action = {
> +        callback: () => { openManualUpdateUrl(); }
> +      };

Instead of pulling this method out into a local variable, just make the callback function call `gMenuButtonUpdateBadge.openManualUpdateUrl();` directly? That also fixes `this` in the method (which we don't use right now, but it's always an annoying gotcha if you try and use it later).

::: browser/base/content/browser.js:2689
(Diff revision 6)
> -    let stringId;
> -    let updateButtonText;
> -    if (succeeded) {
> -      let brandBundle = document.getElementById("bundle_brand");
> +    let requestRestart = this.requestRestart;
> +    let action = {
> +      callback: () => { requestRestart(); }
> +    };

Same here.

::: browser/base/content/test/general/browser_panelUINotifications.js:1
(Diff revision 6)
> +add_task(function*() {

Nit: Please name this function and add a short docstring comment indicating what this test does.


Also, please add this in the customizableui dir instead of in browser/.../general/ [0]. CUI isn't the perfect place, but it's moderately topical considering these tests mostly revolve around the innards of panelUI.js, which lives there.

[0] I ask because that dir is a dumping ground and a haven for intermittents that are impossible to reproduce or fix because of the large number of tests that get run in a single dir.

::: browser/base/content/test/general/browser_panelUINotifications.js:2
(Diff revision 6)
> +add_task(function*() {
> +  yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:blank"},

I know it's not obvious from the doc, but this works if you just pass a string for a URL, instead of a full options object. :-)

::: browser/base/content/test/general/browser_panelUINotifications.js:10
(Diff revision 6)
> +
> +      let mainActionCalled = false;
> +      let mainAction = {
> +        callback: () => { mainActionCalled = true; }
> +      };
> +      PanelUI.showNotification("update-manual", mainAction, [], {});

Showing popups is async on non-Linux platforms, I think (and so I'm surprised this test passes as-is). That said, I guess if it works reliably on infra, I'll take it, esp. because I've also seen waiting for popups to show cause intermittents. :-\

::: browser/base/content/test/general/browser_panelUINotifications.js:68
(Diff revision 6)
> +      menuItem.click();
> +      ok(mainActionCalled, "Main action callback was called")
> +
> +      is(PanelUI.menuButton.getAttribute("badge-status"), "fxa-needs-authentication", "Fxa badge is shown on PanelUI button.");
> +    });
> +});

It would be nice if there were also tests for what happens if multiple (non-badge) notifications are added.

::: browser/components/customizableui/content/panelUI.inc.xul:422
(Diff revision 6)
> +       noautofocus="true"
> +       noautohide="true"

Pretty sure we don't want noautohide here? Or was that intentional?

::: browser/components/customizableui/content/panelUI.js:173
(Diff revision 6)
>          console.error("Error showing the PanelUI menu", reason);
>        });
>      });
>    },
>  
> +  showNotification: function(id, mainAction, secondaryActions, options) {

Can you default the options to `{}` and the `secondaryActions` to `[]` here, to avoid having to pass empty stuff from the callers? :-)

Also, here and below please use ES6 method syntax:

```js
showNotification(id, mainAction, secondaryActions = [], options = {})
```

::: browser/components/customizableui/content/panelUI.js:185
(Diff revision 6)
> +  showBadgeOnlyNotification: function(id) {
> +    return this.showNotification(id, null, null, { badgeOnly: true });
> +  },

Right now this means we can't show a badge without closing any open notifications, right? Is that intentional?

::: browser/components/customizableui/content/panelUI.js:191
(Diff revision 6)
> +    return this.showNotification(id, null, null, { badgeOnly: true });
> +  },
> +
> +  removeNotification: function(id) {
> +    let notifications;
> +    if (typeof(id) == "string") {

Nit: typeof is an operator, so you don't need braces.

::: browser/components/customizableui/content/panelUI.js:440
(Diff revision 6)
>  
> -      let iconAnchor =
> +      let anchor = this._getPanelAnchor(aAnchor);
> -        document.getAnonymousElementByAttribute(aAnchor, "class",
> -                                                "toolbarbutton-icon");
>  
> -      if (iconAnchor && aAnchor.id) {
> +      if ((aAnchor != anchor) && aAnchor.id) {

This doesn't need extra braces around the inner condition.

::: browser/components/customizableui/content/panelUI.js:720
(Diff revision 6)
> +    }
> +
> +    let dismiss = true;
> +    if (action) {
> +      try {
> +        action.callback.call(undefined);

Just action.callback() should do, I think? :-)

::: browser/components/customizableui/content/panelUI.js:783
(Diff revision 6)
>    } catch (ex) {
>      return "en-US";
>    }
>  }
> +
> +function Notification(id, mainAction, secondaryActions, options) {

We are rubbish at scoping so this is going to live in the global scope of browser windows, so naming it just `Notification` doesn't seem great. It shadows the builtin window.Notification (DOM) construtor, too. Can we make it more specific?

In more hip news, we do support ES6 class notation, so you could use that here if it helps at all.

::: browser/components/customizableui/content/panelUI.js:786
(Diff revision 6)
> +  this.secondaryActions = secondaryActions || [];
> +  this.options = options || {};

For these, please use defaults in the arguments instead of the `||`.

::: browser/components/customizableui/content/panelUI.js:792
(Diff revision 6)
> +  id: null,
> +  mainAction: null,
> +  secondaryActions: null,
> +  browser: null,
> +  owner: null,
> +  options: null,
> +  timeShown: null,
> +  dismissed: null,

Please remove this. The way prototype-based "inheritance" works, you're never actually changing these, you're always assigning to the instance which just shadows the properties on the prototype, which might as well be undefined (ie not present).

Some of these also seem to be unused?

::: browser/components/customizableui/content/panelUI.js:809
(Diff revision 6)
> +  while (parent && (parent = aElement.ownerDocument.getBindingParent(parent)))
> +    notificationEl = parent;

Nit: Please brace the loop.

::: browser/locales/en-US/chrome/browser/browser.dtd:888
(Diff revision 6)
>  <!ENTITY emeLearnMoreContextMenu.label            "Learn more about DRM…">
>  <!ENTITY emeLearnMoreContextMenu.accesskey        "D">
> +
> +<!ENTITY updateAvailable.message "Update your &brandShorterName; for the latest in speed and privacy.">
> +<!ENTITY updateAvailable.whatsnew.label "See what's new.">
> +<!ENTITY updateAvailable.whatsnew.href "http://www.mozilla.org/">

These links should not be in a locale file. We should probably use the URL formatter to factor in l10n bits, but they'll want to go to release notes or something, rather than hardcoded to mozilla.org (and even in that case, we should definitely be using https).

::: browser/locales/en-US/chrome/browser/browser.properties:636
(Diff revision 6)
>  appmenu.restartBrowserButton.label = Restart %S
> -appmenu.downloadUpdateButton.label = Download Update
> +appmenu.downloadUpdateButton.label = Download %S Update

Both of the button label strings are dead, AFAICT. Please remove them and update the localization note. Archaeology suggests this was a second patch in bug 1080406 that I wanted moved to another bug, which doesn't seem to ever have happened. :-(

::: browser/themes/shared/customizableui/panelUI.inc.css
(Diff revision 6)
>  #PanelUI-menu-button[badge-status] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    display: -moz-box;
>    height: 10px;
>    width: 10px;
>    background-size: contain;
> -  border: none;

I'm surprised to see this border: none disappear. Why are we doing that? Is it set elsewhere already (if so, where?)

::: browser/themes/shared/customizableui/panelUI.inc.css
(Diff revision 6)
>  }
>  
>  #PanelUI-menu-button[badge-status="download-success"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    display: none;
>  }
> -

Nit: please leave this blank line between the rules.

::: browser/themes/shared/customizableui/panelUI.inc.css:111
(Diff revision 6)
>  #PanelUI-menu-button[badge-status="update-succeeded"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    background: #74BF43 url(chrome://browser/skin/update-badge.svg) no-repeat center;
>    height: 13px;
>  }

This can be unified with the rule underneath, right?

::: browser/themes/shared/customizableui/panelUI.inc.css:132
(Diff revision 6)
>  
>  #PanelUI-menu-button[badge-status="download-warning"] > .toolbarbutton-badge-stack > .toolbarbutton-badge,
>  #PanelUI-menu-button[badge-status="fxa-needs-authentication"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    box-shadow: none;
>    filter: drop-shadow(0 1px 0 hsla(206, 50%, 10%, .15));
> +  border-radius: 2px;

Why the border-radius change here?

::: browser/themes/shared/customizableui/panelUI.inc.css:141
(Diff revision 6)
>  #PanelUI-menu-button[badge-status="download-warning"] > .toolbarbutton-badge-stack > .toolbarbutton-badge,
>  #PanelUI-menu-button[badge-status="download-severe"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    width: 7px;
>    height: 7px;
>    min-width: 0;
> -  border-radius: 50%;
> +  min-height: 0;

And this change?

::: browser/themes/shared/customizableui/panelUI.inc.css:449
(Diff revision 6)
>    background-color: var(--arrowpanel-dimmed);
>  }
>  
>  #PanelUI-multiView[viewtype="subview"] #PanelUI-mainView > #PanelUI-contents-scroller > #PanelUI-contents > .panel-wide-item,
>  #PanelUI-multiView[viewtype="subview"] #PanelUI-mainView > #PanelUI-contents-scroller > #PanelUI-contents > .toolbarbutton-1:not([panel-multiview-anchor="true"]),
> -#PanelUI-multiView[viewtype="subview"] #PanelUI-mainView > #PanelUI-footer > #PanelUI-update-status,
> +#PanelUI-multiView[viewtype="subview"] #PanelUI-mainView > #PanelUI-footer > #PanelUI-update-available-menu-item,

Here and in a number of other cases, the id selectors will just multiply happily, and we'd have to remember all the places where we need to do that if we add another notification with a menuitem.

Can we instead make the object that creates these menu-item elements add a class, and select for the class? That will reduce churn, simplify the rules, and provide warm fuzzies etc. :-)

Note that this will lower specificity. For the rule where I'm commenting, that likely won't matter because it's through the roof already (heck, given the descendant selector, why are we bothering selecting for mainView and footer - those elements never move!), but for other rules, you *may* (please check!) need to add a single child selector with the ID of the #PanelUI-footer to ensure you override other rules. In such cases, please add a comment.

::: browser/themes/shared/notification-icons.inc.css:43
(Diff revision 6)
>  %endif
>    }
>  }
>  
> -#notification-popup > .panel-arrowcontainer > .panel-arrowcontent {
> +#notification-popup > .panel-arrowcontainer > .panel-arrowcontent,
> +#PanelUI-notification-popup > .panel-arrowcontainer > .panel-arrowcontent {

This is sad (shouldn't really be in this file), but I don't see a better way - duplicating everything will lead to divergence, and I don't know of a better file to keep this in, off-hand.

::: browser/themes/shared/notification-icons.inc.css:349
(Diff revision 6)
> +.popup-notification-icon[popupid="update-available"],
> +.popup-notification-icon[popupid="update-manual"],
> +.popup-notification-icon[popupid="update-restart"] {
> +  background: #74BF43 url(chrome://browser/skin/notification-icons.svg#update) no-repeat center;
> +  border-radius: 50%;
> +}

I don't think this is used?

::: browser/themes/shared/notification-icons.svg:49
(Diff revision 6)
> +    #update-icon {
> +      stroke: #fff;
> +      stroke-width: 3px;
> +      stroke-linecap: round;
> +    }

I don't think we use this icon anywhere? These icons are for the location bar, and I don't think we'll have an icon there, so I think the change to this file can be reverted?

::: toolkit/themes/osx/global/toolbarbutton.css:84
(Diff revision 6)
>  .toolbarbutton-badge {
>    background-color: #d90000;
>    font-size: 9px;
>    padding: 1px 2px;
>    color: #fff;
> -  border-radius: 2px;
> +  border-radius: 50%;

I'm not going to review this file (and its x-platform friends) in detail, I hope Neil or Dão will if we keep it this way. I'll just say that this affects add-ons as well, so please test carefully. I suspect just cropping all the icons (because of the border-radius) add-ons provide is likely not OK. See bug 1188001 for some of the history. If we're only intending to modify the style of the badges we control, this might want to be living in more specific rules in browser/ CSS files instead.
Comment 39 User image Romain Testard [:RT] 2017-01-04 07:36:33 PST
> Regarding the UI, do we want this popup to autohide when the user clicks
> somewhere else in the screen, or do we want to require them to click "Not
> now"?

I think we want to persist the panel until the user clicks one of the buttons given how easy it is to dismiss it without noticing (I recall the same issue existed on WebRTC prompts). Bram what do you think? Again we should probably align practices with existing door hangers? (bug 1267604)

Also regarding "Download a fresh copy of Firefox" through the panel or the hamburger menu - the UX shows that the green arrow goes away as the user clicks and reaches the download page. This implies that if the user does not download or install Firefox the notification is not displayed anymore - can we change that so the arrow persists until the user installs a fresh copy?
Comment 40 User image Doug Thayer [:dthayer] 2017-01-04 12:38:13 PST Comment hidden (mozreview-request)
Comment 41 User image Doug Thayer [:dthayer] 2017-01-04 12:50:28 PST Comment hidden (mozreview-request)
Comment 42 User image Doug Thayer [:dthayer] 2017-01-04 13:14:51 PST Comment hidden (mozreview-request)
Comment 43 User image Doug Thayer [:dthayer] 2017-01-04 13:29:54 PST Comment hidden (mozreview-request)
Comment 44 User image Doug Thayer [:dthayer] 2017-01-04 13:35:36 PST
> I haven't tested this patch - these are just code comments. I'll try to
> remember to look in more detail in the next round of reviews.
> 
> This generally looks pretty good already, so please don't be put off by the
> comments. :-)

Of course! Just happy to be getting my first review :)

> Note though that the current badging system is preffed off except on
> nightly/devedition. Do we want to use it everywhere? And if so do we want to
> remove / turn off the dialog stuff?

Yeah that was the intent. Details of that are still to be sorted out.

> Showing popups is async on non-Linux platforms, I think (and so I'm
> surprised this test passes as-is). That said, I guess if it works reliably
> on infra, I'll take it, esp. because I've also seen waiting for popups to
> show cause intermittents. :-\

Yeah - I think it works because it's just testing for the state "closed", and if the popup is in the process of showing it will have a state of "showing".

> ::: browser/components/customizableui/content/panelUI.inc.xul:422
> (Diff revision 6)
> > +       noautofocus="true"
> > +       noautohide="true"
> 
> Pretty sure we don't want noautohide here? Or was that intentional?

This is intentional - I can make this an option in the API for each notification rather than a universal property of the panel, but we want to make sure in our use case (per Romain's comment above) that we don't automatically hide the doorhanger when a user clicks outside of it.

> Also, here and below please use ES6 method syntax:

I was debating whether I should use ES6 and update the whole file to that syntax, or just do it for my own work, or stay consistent with the rest of the file. My new review updates my own work, but should I apply that change to the rest of the file as well?

> ::: browser/locales/en-US/chrome/browser/browser.dtd:888
> (Diff revision 6)
> >  <!ENTITY emeLearnMoreContextMenu.label            "Learn more about DRM…">
> >  <!ENTITY emeLearnMoreContextMenu.accesskey        "D">
> > +
> > +<!ENTITY updateAvailable.message "Update your &brandShorterName; for the latest in speed and privacy.">
> > +<!ENTITY updateAvailable.whatsnew.label "See what's new.">
> > +<!ENTITY updateAvailable.whatsnew.href "http://www.mozilla.org/">
> 
> These links should not be in a locale file. We should probably use the URL
> formatter to factor in l10n bits, but they'll want to go to release notes or
> something, rather than hardcoded to mozilla.org (and even in that case, we
> should definitely be using https).

Updated to use the same mechanism that the current about page uses (a preprocessor directive). Does that seem reasonable?
Comment 45 User image Doug Thayer [:dthayer] 2017-01-04 16:13:20 PST
@rstrong: How do we want to approach disabling the old update mechanism and enabling the new one? Do you want me to dive into that or is this awaiting work from you? I'm happy to dive into it, but I'm unfamiliar with how to simulate updates on my machine in order to produce that workflow from start to finish.
Comment 46 User image :Gijs 2017-01-05 06:43:28 PST
Comment on attachment 8823803 [details]
Bug 893505 - clean up js and simplify css for PanelUI notifications

https://reviewboard.mozilla.org/r/102302/#review102896

I see I got confused about the icons, thanks for clarifying. The comment can probably be removed, I was just wrong, we do use those icons. Sorry!

This covers most of my comments. We'll need to sort out the URL, let me know (here or on IRC) if you need more pointers - https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog-appUpdater.js#53-56 is another example in the updater code that uses formatURLPref. We might even be able to reuse some of the existing URL prefs for this - Rob would know, I suspect.

Unlike github (I know, inconsistencies... they're annoying) on mozreview we generally fold/amend and repush the amended cset so we get multiple versions of the same commit, rather than "fixup" commits that are rolled together before landing. If you keep the weird moz commit id string in the commit description[0] of the commit you're changing (the first of the two commits on this review now), mozreview knows you're updating. Unlike with git, you won't need --force to repush or anything. Mozreview will re-request review on the commit. See https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#changing-code-after-reviews for a description. So, please fold/"roll" these two commits (and any further changes) when you next push. Thanks!

[0] and I think even if you don't keep it, it can do the math in most cases, but there are edgecases with history rewriting where it'll lose track.

::: browser/base/content/browser.css:1147
(Diff revision 4)
>    -moz-box-flex: 1;
>    width: 10em;
>    max-width: 10em;
>  }
>  
> -#main-window[customizing=true] #PanelUI-update-available-menu-item,
> +#main-window[customizing=true] #PanelUI-notification-menu-item {

I guess this should be a class selector?

::: browser/components/customizableui/test/browser_panelUINotifications.js:1
(Diff revision 4)
> -add_task(function*() {
> -  yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:blank"},
> -    function*(browser) {
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Hm, why did you add this? :-)

No license indicates it's licensed public domain, which is our default ( https://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/ ).

::: browser/components/customizableui/test/browser_panelUINotifications.js:254
(Diff revision 4)
> +    secondaryActionButton.click();
> +
> +    is(PanelUI.notificationPanel.state, "closed", "update-manual doorhanger is closed.");
> +    is(PanelUI.menuButton.getAttribute("badge-status"), "update-restart", "update-restart badge is displaying on PanelUI button.");
> +
> +    let menuItem

Nit: missing semicolon

::: browser/components/customizableui/content/panelUI.inc.xul:439
(Diff revision 4)
> +#ifndef NIGHTLY_BUILD
>        <label class="text-link" value="&updateAvailable.whatsnew.label;"
> -             href="&updateAvailable.whatsnew.href;"/>
> +#expand      href="https://www.mozilla.org/firefox/__MOZ_APP_VERSION__/releasenotes/"/>
> +#endif

Unfortunately, no, I don't think we should do this. We should add code that sets the .href of the label at runtime, probably based on a pref and formatURLPref. I realize that's going to be messy because of the XBL involved, which is unfortunate, but I think we may want the ability to configure this for e.g. partner builds or funnelcakes.

::: browser/components/customizableui/content/panelUI.js:452
(Diff revision 4)
>        CustomizableUI.addPanelCloseListeners(tempPanel);
>        tempPanel.addEventListener("popuphidden", panelRemover);
>  
>        let anchor = this._getPanelAnchor(aAnchor);
>  
> -      if ((aAnchor != anchor) && aAnchor.id) {
> +      if ((aAnchor != anchor) && aAnchor.id)

Sorry, I think I was unclear. I meant that operator precedence means that your if condition can be written as:

```js
if (aAnchor != anchor && aAnchor.id) {
```

(without the inner () )

and I don't think that's unclear. If you think it's easier to read, you could move the aAnchor.id check first (aAnchor.id && aAnchor != anchor) which would evaluate the same independent of operator precedence anyway. :-)

::: browser/components/customizableui/content/panelUI.js:679
(Diff revision 4)
>      let menuItemId = this._getMenuItemId(notification);
>      let menuItem = document.getElementById(menuItemId);
>      if (menuItem) {
>        menuItem.notification = notification;
>        menuItem.setAttribute("oncommand", "PanelUI._onNotificationMenuItemSelected(event)");
> +      menuItem.className += " PanelUI-notification-menu-item";

Yay for modern browsers!

```js
menuItem.classList.add("PanelUI-notification-menu-item");
```

:-)

::: browser/themes/shared/customizableui/panelUI.inc.css:101
(Diff revision 4)
>  
>  #nav-bar[brighttext] > #PanelUI-button {
>    border-image-source: linear-gradient(transparent, rgba(100%,100%,100%,.2) 20%, rgba(100%,100%,100%,.2) 80%, transparent);
>  }
>  
>  #PanelUI-menu-button[badge-status] > .toolbarbutton-badge-stack > .toolbarbutton-badge {

This block is still missing the border: none styling that was there earlier.

::: browser/themes/shared/customizableui/panelUI.inc.css
(Diff revision 4)
>  }
>  
>  #PanelUI-menu-button[badge-status="download-success"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    display: none;
>  }
> -#PanelUI-menu-button[badge-status="update-succeeded"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {

Shouldn't this selector be appended to the rule underneath it, or did this just go away entirely?

::: browser/themes/shared/customizableui/panelUI.inc.css:119
(Diff revision 4)
> -#PanelUI-menu-button[badge-status="update-failed"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> -  background: #D90000 url(chrome://browser/skin/update-badge-failed.svg) no-repeat center;
> +  margin: -9px 0 0 !important;
> +  margin-inline-end: -6px !important;

!important is generally frowned upon... can you clarify why it's necessary here, in a comment?

::: browser/themes/shared/customizableui/panelUI.inc.css:135
(Diff revision 4)
>  
>  #PanelUI-menu-button[badge-status="download-warning"] > .toolbarbutton-badge-stack > .toolbarbutton-badge,
>  #PanelUI-menu-button[badge-status="download-severe"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    width: 7px;
>    height: 7px;
>    min-width: 0;

This is still missing the border-radius: 50% that was removed in the previous cset, I think?
Comment 47 User image Doug Thayer [:dthayer] 2017-01-05 10:59:38 PST
Pushing updates now. I also made one change that wasn't requested in your reviews: I pulled out some of the update-specific css to use the id selectors again. Pretty much just the icons, since future notifications are unlikely to be using the same icons as us. Other than that it should all be expected stuff.


> This covers most of my comments. We'll need to sort out the URL, let me know
> (here or on IRC) if you need more pointers -
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/
> aboutDialog-appUpdater.js#53-56 is another example in the updater code that
> uses formatURLPref. We might even be able to reuse some of the existing URL
> prefs for this - Rob would know, I suspect.

Sounds good. There's already an app.releaseNotesURL pref which is used in mobile, so I just added it to desktop. For the Nightly release notes, we didn't have a design and :bram is on PTO for a while(?), so I just went with the nightly blog, because it seems to act as informal release notes. The other option would be to just not display the link, but I like having the consistency.
 
> Unlike github (I know, inconsistencies... they're annoying) on mozreview we
> generally fold/amend and repush the amended cset so we get multiple versions
> of the same commit, rather than "fixup" commits that are rolled together
> before landing.

Woops. That's my bad for reading that workflow doc too quickly, because I completely misread that section. Sorry about that.

> Unfortunately, no, I don't think we should do this. We should add code that
> sets the .href of the label at runtime, probably based on a pref and
> formatURLPref. I realize that's going to be messy because of the XBL
> involved, which is unfortunate, but I think we may want the ability to
> configure this for e.g. partner builds or funnelcakes.

I went with a solution specific to our use case, which was fairly simple and didn't require any XBL fiddling. Hopefully that seems reasonable? I'm not sure a more general solution would be used by anything else.

> Sorry, I think I was unclear. I meant that operator precedence means that
> your if condition can be written as:
> 
> ```js
> if (aAnchor != anchor && aAnchor.id) {
> ```
> 
> (without the inner () )
> 
> and I don't think that's unclear. If you think it's easier to read, you
> could move the aAnchor.id check first (aAnchor.id && aAnchor != anchor)
> which would evaluate the same independent of operator precedence anyway. :-)

Ah, yeah I'm fine with that. I don't mind relying on operator precedence in clearer cases like this. Just happy to be learning the conventions :)

> ```js
> menuItem.classList.add("PanelUI-notification-menu-item");
> ```
> 
> :-)

Oh gosh. Sane ways of doing things. I'll get there eventually, I promise.

> ::: browser/themes/shared/customizableui/panelUI.inc.css
> (Diff revision 4)
> >  }
> >  
> >  #PanelUI-menu-button[badge-status="download-success"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> >    display: none;
> >  }
> > -#PanelUI-menu-button[badge-status="update-succeeded"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> 
> Shouldn't this selector be appended to the rule underneath it, or did this
> just go away entirely?

Sorry, I forgot to mention that these badges are dead with this change. They were only used by the gMenuButtonUpdateBadge, which I've rewritten to use the new badges/notifications.
Comment 48 User image Doug Thayer [:dthayer] 2017-01-05 11:00:41 PST Comment hidden (mozreview-request)
Comment 49 User image Doug Thayer [:dthayer] 2017-01-05 11:15:20 PST Comment hidden (mozreview-request)
Comment 50 User image :Gijs 2017-01-06 13:45:42 PST
Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review103556

With the below addressed, this looks pretty much OK to me. Probably a good idea for Rob and/or Neil to have a look, too, though.

One more thing besides the code things below: please update the commit msg of the single commit to only have the description of what the total commit does, and its funky mozreview commit id. So just:

```
Bug 893505 - Simplify the application update UI

MozReview-Commit-ID: 24sESMTosNX
```

would do.

Thanks for working on this and all the work that went into the test!

::: browser/base/content/browser.js:2672
(Diff revisions 6 - 8)
>        let action = {
> -        callback: () => { openManualUpdateUrl(); }
> +        callback: () => { gMenuButtonUpdateBadge.openManualUpdateUrl(); }
>        };
>  
> -      PanelUI.showNotification("update-manual", action, [], {});
> +      let releaseNotesURL = Services.urlFormatter.formatURLPref("app.releaseNotesURL");
> +      document.getElementById("update-manual-whats-new").href = releaseNotesURL;

This only affects the manual one, not the "update-available" one. Is that intentional?

::: browser/app/profile/firefox.js:82
(Diff revision 8)
>  pref("browser.dictionaries.download.url", "https://addons.mozilla.org/%LOCALE%/firefox/dictionaries/");
>  
> +// When we display a doorhanger to update firefox, there will be a "What's new"
> +// link which will point to this URL.
> +#ifdef RELEASE_OR_BETA
> +pref("app.releaseNotesURL", "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/releasenotes/");

This will make this link appear on the page that shows up if you load "about:" in the browser. Probably not a bad thing, but just an FYI (we don't expose a link to "about:" anywhere, I think).

::: browser/app/profile/firefox.js:84
(Diff revision 8)
> +// When we display a doorhanger to update firefox, there will be a "What's new"
> +// link which will point to this URL.
> +#ifdef RELEASE_OR_BETA
> +pref("app.releaseNotesURL", "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/releasenotes/");
> +#else
> +#ifdef MOZ_DEV_EDITION

Use NIGHTLY_BUILD (maybe with ifndef, or swap the if/else), because MOZ_DEV_EDITION is a branding thing rather than a channel thing. It's confusing, I know! Basically, aurora used to just be an alpha branch, and then we started marketing it as devedition...
Comment 51 User image Doug Thayer [:dthayer] 2017-01-09 08:29:32 PST
> Thanks for working on this and all the work that went into the test!

Of course. Thank you for your time on this, Gijs! :) I really appreciate it.

> ::: browser/base/content/browser.js:2672
> (Diff revisions 6 - 8)
> >        let action = {
> > -        callback: () => { openManualUpdateUrl(); }
> > +        callback: () => { gMenuButtonUpdateBadge.openManualUpdateUrl(); }
> >        };
> >  
> > -      PanelUI.showNotification("update-manual", action, [], {});
> > +      let releaseNotesURL = Services.urlFormatter.formatURLPref("app.releaseNotesURL");
> > +      document.getElementById("update-manual-whats-new").href = releaseNotesURL;
> 
> This only affects the manual one, not the "update-available" one. Is that
> intentional?

It is. There's still no place where we display the update-available notification. I had a design question about that one up above, since it's not altogether clear to me how that notification fits into the whole picture. If there's a clear place where it should be triggered then we'll do the same thing with it, but I was waiting on rstrong for that one.

> This will make this link appear on the page that shows up if you load
> "about:" in the browser. Probably not a bad thing, but just an FYI (we don't
> expose a link to "about:" anywhere, I think).

Yeah I saw that guy - I couldn't find any links to it so I just left it, since it seemed like it made sense to have the release notes there anyway.

> Use NIGHTLY_BUILD (maybe with ifndef, or swap the if/else), because
> MOZ_DEV_EDITION is a branding thing rather than a channel thing. It's
> confusing, I know! Basically, aurora used to just be an alpha branch, and
> then we started marketing it as devedition...

Ah, weird. Alright, I'll make sure to get that in! Thanks so much again!
Comment 52 User image Doug Thayer [:dthayer] 2017-01-09 09:08:21 PST
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #31)
> (In reply to Doug Thayer [:dthayer] from comment #30)
> > I'm just realizing this, sorry it's a bit late, but there's an infinite UI
> > loop in this diagram if downloads consistently fail:
> > https://pageshot.net/osmBeugOW0Mhmh32/www.lucidchart.com. What do we want to
> > do about this? I also don't really understand the user flow in this loop. If
> > the download failed, what reason do we have to believe that the user
> > manually clicking something to trigger the same download is going to work
> > any better? I understand if we were directing them to a manual download
> > page, like in the "Download a fresh copy" flow, but in this case we're just
> > re-triggering what we tried already. It seems like we should automatically
> > retry, and if we fail enough times, then show them the "Download a fresh
> > copy" flow. In this case, we don't need the "Update available" notification
> > at all.
> If all mar downloads fail it should be the Download a fresh copy UI. Note:
> some updates have both a partial and complete mar and others will only have
> a complete mar.

I'm still not certain when we want to show the Update Firefox tour from this. If all mar downloads fail, then we go to Download a Fresh Copy, but if only some of them fail, do we want to show anything at all? It seems like we shouldn't prompt the user for anything until we've exhausted all options, and so I don't see where the Update Firefox tour fits in.
Comment 53 User image Bram Pitoyo [:bram] 2017-01-11 16:47:03 PST
Answering Doug’s questions:

> The "Restyle doorhanger notifications" bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1267604) seems to overlap with this a bit, in that we're using those designs for the doorhangers for this. However, they don't _quite_ match up with the design you linked at https://mozilla.invisionapp.com/share/Y776FIBWS#/screens/155743946.

> Another way the design doesn't match up is that the top label ("Firefox can't update to the latest version"), isn't any larger than the bottom description ("Download a fresh copy..."). Again, if we were to align this with your design, we'd probably want to align all doorhangers. Should I also make the change to do that?

We should try to match and reuse the restyled doorhanger as much as we can instead of designing/tweaking our own. If my styles don’t match, let’s change them to fit. It means more UI consistency across Firefox.


> Should we display the green arrow _before_ the doorhanger (maybe after a few days of not restarting rather than a whole week)? I wasn't totally sure of this based on the design. It might be nice because they have the opportunity to voluntarily engage with it and not have to be disrupted by the doorhanger when they didn't realize they even needed to restart, but it also could be worse because the ideal scenario is they never had to notice anything at all.

The reason why I’ve put the green arrow so long after not restarting was because we know that most users close their browsers every day (so the update will be installed the next day). In other words, most people will never see this green arrow, which is great, because the arrow is supposed to only be shown when something important needs your attention. That is, if we show a notification too early or too many times, we may cause attention fatigue and the user may not pay as much attention to other life-threatening notifications (like password inputted on HTTP sites).


Answering Romain’s questions:

> I think we want to persist the panel until the user clicks one of the buttons given how easy it is to dismiss it without noticing (I recall the same issue existed on WebRTC prompts). Bram what do you think? Again we should probably align practices with existing door hangers? (bug 1267604)

Yes. The new doorhanger dialogues was designed to persist until clicked and not so easily dismissed like before. We should follow this behaviour.


> Also regarding "Download a fresh copy of Firefox" through the panel or the hamburger menu - the UX shows that the green arrow goes away as the user clicks and reaches the download page. This implies that if the user does not download or install Firefox the notification is not displayed anymore - can we change that so the arrow persists until the user installs a fresh copy?

We can do this, but I think the message should be tweaked a little:

IF this is the first time you see the dialogue, THEN print “Download a fresh copy of Firefox”.

IF you have clicked the “Download fresh copy” button, THEN keep the green arrow showing, THEN when you open the hamburger menu, print something like “Install the Firefox that you have just downloaded, or click here to re-download a fresh copy of Firefox”.


I think we should rewrite the second message because it’s too wordy and a bit confusing, but the idea is there. What do you think?
Comment 54 User image Doug Thayer [:dthayer] 2017-01-11 23:05:11 PST
> The reason why I’ve put the green arrow so long after not restarting was
> because we know that most users close their browsers every day (so the
> update will be installed the next day). In other words, most people will
> never see this green arrow, which is great, because the arrow is supposed to
> only be shown when something important needs your attention. That is, if we
> show a notification too early or too many times, we may cause attention
> fatigue and the user may not pay as much attention to other life-threatening
> notifications (like password inputted on HTTP sites).

I totally understand that - I wasn't so much trying to say that we should show the arrow *earlier*, just that it might be a good idea to show it *before* the doorhanger, since it's less intrusive than the doorhanger. This way they have an opportunity to interact with the notification before it pops into their screen space demanding immediate attention. For instance, we could show the green arrow at 7 days, and then pop up the doorhanger on day 8.

> > Also regarding "Download a fresh copy of Firefox" through the panel or the hamburger menu - the UX shows that the green arrow goes away as the user clicks and reaches the download page. This implies that if the user does not download or install Firefox the notification is not displayed anymore - can we change that so the arrow persists until the user installs a fresh copy?
> 
> We can do this, but I think the message should be tweaked a little:
> 
> IF this is the first time you see the dialogue, THEN print “Download a fresh
> copy of Firefox”.
> 
> IF you have clicked the “Download fresh copy” button, THEN keep the green
> arrow showing, THEN when you open the hamburger menu, print something like
> “Install the Firefox that you have just downloaded, or click here to
> re-download a fresh copy of Firefox”.
> 
> 
> I think we should rewrite the second message because it’s too wordy and a
> bit confusing, but the idea is there. What do you think?

In theory we should be able to look at their downloads and see that the fresh copy is still fresh and is valid. If that's the case, giving them the a button to keep downloading fresh copies of Firefox ad infinitum seems like we're ignoring that the user doesn't know what to do with the thing they just downloaded. It might be better to just give them a button which simply runs the installer that they downloaded from the previous step. Thoughts?
Comment 55 User image Bram Pitoyo [:bram] 2017-01-12 22:24:37 PST
(In reply to Doug Thayer [:dthayer] from comment #54)
> […] it might be a good idea to show it [the green arrow] *before* the doorhanger, since it's less intrusive […]
> For instance, we could show the green arrow at 7 days, and then pop up the doorhanger on day 8.

Showing something less intrusive before popping something in your face is an interesting idea. It would be the equivalent of an unread message label on your home screen icon.

Here’s a proposal: if we know that most of our users restarts Firefox every day, why not show the green arrow at day 2 instead of day 7? I don’t have a good sense of exactly when to show the arrow, but in principle, I want most users not to see or have to worry about it. So this may help determine our day counter.


> In theory we should be able to look at their downloads and see that the fresh copy is still fresh and is valid.

I designed the flow to not be so smart or involve things like file detection. That way, it’s easy to implement, test and refine. If it’s easy to do this, I would love for us to be able to say “Hey, you have downloaded Firefox successfully. Click here to run the installer!”.
Comment 56 User image Robert Strong [:rstrong] (use needinfo to contact me) 2017-01-12 22:35:21 PST
I suspect that showing the green arrow earlier wouldn't hurt but I don't think it will help very much either since most users restart much sooner. I am fairly confident this is a very small number of users but I'm not against it. If this is added it should be based off of the prompt to restart delay since that can be controlled from the server so if there is ever a major issue we can ask users to restart prior to the 7 day default.

One thing that might be nice is to only show the popup when there has been some user interaction. That way it wouldn't show up on kiosks, etc around offices unless someone interacted with the system and this person could then perform the restart.
Comment 57 User image Robert Strong [:rstrong] (use needinfo to contact me) 2017-01-13 01:41:25 PST
*** Bug 347585 has been marked as a duplicate of this bug. ***
Comment 58 User image Robert Strong [:rstrong] (use needinfo to contact me) 2017-01-13 02:02:23 PST
(In reply to Doug Thayer [:dthayer] from comment #45)
> @rstrong: How do we want to approach disabling the old update mechanism and
> enabling the new one? Do you want me to dive into that or is this awaiting
> work from you? I'm happy to dive into it, but I'm unfamiliar with how to
> simulate updates on my machine in order to produce that workflow from start
> to finish.
Doug, this is difficult to do manually and I planned on doing that part. If you have time then perhaps the existing tests that simulate updates can help guide you?
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/chrome
Comment 59 User image Doug Thayer [:dthayer] 2017-01-18 08:38:20 PST Comment hidden (mozreview-request)
Comment 60 User image Doug Thayer [:dthayer] 2017-01-18 09:16:38 PST Comment hidden (mozreview-request)
Comment 61 User image Doug Thayer [:dthayer] 2017-01-18 09:29:37 PST
I just pushed a change which does a few things:

- Heuristically detects if they've downloaded the installer after being prompted to do so, and raises a different message in that case.

- Only shows popups if there is no full screen element (like a video).

Watching downloads for things that look like our installer wasn't too difficult, but the strategy isn't bullet proof. Basically I just looked for the first download that came from the same domain that we directed them toward (minus the subdomain. I.e., if we send them to https://nightly.mozilla.org, and they get redirected to https://www.mozilla.org/en-US/firefox/channel/desktop/?v=b#nightly, then we'll still survive, because it will just match on the "mozilla.org" part. Unfortunately I'm not sure of a better way, but advice is welcome. We can of course scrap this strategy entirely, but I think in 99% of cases it will work just fine.

Additionally, after experimenting with the workflow of downloading a fresh copy for a little bit I landed on the following:

- Direct them to the fresh installer page. On nightly, that is https://nightly.mozilla.org
- Immediately remove the badge once they've been directed there, since it felt confusing that the badge was still there after I interacted with the popup.
- If the user hasn't downloaded the installer within a minute, then bring the badge back, because the user isn't done, and likely got sidetracked
- Once the installer is downloading, show a badge which will give them an option to launch the installer (or launch it once it's done downloading)
- If the user has not launched the installer within five minutes, escalate the badge into a popup.

Does that all seem reasonable?

Another point regarding the "Download a fresh copy" workflow, is that I'm realizing the "See what's new" link will be a bit difficult, since I'm not sure we have enough information to direct them to what's new. The release notes URL requires a version number of the version we're upgrading to, which we may not have depending on where things broke down. I assume the best course of action is to just only display that link if we know what version we're directing them to download.

Regarding full-screen mode, I realized that this popup would be annoying if it showed up while I was watching a video, so I just opted to not display these popups if there's no full-screen element. I was debating whether to hide the popup if we're in full-screen mode at all (i.e., via F11). I don't know what the most common use of full-screen mode is, but I know people use it for things like self-sign-in kiosks, where for instance customers might come and sign themselves into a class or something. If we had the ability to not show the doorhanger in these cases, that would be great, since those customers would not want to upgrade Firefox for the business they're visiting, but I'm not sure how much we care about that or if it's worth it. Just a thought.

Lastly, Gijs mentioned after his r+ that it would still be a good idea to get a review from you or neil, rstrong, so I just thought I'd poke you about this. After my most recent changes there are a few TODO's left over, which are noted in the code, but I think it's close to being in a good state to land. In any case, once it is, and after I run the front-end tests in try, what's the process for getting it landed? Does the reviewer take care of that?
Comment 62 User image Doug Thayer [:dthayer] 2017-01-18 11:43:53 PST
One last thing:

> Doug, this is difficult to do manually and I planned on doing that part.
> If you have time then perhaps the existing tests that simulate updates
> can help guide you?
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/chrome

I'm looking into it now because I don't have much else on my plate. If there are things I can't overcome or too many questions I can't answer myself I'll let you know, but from what I can see there should be enough for me to figure it out.
Comment 63 User image Bram Pitoyo [:bram] 2017-01-18 17:13:40 PST
So far, our strategy has been to open a Firefox download page in a new tab and have the user click the download button. This is simple for us to do, but risky for the user:

* User might accidentally close the tab
* User might get distracted and forgot what he originally came there to do, navigating away from the tab
* User might not be able to find the download button (this is probably an artificial risk, as we’ve designed the page to have prominent buttons)
* User might not click “Save” or “OK” on the download prompt

Now that we’re experimenting with a few behaviours, I wonder if it would be possible to directly trigger the installer download (even better if the user won’t have to click “Save” or “OK”) so there would be very little chance of failure?
Comment 64 User image Doug Thayer [:dthayer] 2017-01-18 20:21:31 PST
(In reply to Bram Pitoyo [:bram] from comment #63)
> Now that we’re experimenting with a few behaviours, I wonder if it would be
> possible to directly trigger the installer download (even better if the user
> won’t have to click “Save” or “OK”) so there would be very little chance of
> failure?

I like the idea, but I have a few concerns:

* If the reason we're in the "Download a fresh copy" workflow is because update checks failed repeatedly, then I think the chances are high that automatically downloading a fresh copy is going to fail as well. One benefit of opening a download page in a new tab is we get to piggy back on existing error handling and troubleshooting documentation to help people figure out why the download is failing. Because of this, if we did automatically download the fresh copy and run it, we'd probably just end up failing and falling back to opening up a new tab anyway.

* We'd have to make it clear that we're going to launch an application for the user, and the user will have to initiate that, because (speculating) it might be disconcerting to the user to have an executable run without their direct consent.

* Automatically downloading the installer might require work in different places than you'd expect. For instance, if you navigate to https://nightly.mozilla.org, the "Download" button links to https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/firefox-53.0a1.en-US.win32.installer-stub.exe. Now, we can easily initiate a download and launch the resulting executable if we know what that URL is beforehand, but that would involve knowing the version that we are trying to download, which isn't necessarily available to us if the update checks failed, since it could be a major version bump or a minor version bump, and if we can't contact the server then we wouldn't know.

* If there are little hiccups like a user's antivirus not allowing the user to run the exe, then following the typical workflow that a user is used to might help them troubleshoot. Once we've hit this point, it's likely that something unexpected is going on on the user's machine, and the more we do automatically, the less the user understands, and the less likely it is that they'll be able to help us out.


These aren't show stoppers, but I wanted to make sure to surface them. Sorry for starting us down this rabbit hole, I know this might be overthinking something that we're not expecting to hit very frequently at all anyway.
Comment 65 User image Bram Pitoyo [:bram] 2017-01-18 20:52:42 PST
(In reply to Doug Thayer [:dthayer] from comment #64)
> (In reply to Bram Pitoyo [:bram] from comment #63)
> […] One benefit
> of opening a download page in a new tab is we get to piggy back on existing
> error handling and troubleshooting documentation to help people figure out
> why the download is failing.
> 
> […] it
> might be disconcerting to the user to have an executable run without their
> direct consent.
> 
> […] the more we do
> automatically, the less the user understands, and the less likely it is that
> they'll be able to help us out.

All these reasons make sense, and I agree that the positives (giving users a chance to troubleshoot when all automatic processes had failed) outweigh the negatives (users need to manually find the download button, click it, then click “Save”).

Let’s go with your initial proposal, then: open download page on a new tab, then focus on that tab. If user hasn’t downloaded the file or run the executable, we’ll escalate the doorhanger message into something stronger.
Comment 66 User image :Gijs 2017-01-19 05:34:34 PST
(In reply to Doug Thayer [:dthayer] from comment #61)
> I just pushed a change which does a few things:

Do I need to review this again?


> - Heuristically detects if they've downloaded the installer after being
> prompted to do so, and raises a different message in that case.
> 
> - Only shows popups if there is no full screen element (like a video).
> 
> Watching downloads for things that look like our installer wasn't too
> difficult, but the strategy isn't bullet proof. Basically I just looked for
> the first download that came from the same domain that we directed them
> toward (minus the subdomain. I.e., if we send them to
> https://nightly.mozilla.org, and they get redirected to
> https://www.mozilla.org/en-US/firefox/channel/desktop/?v=b#nightly, then
> we'll still survive, because it will just match on the "mozilla.org" part.
> Unfortunately I'm not sure of a better way, but advice is welcome. We can of
> course scrap this strategy entirely, but I think in 99% of cases it will
> work just fine.

It looks like you're doing some manual string splitting. We have the eTLD service to do this correctly for funny things like foo.co.uk and the like.

> <snip>

> Regarding full-screen mode, I realized that this popup would be annoying if
> it showed up while I was watching a video, so I just opted to not display
> these popups if there's no full-screen element. I was debating whether to
> hide the popup if we're in full-screen mode at all (i.e., via F11). I don't
> know what the most common use of full-screen mode is, but I know people use
> it for things like self-sign-in kiosks, where for instance customers might
> come and sign themselves into a class or something. If we had the ability to
> not show the doorhanger in these cases, that would be great, since those
> customers would not want to upgrade Firefox for the business they're
> visiting, but I'm not sure how much we care about that or if it's worth it.
> Just a thought.

I think this is a good idea. There are issues with what happens to popups generally if the anchor disappears (off-screen or otherwise), so I don't think it would work well when the toolbars are hidden (which we don't do on OS X but you should be able to repro on Windows or Linux). I forget how we deal with this for the URL-bar based notifications; :florian or :past might know.
Comment 67 User image Doug Thayer [:dthayer] 2017-01-19 09:01:55 PST
(In reply to :Gijs from comment #66)
> I think this is a good idea. There are issues with what happens to popups
> generally if the anchor disappears (off-screen or otherwise), so I don't
> think it would work well when the toolbars are hidden (which we don't do on
> OS X but you should be able to repro on Windows or Linux). I forget how we
> deal with this for the URL-bar based notifications; :florian or :past might
> know.

I tested it on Windows and it seems to work as you would hope: it's anchored to the top of the screen and sits where it normally would horizontally. I'm still on the fence about disabling it - since while self-sign-in kiosks are troubling, another use case for full-screen mode would be a kiosk operated by an employee. I imagine these kinds of kiosks are prime examples of Firefox instances which never get restarted and thus never receive updates, which I think would mean we want to make sure we reach them. The badges of course won't work - we could however hover a badge in the top right of the screen while in full-screen mode if we wanted to. Just a thought.

Regarding the eTLD service, thank you! I was looking for something like that but was having difficulty figuring out what to search for.
Comment 68 User image Doug Thayer [:dthayer] 2017-01-19 10:37:59 PST Comment hidden (mozreview-request)
Comment 69 User image Doug Thayer [:dthayer] 2017-01-19 10:46:58 PST
Talked about this with Gijs on IRC. If you enter full-screen mode while a doorhanger is showing, it doesn't behave very well. You can observe the behavior by going to https://addons.mozilla.org, downloading an add-on, and then hitting F11. Though the bug is present with add-ons, I think it's simpler to just side-step it in our case and not show the notification.

I pushed these changes to reviewboard, along with using the eTLD service instead of manual string splitting.

There's still more to come though, since I'm working through the process of tying this to the back-end.
Comment 70 User image Doug Thayer [:dthayer] 2017-01-24 13:10:37 PST Comment hidden (mozreview-request)
Comment 71 User image Doug Thayer [:dthayer] 2017-01-24 13:31:01 PST
Just pushed the patch with the back-end wired up. I decided to write new tests for this adapted from the old tests, rather than just modifying the old tests for a few reasons:

- The old tests were testing in some cases substantially different workflows in which they, for instance, have a download progress page they're waiting for.
- These changes currently only targeted at Firefox. I don't know how much would be required to get it to work with Thunderbird, etc. because it's fairly tied up with, for instance, panelUI.js, which is under browser. Accordingly, preserving the old application update code for now, and thus the corresponding tests, seems like the simplest solution.
- I wanted to get my hands dirty with some of the testing and update code, and the best way to do that seemed to be to rewrite some of the framework and see what the tests really required.

I'm open to reverting this decision though, I just wanted to run with it for now.

Additionally, it was a bit tricky deciding exactly where to add events off from nsUpdateService.js, and where it should exit early if we're using the new UI, so I would appreciate any feedback dealing with that.

rstrong: I believe this is ready for you to fully review, so when you have some time I would greatly appreciate it.

Gijs: If you have the time and think it's warranted I also would appreciate further review from you, since it was tremendously helpful last time :)
Comment 72 User image Bram Pitoyo [:bram] 2017-01-24 15:26:55 PST
(In reply to Doug Thayer [:dthayer] from comment #71)

Additionally, if you’ve got any screenshot of the process that you’re able to share, I’d love to see them and do some UI review!
Comment 73 User image Johann Hofmann [:johannh] 2017-01-25 02:47:16 PST
For screenshots please consider creating a mozscreenshots[0] configuration for this. Makes cross platform UI review much easier and allows us to catch visual regressions that would otherwise go unnoticed.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots

(You can do that in a separate bug)
Comment 74 User image Doug Thayer [:dthayer] 2017-01-25 11:31:58 PST
(In reply to Johann Hofmann [:johannh] from comment #73)
> For screenshots please consider creating a mozscreenshots[0] configuration
> for this. Makes cross platform UI review much easier and allows us to catch
> visual regressions that would otherwise go unnoticed.
> 
> [0] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots
> 
> (You can do that in a separate bug)

Ah okay - yeah I'll be sure to file a bug and do that. For now while it's in development I assume manual screenshots are okay?

Assuming so, then bram, here are those screenshots:

Badge - https://cl.ly/1g2D0v1j1B38

Available doorhanger - https://cl.ly/0F2q1z2l251I
Available menu item - https://cl.ly/0p2c37442X1s

Restart doorhanger - https://cl.ly/093N33042X07
Restart menu item - https://cl.ly/400z452R0520

Download doorhanger - https://cl.ly/1d3n2G0l0j0i
Download menu item - https://cl.ly/04001d2A2r0V

Launch doorhanger (probably needs better text) - https://cl.ly/0d1C033J3O3F
Launch menu item (probably needs better text) - https://cl.ly/2S44221m3U47

I can also grab Windows screenshots if you like. They're not significantly different other than Windows having sharp corners and the blue button being on the left.

Additionally, just calling it out that these are without any custom styles for our doorhangers, as mentioned above. Without custom styles the whitespace around the doorhanger icon isn't balanced, there's a little bit too much space below the text, and the header is the same size as the description.
Comment 75 User image Doug Thayer [:dthayer] 2017-01-25 11:34:48 PST Comment hidden (mozreview-request)
Comment 76 User image :Gijs 2017-01-27 11:02:43 PST
(In reply to Doug Thayer [:dthayer] from comment #71)
> rstrong: I believe this is ready for you to fully review, so when you have
> some time I would greatly appreciate it.
> 
> Gijs: If you have the time and think it's warranted I also would appreciate
> further review from you, since it was tremendously helpful last time :)

I think for this bit, rstrong's review is likely to be more helpful. I'll wait for him to review this first.
Comment 77 User image Doug Thayer [:dthayer] 2017-01-27 13:42:18 PST
(In reply to :Gijs from comment #76)
> I think for this bit, rstrong's review is likely to be more helpful. I'll
> wait for him to review this first.

Sounds good!

@bram, I forgot, I never heard back on an earlier question I had - I didn't really explicitly address it to you though, so I think it just got overlooked. There's an infinite loop in the UI if downloads are failing. It seems to me that if all of our downloads fail (the partial download and the complete download), we should simply prompt them to download a fresh copy of the installer, for the reasons mentioned above about troubleshooting/etc. This is currently what's in my patch, but we could also do something similar to how update checks work (i.e.: only prompt the user after 10 failed downloads.) I'm also open to keeping the current design, I just didn't quite understand the history of it and whether that infinite UI loop was an intended property.

I'm still using the "Update Available" workflow in cases where Balrog's response has showPrompt="true" set (this path isn't in the user flow diagram - do we actively use this property for real updates?), but other than that it would go away.

Anyway, I'm just working on doing what I can to get everything all set for QA (https://wiki.mozilla.org/QA/Application_Update_UI), and just wanted to make sure everything matched up with your docs and that they are up-to-date :)
Comment 78 User image Doug Thayer [:dthayer] 2017-01-27 13:59:59 PST
Also @rstrong, a few questions about the doc @ (https://wiki.mozilla.org/QA/Application_Update_UI):

Risk Analysis:

- To mitigate the risks of this, I think a good strategy is to make sure that we can revert to the old system by just flipping a pref; does that sound reasonable? I believe the code should already be set up such that this is true, but if you agree with me then I can add automated tests to validate this.

- The doc mentions "We currently have a problem where incompatible addons can disable updates". Do you know where the bug is for this, and how it relates to this work? This is the first I'm hearing of it.


Scope:

- We talked about how this stuff is pretty tricky to manually test, so I just wanted to get your take on how QA should be manually testing this, since that's mentioned in the doc? Should they just be installing older versions of Nightly and tweaking their prefs to make the timings easier for manual validation?

- Specifically in regards to testing how the system reacts to various failures, what is the scope of the failures they should be testing? There are some easier things for them to test, like how the system behaves when you're without internet, or don't have sufficient permissions, etc., but what about more exotic failures like malformed XML from Balrog, or a failed signature validation? Are those out of scope?


There's an email thread
Comment 79 User image Bram Pitoyo [:bram] 2017-01-29 21:18:00 PST
(In reply to Doug Thayer [:dthayer] from comment #77)
> […] There's an infinite loop in the UI if downloads are failing. It
> seems to me that if all of our downloads fail (the partial download and the
> complete download), we should simply prompt them to download a fresh copy of
> the installer […] but we could also do something similar
> to how update checks work (i.e.: only prompt the user after 10 failed
> downloads.)

Sorry I’ve missed answering this question from over a month ago!

Thanks for pointing out the infinite loop in the flow.

My original thought: as long as your internet connection is active, re-downloading the update would most certainly fix any issue you have with downloading the MAR file and lead to success.

But you pointed out a good concern: if the download failed repeatedly for any reason, the user would have to keep clicking “Update Firefox” over and over again without any resolution.

So I’m updating the flow as follows:

* The first and second time you download an update, you’ll go through the standard flow
* The third time the update failed to download, show the “Download a fresh copy” dialogue

I thought that one click is worth retrying, but two clicks are enough to escalate it to the next level.

What do you think?
Comment 80 User image Bram Pitoyo [:bram] 2017-01-29 22:00:49 PST
(In reply to Doug Thayer [:dthayer] from comment #74)

> Badge - https://cl.ly/1g2D0v1j1B38

Looks good.

> Available doorhanger - https://cl.ly/0F2q1z2l251I
> Restart doorhanger - https://cl.ly/093N33042X07
> Download doorhanger - https://cl.ly/1d3n2G0l0j0i
>
> Additionally, just calling it out that these are without any custom styles
> for our doorhangers […]

Looks good. The text sizes I had used for the heading and description didn’t conform to the standard doorhanger size, so it’s good that we follow that standard here.

At this point, I’m more worried about the behaviour rather than the style, but thanks a lot for bringing up the style issue. At some point we’ll have to choose whether this notification should look closer to other notifications or to the main Control Center UI. But for now, let’s stick with our current approach.


> Available menu item - https://cl.ly/0p2c37442X1s
> Restart menu item - https://cl.ly/400z452R0520
> Download menu item - https://cl.ly/04001d2A2r0V

Looks good.


> Launch doorhanger (probably needs better text) - https://cl.ly/0d1C033J3O3F

Heading
Firefox is ready to install.

Description
Restart Firefox to install it. When it’s finished, Firefox will relaunch and restore all your open tabs and windows.

[Not Now] [Install and Restore]


> Launch menu item (probably needs better text) - https://cl.ly/2S44221m3U47

Description
Restart and install Firefox


One thing to add: in both these launch scenarios, I assume that we can launch the .exe or .msi on Windows, and open the .dmg file on Mac. Can we confirm this?

I’m not entirely clear what will happen on Linux. Maybe we don’t need to tackle this one, because the Package Manager will take care of the update?
Comment 81 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2017-01-30 01:22:33 PST
(In reply to Doug Thayer [:dthayer] from comment #78)
> - We talked about how this stuff is pretty tricky to manually test, so I
> just wanted to get your take on how QA should be manually testing this,
> since that's mentioned in the doc? Should they just be installing older
> versions of Nightly and tweaking their prefs to make the timings easier for
> manual validation?

Updates are usually not manually tested by QA but our firefox-ui-update tests are used. Those you can run via "./mach firefox-ui-update --binary %path/to/old/firefox%", to let an update test from an older release to a current one. It also offers a couple of options you can see with --help. 

It means when the changes on this bug become active we will also have to update those tests, so that they can step through the new UI correctly, and automatically fallback to the old UI for former Firefox releases.
Comment 82 User image Doug Thayer [:dthayer] 2017-01-30 14:18:32 PST
https://www.mozilla.org/en-US/firefox/all/(In reply to Bram Pitoyo [:bram] from comment #79)
> So I’m updating the flow as follows:
> 
> * The first and second time you download an update, you’ll go through the
> standard flow
> * The third time the update failed to download, show the “Download a fresh
> copy” dialogue
> 
> I thought that one click is worth retrying, but two clicks are enough to
> escalate it to the next level.
> 
> What do you think?

Sounds good! Do we want to have any layers of automatic retrying of the downloads though? Seems like we might want to at least give that a shot after some delay before prompting the user.

(In reply to Bram Pitoyo [:bram] from comment #80)
> > Launch doorhanger (probably needs better text) - https://cl.ly/0d1C033J3O3F
> 
> Heading
> Firefox is ready to install.
> 
> Description
> Restart Firefox to install it. When it’s finished, Firefox will relaunch and
> restore all your open tabs and windows.
> 
> [Not Now] [Install and Restore]
> 
> 
> > Launch menu item (probably needs better text) - https://cl.ly/2S44221m3U47
> 
> Description
> Restart and install Firefox

I'm not sure it's going to be so simple to deliver on this text. On OSX, we can launch the .dmg, but we can't restart when they're done or do anything fancy like that. In fact, they can't install via the .dmg until we've shut Firefox down, so the best we can do is launch the .dmg and then immediately shut Firefox down, and just wait for them to launch the new install. I think we can restore their session in OSX, but it might take a bit of fiddling to do so automatically (if we were to trigger a restart, we'd get it for free, but I'm not 100% sure how much work it would take to automatically restore their session via a close/manual open.)

On Windows, we have a couple of options. If we use the stub installer that you'd find at https://www.mozilla.org/en-US/firefox/new/, then once they're all the way through the installer, they'll see a message that reads like this:


> Firefox is already running.
> 
> Please close Firefox prior to launching the version you have just installed.
> 
> [OK]

Our options for this are:

- Just accept that this message is there and the user has to click OK, and then restart Firefox once the installer closes, which will by default preserve their session.
- Sidestep this issue by directing them to download the full installer (located here: https://www.mozilla.org/en-US/firefox/all/) instead of the stub installer. This page might be too complicated for a regular user to understand and navigate.
- Close Firefox right after launching the installer - this way, they won't see the "Firefox is already running" message, but if they exit out of the installer early or anything, we won't automatically launch Firefox for them. This also has the same problem as the OSX side where I'll need to figure out what it will take the restore the session.


> One thing to add: in both these launch scenarios, I assume that we can
> launch the .exe or .msi on Windows, and open the .dmg file on Mac. Can we
> confirm this?
> 
> I’m not entirely clear what will happen on Linux. Maybe we don’t need to
> tackle this one, because the Package Manager will take care of the update?

And yes, with the caveats I mention above, we can launch the .exe and we can open the .dmg file. Linux is a good question - I haven't fully explored this on any Linux distros.
Comment 83 User image Doug Thayer [:dthayer] 2017-01-30 15:05:41 PST
Update: saving the session to be restored without requiring a restart turned out to be fairly simple, so disregard my previous comments about it potentially requiring work.

Should be as simple as:

```
      let didSave = Cc["@mozilla.org/supports-PRBool;1"]
                    .createInstance(Ci.nsISupportsPRBool);
      Services.obs.notifyObservers(didSave, "session-save", null);

      if (!didSave.data) {
        Cu.reportError("Tried to save session before restart for update and failed.");
      }

      // ... now we can quit the application

```
Comment 84 User image Bram Pitoyo [:bram] 2017-01-30 15:56:02 PST
(In reply to Doug Thayer [:dthayer] from comment #82)
> […] Do we want to have any layers of automatic retrying of the
> downloads though? Seems like we might want to at least give that a shot
> after some delay before prompting the user.

Is this method similar to automatically retrying to fetch the update file (early in the flow)? If yes, then I think it’s worth trying.

The way it might work could look something like this:

* Prompt for manual entry
* When “Download Firefox” is clicked, download file
* If download failed, retry automatically a few times
* If all automatic retry attempts failed, go back to start of flow


> […] the best we can do is launch the .dmg and then immediately
> shut Firefox down, and just wait for them to launch the new install. I think
> we can restore their session in OSX, but it might take a bit of fiddling to
> do so automatically.

This sounds a good compromise. Most Mac users – including those who hadn’t updated their apps in a long time – know where their apps are located and how to launch them. And when they launch, their session would be restored.

I recall clicking History > Restore Previous Session after quitting Firefox manually and then reopening it. If we can’t do the restore for them automatically, can we show an instructional page that tells them to click on that menu item, or show about:sessionrestore (which is normally shown after a crash, but we can modify the messaging)?


> On Windows, we have a couple of options.
>
> […]
> 
> - Just accept that this message is there and the user has to click OK […]
> - Sidestep this issue by directing them to download the full installer […]
> - Close Firefox right after launching the installer […]

I prefer going with option #3. If the user drops out of the installer for any reason and then reopen Firefox manually, the same update system can tell them that they still need to install Firefox.

If they click “Install”, we can redo what we just did: close Firefox and launch the installer.
Comment 85 User image Doug Thayer [:dthayer] 2017-01-30 16:03:58 PST
Sounds good! Then do we want to change the text below on OSX?

Restart Firefox to install it. When it’s finished, Firefox will relaunch and restore all your open tabs and windows.

(Since it won't relaunch automatically)
Comment 86 User image Bram Pitoyo [:bram] 2017-01-30 16:22:36 PST
Absolutely. Based on this new behaviour, the text on Windows should look like this:

Heading
Firefox is ready to install.

Description
Firefox will quit and a new window called Firefox Setup will open. Click the “Install” button. When it’s finished, Firefox will relaunch and restore all your open tabs and windows.

[Not Now] [Install Firefox]


And on Mac, it should look like this:

Heading
Firefox is ready to install.

Description
Firefox will quit and a new Finder window containing the Firefox application will pop open. Drag the Firefox icon on top of the Applications folder in order to copy it there. Finally, start Firefox by clicking on its icon on your dock. Firefox will launch and restore all your open tabs and windows.

[Not Now] [Install Firefox]


How does that sound?
Comment 87 User image Doug Thayer [:dthayer] 2017-01-31 11:04:49 PST Comment hidden (mozreview-request)
Comment 88 User image Doug Thayer [:dthayer] 2017-01-31 11:05:35 PST
Sounds great - just pushed a revision with all of these changes :)
Comment 89 User image Robert Strong [:rstrong] (use needinfo to contact me) 2017-02-09 23:08:19 PST
Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review112750

Sorry for taking so long on this.

Note: there are several new blank lines in nsUpdateService.js that aren't necessary that should be removed.

After the patch is updated I'll land it on oak for testing.

::: browser/app/profile/firefox.js:92
(Diff revision 14)
> +#ifdef NIGHTLY_BUILD
> +pref("app.releaseNotesURL", "https://blog.nightly.mozilla.org/");
> +#else
> +pref("app.releaseNotesURL", "https://www.mozilla.org/%LOCALE%/firefox/{targetVersion}/auroranotes/");
> +#endif
> +#endif

Instead of a new pref this should use the update's detailsURL and if that doesn't exist then the app.update.details.url preference.

The app.update.details.url preference is located in browser/branding/<channel>/pref/firefox-branding.js file

Both release and beta share the same firefox-branding.js file under browser/branding/official/pref. I could be wrong but I don't think this should be an issue since the version should differentiate it well enough for this.

::: browser/base/content/browser.js:15
(Diff revision 14)
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/ContextualIdentityService.jsm");
>  Cu.import("resource://gre/modules/NotificationDB.jsm");
> +Cu.import("resource://gre/modules/Subprocess.jsm");

I would prefer it if for this patch you just open the web page to download the installer and the new code for launching the installer, etc. was done in a separate bug. This way if there is a problem with launching the installer the new update notification UI won't have to be disabled or backed out.

::: browser/base/content/browser.js:2584
(Diff revision 14)
>  
> -  init() {
> -    PanelUI.panel.addEventListener("popupshowing", this, true);
> +  tryGetPref(type, name, defaultValue) {
> +    try {
> +      return Services.prefs[`get${type}Pref`](name);
> +    } catch (e) {
> +      return defaultValue;

nit: iirc this is a strict error for not always having a return value which can be avoided by returning the default outside of the catch.

::: browser/base/content/browser.js:2853
(Diff revision 14)
> +        break;
> +    }
>    },
>  
> -  displayBadge(succeeded) {
> -    let status = succeeded ? "succeeded" : "failed";
> +  handleUpdateStaged(update, status) {
> +    handleUpdateDownloaded(status);

Shouldn't this be |this.handleUpdateDownloaded(status);| ?

I saw handleUpdateDownloaded as undefined in the console when I manually tested the patch and changing it as above fixed lots of stuff. Since the existing tests didn't catch this can you add a test for this?

::: browser/base/content/test/simpleUpdate/browser.ini:17
(Diff revision 14)
> +[browser_updatesCompleteAndPartialPatchesWithBadCompleteSize.js]
> +[browser_updatesCompleteAndPartialPatchesWithBadPartialSize.js]
> +[browser_updatesCompleteAndPartialPatchesWithBadSizes.js]
> +[browser_updatesCompletePatchApplyFailure.js]
> +[browser_updatesCompletePatchWithBadCompleteSize.js]
> +[browser_updatesDownloadFailures.js]

browser_updatesDownloadFailures.js doesn't exist in this patch

::: browser/base/content/test/simpleUpdate/browser_noUpdates.js:1
(Diff revision 14)
> +add_task(function* testNoUpdates() {

Not certain this is still the case and I just didn't get the memo but we usually add the following license for test files.

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

::: browser/themes/shared/notification-icons.inc.css:261
(Diff revision 14)
>  }
>  
>  #notification-popup-box[hidden] {
>    /* Override display:none to make the pluginBlockedNotification animation work
>       when showing the notification repeatedly. */
> -  display: -moz-box;
> +  display: -moz-box;j

Typo?

::: browser/themes/shared/notification-icons.inc.css:331
(Diff revision 14)
> +.popup-notification-icon[popupid="update-manual"],
> +.popup-notification-icon[popupid="update-launch"],
> +.popup-notification-icon[popupid="update-restart"] {
> +  background: #74BF43 url(chrome://browser/skin/notification-icons.svg#update) no-repeat center;
> +  border-radius: 50%;
> +}

nit: please add a newline.
Comment 90 User image Doug Thayer [:dthayer] 2017-02-10 12:25:16 PST Comment hidden (mozreview-request)
Comment 91 User image Doug Thayer [:dthayer] 2017-02-10 12:27:19 PST
Pushed a new changeset.

Regarding the license - per Gijs' comment, no license now indicates that it's public domain, so we can omit the license declaration at the top: https://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/
Comment 92 User image Robert Strong [:rstrong] (use needinfo to contact me) 2017-02-10 12:32:25 PST
Doug, this patch doesn't apply off of the latest m-c. Can you update it?
Comment 93 User image Robert Strong [:rstrong] (use needinfo to contact me) 2017-02-10 12:36:56 PST
Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review112958

Patch needs to be updated to latest m-c
Comment 94 User image Doug Thayer [:dthayer] 2017-02-12 13:29:21 PST Comment hidden (mozreview-request)

Note You need to log in before you can comment on or make changes to this bug.