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

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: liuche, Assigned: aminb, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 41
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Flags: needinfo?(alam)
I think this is a good idea. I have a feeling that "switch" should open a new tab and have Reading List "switched to" :)
Flags: needinfo?(alam)
(Assignee)

Comment 2

4 years ago
I'd like to work on this issue.
Assignee: nobody → me
Mentor: michael.l.comella
(Assignee)

Updated

4 years ago
Mentor: nalexander
Flags: needinfo?(nalexander)
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
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.

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.
(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.
(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!)
Flags: needinfo?(nalexander)
(Assignee)

Comment 7

4 years ago
Created attachment 8613060 [details] [diff] [review]
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.
Attachment #8612672 - Attachment is obsolete: true
Flags: needinfo?(nalexander)
(Assignee)

Comment 8

4 years ago
Created attachment 8613198 [details] [diff] [review]
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.
Attachment #8613060 - Attachment is obsolete: true
Flags: needinfo?(nalexander)
Attachment #8613198 - Flags: review?(nalexander)
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.
Attachment #8613198 - Flags: review?(nalexander) → review+
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!
Status: NEW → ASSIGNED
(Assignee)

Comment 12

4 years ago
(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 :)
https://hg.mozilla.org/mozilla-central/rev/98d52b3b2e1a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Created attachment 8623565 [details]
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)

Updated

4 years ago
Depends on: 1175457
You need to log in before you can comment on or make changes to this bug.