1167360 - Make "Page added to your reading list" a super toast

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Reading list might not actually be very discoverable, so let's make the toast a super toast that will switch/new-tab you to the reading list home panel.

Need some UX direction here, but let's start with a prototype and see which method of switching you to reading list is better.

Comment 1

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

I think this is a good idea. I have a feeling that "switch" should open a new tab and have Reading List "switched to" :)

Comment 2

[Amin Bandali [:bandali]]

I'd like to work on this issue.

Comment 3

[Amin Bandali [:bandali]]

I had a discussion with Nick Alexander on #mobile about implementing this.

Use of Tabs.TabEvents was suggested, since bookmarks are handled in a similar way (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#489).

We add a Tabs.TabEvents.READING_LIST_ADDED event, and listen for it in the BrowserApp and call the appropriate method when we receive it.

I've tagged nalexander for more info. I'll submit a patch which does the things I described, shortly.

Comment 4

[Amin Bandali [:bandali]]

Attachment: bug1167360_reading-list-button-toast.patch

So, this is what I have so far.

There are 3 issues however:

1. The "Page added to your reading list" is truncated to "Page added to your..." because of the new "Switch to Reading List" button.

2. Clicking the button opens "Top Sites" instead of "Reading List".

3. The page is opened in the current tab (it should be opened in a new tab).

Any help with these is appreciated.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Amin Bandali (:aminb) from comment #4)
> Created attachment 8612672 [details] [diff] [review]
> bug1167360_reading-list-button-toast.patch
> 
> So, this is what I have so far.
> 
> There are 3 issues however:
> 
> 1. The "Page added to your reading list" is truncated to "Page added to
> your..." because of the new "Switch to Reading List" button.

Let's think about just using "Switch to", keeping things short. I think that is our convention in other ButtonToast cases.

> 2. Clicking the button opens "Top Sites" instead of "Reading List".

I think I gave you the wrong panel ID. After looking more closely, I see the ID's are guids, not tags:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeConfig.java#1525

> 3. The page is opened in the current tab (it should be opened in a new tab).

We use the HomePager in two ways:
1. It displays when you navigate to "about:home"
2. It pops up over the current tab when you tap the URL bar.

I think BrowserApp.showHomePager may be doing #2. I think you'll need to manually open a new tab ("about:home"), then call BrowserApp.showHomePager.

There is a different idea:
Add a new tab which also displays the reading list at the same time. You should be able to open "about:home?panel=20f4549a-64ad-4c32-93e4-1dcef792733b" which will open the Home Pager and switch to the reading list. Code is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#2419
> 
> Any help with these is appreciated.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> (In reply to Amin Bandali (:aminb) from comment #4)
> > Created attachment 8612672 [details] [diff] [review]
> > bug1167360_reading-list-button-toast.patch
> > 
> > So, this is what I have so far.
> > 
> > There are 3 issues however:
> > 
> > 1. The "Page added to your reading list" is truncated to "Page added to
> > your..." because of the new "Switch to Reading List" button.
> 
> Let's think about just using "Switch to", keeping things short. I think that
> is our convention in other ButtonToast cases.

For "open in new tab", we use "SWITCH".  We should do the same here.  See

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#80

> > 2. Clicking the button opens "Top Sites" instead of "Reading List".
> 
> I think I gave you the wrong panel ID. After looking more closely, I see the
> ID's are guids, not tags:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> HomeConfig.java#1525
> 
> > 3. The page is opened in the current tab (it should be opened in a new tab).
> 
> We use the HomePager in two ways:
> 1. It displays when you navigate to "about:home"
> 2. It pops up over the current tab when you tap the URL bar.
> 
> I think BrowserApp.showHomePager may be doing #2. I think you'll need to
> manually open a new tab ("about:home"), then call BrowserApp.showHomePager.
> 
> There is a different idea:
> Add a new tab which also displays the reading list at the same time. You
> should be able to open
> "about:home?panel=20f4549a-64ad-4c32-93e4-1dcef792733b" which will open the
> Home Pager and switch to the reading list. Code is here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#2419

Nota bene: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AboutPages.java#105

(This is even tested in Robocop!)

Comment 7

[Amin Bandali [:bandali]]

Attachment: bug1167360_reading-list-button-toast-rev2.patch

Here's a new patch, which solves the 3 problems I mentioned in my last comment.

I have one more question regarding this: Right now, adding to Reading List from the panel (main menu) uses this "super toast", however when long-clicking a link and selecting "Add to Reading List" in the context menu, the old normal toast shows up. Any thoughts?

---

P.S. when uploading this patch, I checked the "Obsoletes" checkbox for my previous patch, hoping it only does what it says.

Comment 8

[Amin Bandali [:bandali]]

Attachment: bug1167360_reading-list-button-toast-rev3.patch

This patch takes care of the "Add to Reading List" in the long-click context menu as well.

BrowserApp implements a new interface, OnReadingListEventListener, defined in the ReadingListHelper, which has 3 methods for displaying appropriate toasts for add/remove/duplicate items.

An instance of the BrowserApp is passed to the ReadingListHelper constructor as the listener, and READING_LIST_ADDED & READING_LIST_REMOVED were added to Tabs.TabEvents and are used to notify BrowserApp from Tab.java.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8613198 [details] [diff] [review]
bug1167360_reading-list-button-toast-rev3.patch

Review of attachment 8613198 [details] [diff] [review]:
-----------------------------------------------------------------

This is a solid patch!  We need UX feedback on the styles.xml change; let's roll-back for now.  I have one substantive request, and that's just to anticipate a consumer's need, which will include the URL of the page.  So thread it through, even though we don't use it quite yet.

r+ since I'll see the changes for landing.  Don't forget to update the commit message!

I'd love to see an UNDO option in the remove from reading list branch...

::: mobile/android/base/BrowserApp.java
@@ +155,5 @@
>                                     BrowserSearch.OnSearchListener,
>                                     BrowserSearch.OnEditSuggestionListener,
>                                     OnUrlOpenListener,
>                                     OnUrlOpenInBackgroundListener,
> +                                   OnReadingListEventListener,

nit: let's make this ReadingListHelper.OnReadingListEventListener, and remove the import.

@@ +597,5 @@
> +    }
> +
> +    private void switchToReadingList() {
> +        final String url = AboutPages.getURLForBuiltinPanelType(PanelType.READING_LIST);
> +        Tabs.getInstance().loadUrlInTab(url);

Fold this in to the onButtonClicked.  In general I like helpers, but let's save some clicks.

::: mobile/android/base/ReadingListHelper.java
@@ +276,2 @@
>       */
> +    private void handleEvent(ReadingListEvent event) {

Include the URL to this function.

@@ +280,5 @@
>              @Override
>              public void run() {
> +                switch(e) {
> +                    case ADDED:
> +                        onReadingListEventListener.onAddedToReadingList();

And thread the URL through to here.

::: mobile/android/base/Tabs.java
@@ +638,5 @@
>          RECORDING_CHANGE,
>          BOOKMARK_ADDED,
> +        BOOKMARK_REMOVED,
> +        READING_LIST_ADDED,
> +        READING_LIST_REMOVED

nit: add trailing ,.

::: mobile/android/base/resources/values/styles.xml
@@ +686,5 @@
>          <item name="android:layout_width">0dp</item>
>          <item name="android:layout_weight">1</item>
>          <item name="android:layout_height">wrap_content</item>
>          <item name="android:layout_gravity">center_vertical</item>
> +        <item name="android:ellipsize">none</item>

This is questionable.  I think we want UX guidance here.

Comment 10

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/98d52b3b2e1a

Comment 11

[Nick Alexander :nalexander [he/him]]

aminb: I made these changes and landed since I think you ran into machine/configuration problems.  I would like to work with you again -- ping me in #mobile so we can find you an interesting ticket.  Thanks for your contribution!

Comment 12

[Amin Bandali [:bandali]]

(In reply to Nick Alexander :nalexander from comment #11)
> aminb: I made these changes and landed since I think you ran into
> machine/configuration problems.  I would like to work with you again -- ping
> me in #mobile so we can find you an interesting ticket.  Thanks for your
> contribution!

Thank you very much for doing the things and landing my patch. Really appreciate it! Like you said I've been having some problems and I haven't been able to contribute.

I look forward to working with you again :)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98d52b3b2e1a

Comment 14

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-06-17-08-43-33.png

Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 41.0a1 (2015-06-16)

1. I think it would be more clear that the super toast should be "Page added to your Reading List | SWITCH" (filled Bug 1175451)

2. Long-tapping on the reader view icon from the URL Bar will display "Added to list | SWITCH" toast notification and you are able to switch to it.
Long-tapping once again on the reader icon from the URL Bar will display too short the notification. You barely see it (filled Bug 1175457)