PWA Badge and Onboarding - Main workflow

VERIFIED FIXED in Firefox 58

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: cnevinchen, Assigned: cnevinchen)

Tracking

(Blocks 1 bug)

56 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 verified)

Details

(Whiteboard: [pwa-front-end][FNC][SPT58.2][INT])

Attachments

(2 attachments, 4 obsolete attachments)

Currently there is a design for PWA page action and onboarding. 
This bug is test for it and maybe separate page action and oboarding to two bugs in the future.
Comment hidden (mozreview-request)
Hi Mark
Please help post your design and spec here. So we can record the discussion and have our module owner/reviewer to review it before we investigate more time to implement this feature.
Thanks a lot!
Flags: needinfo?(mliang)
(In reply to Mark Liang(:mark_liang) from comment #4)
> Here: https://mozilla.invisionapp.com/share/3HD23V6KZ

\o/ Awesome.
Priority: -- → P1
Hi Dale

May I know what does manifest.install()[1] do? 
Is there a reason why you didn't pass the resolved manifest JSON string directly, instead just pass the url and icon?

I want to get the icon image from the manifest before the user decides to add it as app shortcut(see comment 4 page 3)
Can I just pass the entire JSON o
What I want to do is:
1. From Javaside, get the manifest url when I got "Link:Manifest" event [2]
2. Pass result JSON string reolved from (1) into "Browser:LoadManifestz" [3]
3. If possible, pass that Json String around from to  "Website:AppInstalled" event[4]  

Please 

[1] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/mobile/android/chrome/content/browser.js#2238

[2] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/mobile/android/base/java/org/mozilla/gecko/Tabs.java#650

[3] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#505

[4] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#505
Flags: needinfo?(dharvey)
Hey Nevin, the code for Manifest is @ http://searchfox.org/mozilla-central/source/dom/manifest/Manifest.jsm#81, when I wrote that ManifestObtainer already existing and was tested so Manifest uses that to fetch the manifest data. 

If we want to prefetch the manifest every time we see link:manifest we will need to be very sure they are cached as we will see them often and do not want to be using up users data for this, if you do cache it then you can cache it with an id that Java can see and java can read the manifest directly @ http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/webapps/WebAppManifest.java including the cached_icon field which we create @ http://searchfox.org/mozilla-central/source/dom/manifest/Manifest.jsm#99
Flags: needinfo?(dharvey)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Hi Mark.
Please find this test APK before I request code review.
Thank you!

https://drive.google.com/open?id=0Bw-r8iYPh2ZDb0NsVG5GZ1ZJVU0
Flags: needinfo?(mliang)
Comment hidden (mozreview-request)
I added a few UI details in the spec document:
https://mozilla.invisionapp.com/share/QFDRJ08M2

Let me know when you are done updating, thanks!
Flags: needinfo?(mliang)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I'm waiting for detail spec about this. Before I got the measurements, I'll reference focus for Android per Mark's suggestion
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8915461 - Attachment is obsolete: true
Attachment #8916510 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

2 years ago
mozreview-review
Comment on attachment 8901130 [details]
Bug 1393672 - Add a badge (at the same position of Page Actio) for PWA.

https://reviewboard.mozilla.org/r/172606/#review193474

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:377
(Diff revision 6)
>          }
>  
> +        public static void showPwaPageAction() {
> +            GeckoBundle bundle = new GeckoBundle();
> +            bundle.putString("id", UUID_PAGE_ACTION_PWA);
> +            bundle.putString("title", "Add PWA Shortcut");

Should we assign title from string resource to support multiple languages?
Attachment #8901130 - Flags: review?(topwu.tw) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8912540 - Attachment is obsolete: true
Attachment #8912540 - Flags: review?(topwu.tw)
Attachment #8913152 - Attachment is obsolete: true
Attachment #8913152 - Flags: review?(topwu.tw)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

2 years ago
mozreview-review
Comment on attachment 8912149 [details]
Bug 1393672 - Show PWA onboarding and confrim prompt

https://reviewboard.mozilla.org/r/183528/#review194096

::: mobile/android/app/src/main/res/layout/pwa_confirm.xml:12
(Diff revision 8)
> +                                      android:layout_marginTop="@dimen/browser_toolbar_height_flipper"
> +                                      android:background="@color/dark_transparent_overlay"
> +                                      android:clickable="true">
> +
> +
> +    <RelativeLayout

Since `PwaConfirm` is already a RelativeLayout, should we need another RelativeLayout here? Why not just combine these two ViewGroups?

::: mobile/android/app/src/main/res/layout/pwa_confirm.xml:43
(Diff revision 8)
> +            android:layout_toEndOf="@+id/pwa_confirm_icon_wrap"
> +            android:layout_toStartOf="@+id/pwa_confirm_cancel"

To support older version than API 17, you should use `layout_toRightOf/layout_toLeftOf` here.

I think a better solution is moving the attributes to different styles.xml
1. move `layout_toRightOf/layout_toLeftOf` into `values/styles.xml` and
2. move `layout_toEndOf/layout_toStartOf` into `values-v17/styles.xml`

::: mobile/android/app/src/main/res/layout/pwa_confirm.xml:62
(Diff revision 8)
> +            android:layout_alignParentEnd="true"
> +            android:layout_alignParentTop="true"
> +            android:background="@drawable/pwa_cancel_button"
> +            android:scaleType="center"
> +            android:src="@drawable/ic_cancel_nm"/>
> +x

Typo error here?

::: mobile/android/app/src/photon/res/values/colors.xml:273
(Diff revision 8)
>  
>      <color name="bookmark_folder_bg_color">#FCFCFC</color>
> +
> +    <!-- PWA -->
> +    <color name="pwa_cancel_btn_press">#ededf0</color>
> +    <color name="pwa_cancel_btn">#00ffffff</color>

Perhaps using `@android:color/transparent` would make the meaning more clear.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:138
(Diff revision 8)
>              final boolean useTint = message.getBoolean("useTint");
>  
>              addPageAction(id, title, imageURL, useTint, new OnPageActionClickListeners() {
>                  @Override
>                  public void onClick(final String id) {
> -                    if (id != null && id.equals(PageAction.UUID_PAGE_ACTION_PWA)) {
> +                    if (id != null && id.equals(UUID_PAGE_ACTION_PWA)) {

The expression can be simplified to `UUID_PAGE_ACTION_PWA.equals(id)`

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:163
(Diff revision 8)
>          }
>      }
>  
> +    private void maybeShowPwaOnboarding(String id) {
> +        // only show pwa at normal mode
> +        if (id.equals(UUID_PAGE_ACTION_PWA) && Tabs.getInstance().getSelectedTab().isPrivate() == false) {

Since we're sure `id` is null or not, using `UUID_PAGE_ACTION_PWA.equals(id)` can prevent NullPointerException.

Also the return value of `getSelectedTab()` is nullable [1], we should also add a null check before accessing it.

[1] https://dxr.mozilla.org/mozilla-central/rev/3d918ff5d63442d7b88e1b7e9cb03b832bc28fdf/mobile/android/base/java/org/mozilla/gecko/Tabs.java#376
Assignee

Comment 44

2 years ago
mozreview-review-reply
Comment on attachment 8912149 [details]
Bug 1393672 - Show PWA onboarding and confrim prompt

https://reviewboard.mozilla.org/r/183528/#review194096

> Since `PwaConfirm` is already a RelativeLayout, should we need another RelativeLayout here? Why not just combine these two ViewGroups?

The outer RelativeLayout is used as an overlay. So it must exist. The inner relative layout is the prompt itself. So these two can't be combined
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 48

2 years ago
mozreview-review
Comment on attachment 8912149 [details]
Bug 1393672 - Show PWA onboarding and confrim prompt

https://reviewboard.mozilla.org/r/183528/#review194636
Attachment #8912149 - Flags: review?(topwu.tw) → review+

Comment 49

2 years ago
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b38a9374daba
Add a badge (at the same position of Page Actio) for PWA. r=jwu
https://hg.mozilla.org/integration/autoland/rev/385f9bbb4fd8
Show PWA onboarding and confrim prompt r=jwu
https://hg.mozilla.org/mozilla-central/rev/b38a9374daba
https://hg.mozilla.org/mozilla-central/rev/385f9bbb4fd8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Can someone explain where "Add PWA to Home Screen" is displayed?
https://hg.mozilla.org/mozilla-central/rev/385f9bbb4fd8#l18.65

It would be better to add localization comments when a string is unclear out of context, e.g. why there's a + in "+ Add to Home Screen"
Flags: needinfo?(cnevinchen)
I've opened bug 1409261 for this.
Flags: needinfo?(cnevinchen)
As Andreas said, we should call the newly added icon as badge, just at the same position of page aciton. So I rename this bug.
Please remember to turn on Settings->Advanced->Progressive Web App to enable this feature.
Summary: PWA Page Action and Onboarding → PWA Badge and Onboarding - Main workflow
Whiteboard: [pwa-front-end]
Duplicate of this bug: 1395846
Duplicate of this bug: 1395839
Duplicate of this bug: 1379115

Updated

2 years ago
Whiteboard: [pwa-front-end] → [pwa-front-end][FNC][SPT58.2][INT]

Comment 57

2 years ago
The changes made were verified on Nightly 58.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1378421
You need to log in before you can comment on or make changes to this bug.