1240560 - When adding website to homes screen, no notification of success/failure is presented

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Barbara Bermes [:barbara] - NI please!]

I tried adding a few sites on my Nexus 9 tablet to my home screen.

Clicked, Settings-Page-Add to Home Screen, but after that no notification showed up if it actually added it to the home screen. I would assume a little snackbar or toast to indicate that the site was added to my home screen.

Comment 1

[:Margaret Leibovic]

This sounds like a good progressive apps enhancement.

Comment 2

[Barbara Bermes [:barbara] - NI please!]

I added this as an AC to https://mozilla.aha.io/features/FENN-416

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

For the promotion in bug 1232706 we are considering switching from the app to the home screen when creating a shortcut.

@antlam: I guess we should do the same when creating the shortcut from the menu?

Showing a toast or snackbar might not be suitable because some versions of Android already show a toast if a shortcut is created - see bug 1260143. This would require that we carefully investigate what versions do show the toast and only show our visual feedback on the others.

Comment 4

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)

> Showing a toast or snackbar might not be suitable because some versions of
> Android already show a toast if a shortcut is created - see bug 1260143.
> This would require that we carefully investigate what versions do show the
> toast and only show our visual feedback on the others.

This sounds fragile and difficult to get right.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> For the promotion in bug 1232706 we are considering switching from the app
> to the home screen when creating a shortcut.
> 
> @antlam: I guess we should do the same when creating the shortcut from the
> menu?

yes!

Comment 6

[Nevin Chen(Not active on Bugzilla)]

After a day of investigation , I found that there are couple of reasons why sometimes the toast is shown on some device/OS version combination :

1. Each manufacturer (HTC, Samsung ,Sony...) they all have their own launcher app. And some of them will response to "com.android.launcher.action.INSTALL_SHORTCUT" intent. Take HTC M7 for example , if you trace logact using `adb logcat -b main -b events | egrep -i "toast"` , you'll find that they'll send out toast from their Launcher app (toast_enqueue: [com.htc.launcher,0])

2. Even in the sample Launcher app in AOSP have different implementation. (Launcher, Launcher2, Launcher3 )
e.g. Launcher in AOSP , adding shortcut will toast
https://android.googlesource.com/platform/packages/apps/Launcher/+/master/src/com/android/launcher/InstallShortcutReceiver.java#26
e.g. Launcher2 in 5 & 6, there's only toast for duplicated and no space use case
http://androidxref.com/5.0.0_r2/xref/packages/apps/Launcher2/src/com/android/launcher2/InstallShortcutReceiver.java#174
http://androidxref.com/6.0.0_r5//xref/packages/apps/Launcher2/src/com/android/launcher2/InstallShortcutReceiver.java#256
e.g. Launcher3 in 6.0 there's no toast at all
http://androidxref.com/6.0.0_r5/xref/packages/apps/Launcher3/src/com/android/launcher3/InstallShortcutReceiver.java#144

3. In Chrome, a toast will always be shown when the user create a shortcut (although it will cause duplicated toast for some brand/OS combination)
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java?q=ShortcutHelper.java&sq=package:chromium&dr&l=213


In brief, I suggest not sending any toast since we don't have the control of shortcut creation. Take Chrome on HTC for example, if you don't have enough space on mobile desktop, although you see a "shortcut was added to your Home screen" toast, the HTC device will toast and tell you "No more room for your Home screens". That will confuse the user(Please see the screen shots in the attachment)

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Attachment: Screenshot_20161122-172145.png

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Attachment: Screenshot_20161122-172155.png

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, I agree, a toast might not be a good idea if some home launchers already show a toast.

In the "Add to home screen" prompt experiment we just send the user to the home screen and then the launcher icon usually just appears (or a toast in the case of an error) - What do you think, might this be an option there too? Would be interesting to know whether this works with the different launchers.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/promotion/HomeScreenPrompt.java#131-142

Comment 10

[Nevin Chen(Not active on Bugzilla)]

That's a good idea. I'll test your approach on different devices tomorrow

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

I guess adding the flag was an accident or did you have a question? :)

