Closed Bug 949178 Opened 10 years ago Closed 10 years ago

Remove reading list button from reader mode toolbar

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

All
Android
defect

Tracking

(firefox29 wontfix, firefox30 verified, firefox31 verified, firefox32 verified, fennec+)

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified
fennec + ---

People

(Reporter: lucasr, Assigned: Margaret)

References

Details

(Whiteboard: shovel-ready)

Attachments

(2 files, 1 obsolete file)

The Reader Mode toolbar has a button that takes you straight to the 'Reading List' page in about:home. The current code assumes that the 'Reading List' pane will always be in about:home. However, by making about:home configurable, this might not be the case anymore.

mfinkle suggested that maybe we should simply force the 'Reading List' page to show up in about:home if coming from the Reader Mode UI (even if the user has hidden the Reading List page via Settings UI).

The 'Reading List' button in the Reader Mode UI would become less relevant if we implement things like bug 750678 and bug 871593.
Depends on: 949181
Doing a little investigation on the code for this. We send a message from JS to Java that gets handled here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1207

Which then lands us in here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1427

Which causes us to send a message back to JS about the reading list:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tabs.java#727

Which we then get back later in Java:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#659

I think we should take this opportunity to take a step back and simplify the way all this works. I don't think Tab/Tabs should know about HomePager stuff. And I really don't think that JS should need to know about the HomePager pages. It looks like all this was added for this feature in the first place:
http://hg.mozilla.org/mozilla-central/rev/3ce8681e5602

But maybe we can come up with a better solution now that this code has evolved a bit?
Depends on: 950919
Feel free to redirect if you don't have time to work on this yourself.
Assignee: nobody → lucasr.at.mozilla
tracking-fennec: --- → 29+
(In reply to Lucas Rocha (:lucasr) from comment #0)
> mfinkle suggested that maybe we should simply force the 'Reading List' page
> to show up in about:home if coming from the Reader Mode UI (even if the user
> has hidden the Reading List page via Settings UI).

Suggestion from bug 963174: how about we just show the reading list fragment instead of the entire home pager? One of the benefits of fragments is that they're self-contained, reusable modules, so this seems like a good place to take advantage of that.
Priority: -- → P1
Depends on: 871593
No longer depends on: 871593
Ian, Finkle wants me to NI you so you know that this isn't happening in 29
tracking-fennec: 29+ → +
Flags: needinfo?(ibarlow)
What's the plan here? As a temporary measure, should we just hide the reading list button in reader mode? Right now, if the user doesn't have the reading list panel in their home config, we'll just land on the default panel, which isn't the end of the world, but it's not ideal.

This issue affects Fx29, so we should decide if we're okay with this current situation, or if we want some temporary measure.

We could try to implement a fix to just show the reading list fragment like bnicholson suggested in comment 4, but that probably isn't something we'll want to uplift.

I vote that we should just land a patch to hide this button. With sola's fix for bug 909618, the user would be able to hit the back button to get back to their reading list after finishing an article (although that wouldn't be true if they entered reader mode from a webpage, as opposed to the reading list).
(In reply to :Margaret Leibovic from comment #6)

> I vote that we should just land a patch to hide this button. With sola's fix
> for bug 909618, the user would be able to hit the back button to get back to
> their reading list after finishing an article

+1, exactly this.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #7)
> (In reply to :Margaret Leibovic from comment #6)
> 
> > I vote that we should just land a patch to hide this button. With sola's fix
> > for bug 909618, the user would be able to hit the back button to get back to
> > their reading list after finishing an article
> 
> +1, exactly this.

Sounds like a good plan.
Assignee: lucasr.at.mozilla → margaret.leibovic
Summary: Rethink access to Reading List from Reader Mode ui → Remove reading list button from reader mode toolbar
It also looks like _readingListCount is only used for updating this button, but I held off on removing that logic in case there is some other planned use for it.

However, if you think we should get rid of it, I can update the patch to do that.
Attachment #8409911 - Flags: review?(lucasr.at.mozilla)
Attached image screenshot
I just adjusted the style to have the remaining 3 buttons fill up the space.
Attachment #8409912 - Flags: feedback?(ibarlow)
Comment on attachment 8409911 [details] [diff] [review]
Remove reading list button from reader mode toolbar

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

I think you can safely remove the 'list count' message bits. Giving f+ to get an updated patch with these changes.
Attachment #8409911 - Flags: review?(lucasr.at.mozilla) → feedback+
Updated to remove reading list count logic.
Attachment #8409911 - Attachment is obsolete: true
Attachment #8410315 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8410315 [details] [diff] [review]
(v2) Remove reading list button from reader mode toolbar

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

Look good, thanks.
Attachment #8410315 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8410315 [details] [diff] [review]
(v2) Remove reading list button from reader mode toolbar

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 942231 (too late for fx29)
User impact if declined: reading list button in reader mode won't work properly if reading list panel is disabled
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, removes a button from reader mode toolbar
String or IDL/UUID changes made by this patch: none
Attachment #8410315 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6e826c32b6f2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8410315 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in builds:
- 30 beta 1;
- 31.0a2 (2014-04-29);
- 32.0a1 (2014-04-29).
Device: Samsung Galaxy R (Android 2.3.4).
Status: RESOLVED → VERIFIED
Comment on attachment 8409912 [details]
screenshot

just clearing some feedback flags, don't mind me
Attachment #8409912 - Flags: feedback?(ibarlow) → feedback+
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.