1234319 - Remove add to/remove from reading list button in main menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reading List, defect)

Description

[:Margaret Leibovic]

We should wait for the ability to move bookmarks between folders before doing this, but once that's possible, adding an item to your reading list should be equivalent to moving a bookmark to a special folder.

Comment 1

[:Margaret Leibovic]

I think we should consider doing this before removing "add to reading list" in other places. I'm starting to think that we should make the reading list folder just be a magical folder of your saved reader-view-ed pages, not something that users can organize things into or out of.

Removing this button would help solve our problem of letting users add non-reader-view pages to the reading list. Although to do that completely we'd also need to remove the "add to reading list" button from the share overlay, which I'd be in favor of.

Question: if we remove this item from the main menu, what should the menu look like?

Comment 2

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

Attachment: prev_overflow_mock2.png

Attaching mock.

Removing the "Add to RL" button in the current design puts our menu in bit of a weird position. Without the "shareplane" here by default (bug 1140048), we will have 5 icons. 

To add one more space for quick share icons, I tried making room by changing the top row to 4 icons but that seemed too cramped. I think the better design is to keep it at 3 per row.

Given that and looking at our telemetry, the <- back button gets pressed a fair amount but there's also a hardware back button that does the same thing. I'd like to propose that we remove this icon and simplify the here. 

Bug 1189272 mentions moving the menu position up as well and I think that'll help not only make us more visually consistent with Material design, but also requires less movement for common menu actions like 'Refresh'.

Comment 3

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

Attachment: prev_overflow_mock2_default.png

... and this is what the default "share" would look like for the time being (given that we still can't actually keep the share icon around as I learned in bug 1140048).

Feel free to ignore the new carets next to Page and Tools for the time being, I think that can be out of scope of this bug.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

I like the new menu. My only concern is the back button. Without it there's no visual indication that you can go back. The device's back button might go back or close the app.

Comment 5

[Barbara Bermes [:barbara] - NI please!]

1) Are we scope-creeping here by adding the removal of the back button? More/Complicated Eng work?
2) Based on the telemetry info you mentioned for the back button, maybe we "need to" keep it, any other ideas/compromise how to keep it but solve the removal of the "add to reading list"? Also what are we talking about when you say "pressed a fair amount", compared to? 

I'm in favor of removing the back button but would like us to be able to answer the questions above.

As per Sebastian's comment, close the app/switch the another app would only happen on first load of a page though, no?

Comment 6

[:Margaret Leibovic]

I didn't actually notice the removal of the back button when I looked at this briefly yesterday... this does feel like scope creep that could lead to its own set of problems. Looking at UI telemetry, it looks like people use this button about the same amount as the forward button.

Here's some telemetry data for non-tablets:
https://www.dropbox.com/s/sa9siibgkyhdk2a/Screenshot%202016-01-21%2014.23.06.PNG?dl=0

What did we do in the past? Can we have 4 items across the top of the menu?

I do see that Chrome only has a forward button, so maybe this isn't a problem in practice... but I don't want to go with a design just because Chrome does it.

Comment 7

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

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> I like the new menu. My only concern is the back button. Without it there's
> no visual indication that you can go back. The device's back button might go
> back or close the app.

I'm not too worried about users not knowing they can go back without a back arrow. Since, "going back" to the last page is just something that's become so synonymous with browsing in general. Also, no other apps have "back" in the menu, it's usually in the top left (like our Settings).

The hardware back button is also so ubiquitous to Android devices as well that I think it's the primary way users "go back". Are there devices that don't have a back button?

But, I do share a concern about the inconsistency in it's behavior - sometimes close sometimes back. At the same time though, we already design our experience around the hardware back (tabs close if you came in from another app for example), so I don't see this as a major blocking issue.

(In reply to Barbara Bermes [:barbara] from comment #5)
> 1) Are we scope-creeping here by adding the removal of the back button?
> More/Complicated Eng work?

Perhaps a bit. But removing the "Add to RL" alone is going to be hard because of the design of our UI here.

> 2) Based on the telemetry info you mentioned for the back button, maybe we
> "need to" keep it, any other ideas/compromise how to keep it but solve the
> removal of the "add to reading list"? Also what are we talking about when
> you say "pressed a fair amount", compared to? 

