If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
Reading List
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Margaret, Assigned: ahunt)

Tracking

(Blocks: 1 bug)

35 Branch
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Reporter)

Description

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

Comment 1

2 years ago
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?
Flags: needinfo?(alam)
Created attachment 8710124 [details]
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'.
Flags: needinfo?(alam)
Created attachment 8710125 [details]
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.
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.
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?
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(alam)
(Reporter)

Comment 6

2 years ago
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.
Flags: needinfo?(margaret.leibovic)
(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.
Flags: needinfo?(alam)

Updated

2 years ago
No longer depends on: 1232437
(Assignee)

Comment 8

2 years ago
Created attachment 8727665 [details]
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/
Attachment #8727665 - Flags: review?(michael.l.comella)
(Assignee)

Comment 9

2 years ago
Created attachment 8727666 [details]
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/
Attachment #8727666 - Flags: review?(michael.l.comella)
(Assignee)

Comment 10

2 years ago
These two patches only deal with removing the reading list button, the remaining changes are on my TODO list for now.
Will this bug also cover "add to reading list" from the book icon in the urlbar? I didn't see a bug for that.
(Assignee)

Comment 12

2 years ago
(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.
Yep! You're totally right!
Assignee: nobody → ahunt
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.
Attachment #8727666 - Flags: review?(michael.l.comella)
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.
Attachment #8727666 - Flags: review+
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 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
Attachment #8727665 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 18

2 years ago
Created attachment 8735479 [details]
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/
Attachment #8727665 - Attachment description: MozReview Request: Bug 1234319 - Remove reading list button from main menu r?mcomella → MozReview Request: Bug 1234319 - Remove reading list button from main menu r=mcomella
Attachment #8727666 - Attachment description: MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r?mcomella → MozReview Request: Bug 1234319 - Post: cleanup/remove OnReadingListEventListener r=mcomella
Attachment #8735479 - Flags: review?(michael.l.comella)
Attachment #8735480 - Flags: review?(michael.l.comella)
(Assignee)

Comment 19

2 years ago
Created attachment 8735480 [details]
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/
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
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 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
Attachment #8735479 - Flags: review?(michael.l.comella) → review+
Attachment #8735480 - Flags: review?(michael.l.comella) → review+
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?
(Assignee)

Comment 24

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a905dcaf551b
(Assignee)

Comment 25

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89fa48f3f75c
(Assignee)

Comment 26

2 years ago
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?
(Assignee)

Comment 27

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e561ea2fbd28
(Assignee)

Comment 28

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=322aca65282b
(Assignee)

Comment 29

2 years ago
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

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Created attachment 8739866 [details]
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)
status-firefox48: fixed → verified
(Assignee)

Updated

2 years ago
Blocks: 1265433
You need to log in before you can comment on or make changes to this bug.