Closed Bug 1393672 Opened 6 years ago Closed 6 years ago

PWA Badge and Onboarding - Main workflow

Categories

(Firefox for Android Graveyard :: Web Apps (PWAs), enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
Web Apps (PWAs)
Version:
56 Branch
Type:
enhancement

Tracking

(firefox58 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- verified

People

(Reporter: cnevinchen, Assigned: cnevinchen)

References

Details

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

Attachments

(2 files, 4 obsolete files)

Bug 1393672 - Add a badge (at the same position of Page Actio) for PWA.
59 bytes, text/x-review-board-request
jwu
: review+
Details
Bug 1393672 - Show PWA onboarding and confrim prompt
59 bytes, text/x-review-board-request
jwu
: review+
Details 
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.
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)
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)
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)
I'm waiting for detail spec about this. Before I got the measurements, I'll reference focus for Android per Mark's suggestion
Attachment #8915461 - Attachment is obsolete: true
Attachment #8916510 - Attachment is obsolete: true
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+
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 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
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 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+
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
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]
Whiteboard: [pwa-front-end] → [pwa-front-end][FNC][SPT58.2][INT]
The changes made were verified on Nightly 58.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.