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)
Tracking
(firefox58 verified)
VERIFIED
FIXED
Firefox 58
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)
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) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
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)
Comment 4•6 years ago
|
||
Here: https://mozilla.invisionapp.com/share/3HD23V6KZ
Flags: needinfo?(mliang)
Comment 5•6 years ago
|
||
(In reply to Mark Liang(:mark_liang) from comment #4) > Here: https://mozilla.invisionapp.com/share/3HD23V6KZ \o/ Awesome.
Assignee | ||
Updated•6 years ago
|
Priority: -- → P1
Assignee | ||
Comment 6•6 years ago
|
||
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)
Comment 7•6 years ago
|
||
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) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
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) |
Comment 17•6 years ago
|
||
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) |
Assignee | ||
Comment 23•6 years ago
|
||
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) |
Assignee | ||
Updated•6 years ago
|
Attachment #8915461 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
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•6 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) |
Assignee | ||
Updated•6 years ago
|
Attachment #8912540 -
Attachment is obsolete: true
Attachment #8912540 -
Flags: review?(topwu.tw)
Assignee | ||
Updated•6 years ago
|
Attachment #8913152 -
Attachment is obsolete: true
Attachment #8913152 -
Flags: review?(topwu.tw)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 43•6 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•6 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 46•6 years ago
|
||
There are few lint errors should also be fixed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=52b8b421a1eb&selectedJob=136860261
Comment hidden (mozreview-request) |
Comment 48•6 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•6 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
![]() |
||
Comment 50•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b38a9374daba https://hg.mozilla.org/mozilla-central/rev/385f9bbb4fd8
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 51•6 years ago
|
||
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"
Updated•6 years ago
|
Flags: needinfo?(cnevinchen)
Updated•6 years ago
|
Blocks: progressive-apps
Assignee | ||
Comment 53•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Whiteboard: [pwa-front-end]
Updated•6 years ago
|
Whiteboard: [pwa-front-end] → [pwa-front-end][FNC][SPT58.2][INT]
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•