Improve handling of needsRestart hint with mozAddonManager

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨

Other

125 stars 41 forks source link

Improve handling of needsRestart hint with mozAddonManager #10233

Closed ValentinaPC closed 5 years ago

ValentinaPC commented 7 years ago

Steps to reproduce:

Search for a "requires restart" add-on on your device using AMO-dev i.e. https://addons.allizom.org/en-US/android/addon/stylish/?src=hp-dl-featured
Tap the add-on installation button

Expected results: A popup notifying the user that the browser needs to be restarted to apply changes is displayed.

Actual results: 2 popups are displayed: one that requires browser restart and one with "Your add-on is ready" message - as if the add-on was installed.

Notes/Issues:

without the extensions.webapi.testing set to "true", it only the "browser needs to be restarted" popup is displayed.
if the both popups are dismissed the installation button appears to load continuously.

Verified on FF51(Android 6.0.1). Issue is reproducing on AMO-dev and -stage. Video for this issue: videotogif_2017 03 09_12 52 30

muffinresearch commented 7 years ago

Looks like the browser fires onInstallEnded for the promise to be resolved [1]. Presumably if the resolve handed back the installObj it should be possible to get the needsRestart hint and not show the installed notification or handle it in some different way.

There should also be a message to show that a restart is required for desktop and the android case where the restart required notification is dismissed though that will need UX input for the amo context.

It would be important to make sure any changes here don't cause problems in the disco pane.

[1] https://github.com/mozilla/addons-frontend/blob/7f3f48cdf6c8380c65a53c9e45c6ce6ded9ede4e/src/core/addonManager.js#L63

muffinresearch commented 7 years ago

Added the needs: ux label to get input on how a restart required should be displayed. If it's something not too intrusive it could probably work for both mobile and desktop cases.

pwalm commented 7 years ago

How about a little pill/badge under the short description saying "restart required" in yellow (#E7AB00)?

tofumatt commented 7 years ago

I like it.

muffinresearch commented 7 years ago

This also relates to https://github.com/mozilla/addons/issues/10245 which is about removing the post-install overlay for android.

muffinresearch commented 7 years ago

See also mozilla/addons#10313 which related to uninstall for the same thing.

muffinresearch commented 7 years ago

Fixing this requires handling the following:

[ ] Making sure we're receiving the needsRestart hint for both install and uninstall of add-ons that require restarts.
[ ] Responding to needsRestart to show something in the UI for both install and uninstalls of add-ons that require restarts
[ ] Removing the install popup for mobile (covered by mozilla/addons#10245)

kumar303 commented 7 years ago

I lowered the priority on this since add-ons that require a restart are so legacy they are legacy legacy.

kumar303 commented 7 years ago

Also note that the needs restart badge is getting added in https://github.com/mozilla/addons/issues/10560

muffinresearch commented 5 years ago

Closing this as wontfix