(In reply to :Margaret Leibovic from comment #6)
> I didn't actually notice the removal of the back button when I looked at
> this briefly yesterday... this does feel like scope creep that could lead to
> its own set of problems. Looking at UI telemetry, it looks like people use
> this button about the same amount as the forward button.
> 
> Here's some telemetry data for non-tablets:
> https://www.dropbox.com/s/sa9siibgkyhdk2a/Screenshot%202016-01-21%2014.23.06.
> PNG?dl=0

This is where it starts to sound weird to me actually. Since this is telemetry in our "menu" only, we're also not capturing the system's more reliable way of "going back" (the hardware button). Perhaps these numbers for "back" are simply the left-overs?

> What did we do in the past? Can we have 4 items across the top of the menu?

We've had 4 items in the quick share level before.

> I do see that Chrome only has a forward button, so maybe this isn't a
> problem in practice... but I don't want to go with a design just because
> Chrome does it.

Yeah, I agree. 

But given their large market share, I think we should be aware that they have an ability to influence user's expectations and their appetite for this button (or not having it). 

Also, removing the button and simplifying to just 1 spot for 1 thing is helpful to our UX.

Comment 8

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234319 - Remove reading list button from main menu r=mcomella

Review commit: https://reviewboard.mozilla.org/r/38597/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38597/

Comment 9

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella

Review commit: https://reviewboard.mozilla.org/r/38599/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38599/

Comment 10

[Andrzej Hunt :ahunt]

These two patches only deal with removing the reading list button, the remaining changes are on my TODO list for now.

Comment 11

[Chenxia Liu [:liuche]]

Will this bug also cover "add to reading list" from the book icon in the urlbar? I didn't see a bug for that.

Comment 12

[Andrzej Hunt :ahunt]

(In reply to Chenxia Liu [:liuche] from comment #11)
> Will this bug also cover "add to reading list" from the book icon in the
> urlbar? I didn't see a bug for that.

Is that the long-press to add to reading list? I think that was removed in Bug 1234335.

Comment 13

[Chenxia Liu [:liuche]]

Yep! You're totally right!

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8727666 [details]
MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella

https://reviewboard.mozilla.org/r/38599/#review38641

There are some unused references to `onReadingListEventListener` here – I don't think this compiles.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8727666 [details]
MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella

https://reviewboard.mozilla.org/r/38599/#review38649

r+ when you remove those unused references.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://reviewboard.mozilla.org/r/38599/#review38651

::: mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java:35
(Diff revision 1)
>  import android.util.Log;
>  
>  public final class ReadingListHelper implements NativeEventListener {
>      private static final String LOGTAG = "GeckoReadingListHelper";
>  
>      public interface OnReadingListEventListener {

This interface is also unused.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8727665 [details]
MozReview Request: Bug 1234319 - Remove reading list button from main menu r=mcomella

https://reviewboard.mozilla.org/r/38597/#review38647

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3202
(Diff revision 1)
>              return true;
>          }
>  
>          final boolean inGuestMode = GeckoProfile.get(this).inGuestMode();
>  
>          final boolean isAboutReader = AboutPages.isAboutReader(tab.getURL());

unused

Comment 18

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234319 - Post: Don't track or care about the reading list in Tabs r?mcomella

Review commit: https://reviewboard.mozilla.org/r/42799/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42799/

Comment 19

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1234319 - Post: purge ReadingListAccessor and ReadingListProvider r?mcomella

Review commit: https://reviewboard.mozilla.org/r/42801/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42801/

Comment 20

[Andrzej Hunt :ahunt]

Comment on attachment 8727665 [details]
MozReview Request: Bug 1234319 - Remove reading list button from main menu r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38597/diff/1-2/

Comment 21

[Andrzej Hunt :ahunt]

Comment on attachment 8727666 [details]
MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38599/diff/1-2/

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8735479 [details]
MozReview Request: Bug 1234319 - Post: Don't track or care about the reading list in Tabs r?mcomella

https://reviewboard.mozilla.org/r/42799/#review39351

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8735480 [details]
MozReview Request: Bug 1234319 - Post: purge ReadingListAccessor and ReadingListProvider r?mcomella

https://reviewboard.mozilla.org/r/42801/#review39353

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 1)
>          } else if ("Telemetry:Gather".equals(event)) {
>              final BrowserDB db = getProfile().getDB();
>              final ContentResolver cr = getContentResolver();
>              Telemetry.addToHistogram("PLACES_PAGES_COUNT", db.getCount(cr, "history"));
>              Telemetry.addToHistogram("FENNEC_BOOKMARKS_COUNT", db.getCount(cr, "bookmarks"));
> -            Telemetry.addToHistogram("FENNEC_READING_LIST_COUNT", db.getReadingListAccessor().getCount(cr));

Do we want to keep this for the reading list smart folder?

Comment 24

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a905dcaf551b

Comment 25

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89fa48f3f75c

Comment 26

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/42801/#review39353

> Do we want to keep this for the reading list smart folder?

We'll be adding new  telemetry in Bug 1246159, and I think we also want to move to UI telemetry in general, hence removing this is probably the right thing to do for now?

Comment 27

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e561ea2fbd28

Comment 28

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=322aca65282b

Comment 29

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/1ee8f56284d9887ac08b92063ba47416b29aa661
Bug 1234319 - Remove reading list button from main menu r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/32c0ef305a73ad75a61e0a6b4e25f514ef6aec53
Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/3d82a3fa52dbe1b7621364a4c3ad3d92c5ce5d12
Bug 1234319 - Post: Don't track or care about the reading list in Tabs r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/94e0c7b66782e265c22aa6a90c519ced4a2562de
Bug 1234319 - Post: purge ReadingListAccessor and ReadingListProvider r=mcomella

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ee8f56284d9
https://hg.mozilla.org/mozilla-central/rev/32c0ef305a73
https://hg.mozilla.org/mozilla-central/rev/3d82a3fa52db
https://hg.mozilla.org/mozilla-central/rev/94e0c7b66782

Comment 31

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_20160411-104754.png

Verified as fixed using:
Device: Nexus 6 (Android 6.0) and Nexus 7 (Android 5.1)
Build: Firefox for Android 48.0a1 (2016-04-10)