Comment 12

[Nevin Chen(Not active on Bugzilla)]

(In reply to Sebastian Kaspari (:sebastian) from comment #11)
> I guess adding the flag was an accident or did you have a question? :)

Sorry. Just an accident. :P

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1240560 - Extract goToHomeScreen() method to AcitvityUtils

Review commit: https://reviewboard.mozilla.org/r/95238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95238/

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1240560 - After shortcut is created, show the mobile desktop.

Review commit: https://reviewboard.mozilla.org/r/95240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95240/

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8813937 [details]
Bug 1240560 - Extract goToHomeScreen() method to AcitvityUtils

https://reviewboard.mozilla.org/r/95238/#review95486

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/ActivityUtils.java:70
(Diff revision 1)
> +
> +        activity.finish();

I think the call to finish() should not be part of the helper method:

* When showing the prompt we want to "close" it as soon as the launcher icon has been created.
* For BrowserApp we just want to switch to the home screen but do not need to finish the activity.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8813938 [details]
Bug 1240560 - After shortcut is created, show the mobile desktop.

https://reviewboard.mozilla.org/r/95240/#review95488

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8813938 [details]
Bug 1240560 - After shortcut is created, show the mobile desktop.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95240/diff/1-2/

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8813937 [details]
Bug 1240560 - Extract goToHomeScreen() method to AcitvityUtils

https://reviewboard.mozilla.org/r/95238/#review95498

Okay, this makes sense now. But looking at the individual commits is a bit confusing. The first one moves the finish call to the utility method and the second one removes it again. Would have been more clear to update the first commit. However I r+ already. :)

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Thank you for the reminder. I'll be more careful in the future.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df6b2f629500
Extract goToHomeScreen() method to AcitvityUtils r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c08d0aa688dc
After shortcut is created, show the mobile desktop. r=sebastian

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df6b2f629500
https://hg.mozilla.org/mozilla-central/rev/c08d0aa688dc

Comment 22

[Bogdan Surd, QA [:bsurd, NI]]

Hello, I've verified this on a Nexus 9 tablet (Android 7.0) and it is indeed taking me to the home screen where the shortcut is created.

Comment 23

[u549602]

I'm seeing this on 52.0b4 too(lack of notifications when adding to home screen). Should an uplift be performed to beta too(is it too risky) or should we let it ride the train?

Comment 24

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: we have a fix, uplifting it may make sense

Comment 25

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8813937 [details]
Bug 1240560 - Extract goToHomeScreen() method to AcitvityUtils

Approval Request Comment
[Feature/Bug causing the regression]: We should let the user knows adding to home screen is completed.
[User impact if declined]: Users may be confused if anything is added to home screen.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: yes
https://bugzilla.mozilla.org/attachment.cgi?id=8813938
[Is the change risky?]: no
[Why is the change risky/not risky?]: It's a simple fix.
[String changes made/needed]: no

Comment 26

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8813938 [details]
Bug 1240560 - After shortcut is created, show the mobile desktop.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Feature/Bug causing the regression]: We should let the user knows adding to home screen is completed.
[User impact if declined]: Users may be confused if anything is added to home screen.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: yes
https://bugzilla.mozilla.org/attachment.cgi?id=8813937
[Is the change risky?]: no
[Why is the change risky/not risky?]: It's a simple fix.
[String changes made/needed]: no

Comment 27

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8813937 [details]
Bug 1240560 - Extract goToHomeScreen() method to AcitvityUtils

prerequisite for next patch, beta52+

Comment 28

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8813938 [details]
Bug 1240560 - After shortcut is created, show the mobile desktop.

visual clue when adding a shortcut to home screen, beta52+

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/037e7115de25
https://hg.mozilla.org/releases/mozilla-beta/rev/8e39356815b9

Comment 30

[Bogdan Surd, QA [:bsurd, NI]]

Hello, 

I've verified this on 52.0b5 using a Galaxy Note 4 (Android 5.0.1).

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/037e7115de25
https://hg.mozilla.org/releases/mozilla-esr52/rev/8e39356815b9