Add "Add to reading list" button in the browser menu

RESOLVED FIXED in Firefox 38

Status

()

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

People

(Reporter: edwong, Assigned: mcomella)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38+ fixed, firefox39 fixed, fennec38+)

Details

MozReview Requests

()

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

Attachments

(21 attachments, 2 obsolete attachments)

165.19 KB, image/png
Details
17.20 KB, application/zip
Details
42.52 KB, application/zip
Details
8.77 KB, application/zip
Details
3.04 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
88.27 KB, image/png
antlam
: feedback+
Details
80.03 KB, image/png
Details
81.70 KB, image/png
Details
504.77 KB, image/png
antlam
: feedback+
Details
128.63 KB, image/png
antlam
: feedback+
Details
957.08 KB, image/png
Details
429.27 KB, text/plain
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
39 bytes, text/x-review-board-request
Margaret
: review+
Details | Review
(Reporter)

Description

3 years ago
My browsing workflow on fennec is:
1. goto google news, hacker news, local news site
2. long press on each article worthy to be read, and open link in new tab
3. read each tab

I would love to long press, add to reading list then read later or on my desktop!
This should be handled by a part of Bug 1103232. I don't see a specific dependency on file for the decoupling of adding from RM itself, so let's use this bug for that.

"Discoverable" and "stable" means *not* long-pressing the reader mode icon, by the way!
Blocks: 1103232
Component: Overlays → Reader Mode
Summary: Please add the ability to long press on a link and add it to my ReadingList. → Expose discoverable, stable UI for adding pages and links to reading list
Blocks: 871994
See also: bug 814587

This bug added "Add to Reading List" to the context menu, but it was removed in bug 872005. But I think bug 872005 is no longer valid, since we are always going to allow devices to add to the reading list.
(Reporter)

Comment 3

3 years ago
My description could have been more clear.  I'd like the ability to 'Add to Reading List' without even attempting to load the page.  

STR:
1. goto news.ycombinator.com
2. long press on a hyperlink
3. context menu comes up

actual: there is no way to add to Reading List

expected:  'Add to Reading List' is one of the options in context menu.
Yeah, part of this bug will be undoing Bug 814587, which will address your concern, Ed.

Comment 5

3 years ago
(In reply to Richard Newman [:rnewman] from comment #4)
> Yeah, part of this bug will be undoing Bug 814587, which will address your
> concern, Ed.

That bug was fixed, and then it was reverted by bug 872005. I think we should open a new bug to add that feature back, now that we're okay with having non-reader-modeable pages in reading list.
Blocks: 1123102

Updated

3 years ago
tracking-fennec: --- → ?

Updated

3 years ago
Blocks: 1125365

Comment 6

3 years ago
Note: We should remember to update the tip in the empty reading list panel to reflect whatever this new UI is.

antlam, I know I flagged you for info in bug 1123102, but I think we should just fix this add-to-reading-list UI issue here.
Flags: needinfo?(alam)
(In reply to :Margaret Leibovic from comment #5)
> (In reply to Richard Newman [:rnewman] from comment #4)
> > Yeah, part of this bug will be undoing Bug 814587, which will address your
> > concern, Ed.
> 
> That bug was fixed, and then it was reverted by bug 872005. I think we
> should open a new bug to add that feature back, now that we're okay with
> having non-reader-modeable pages in reading list.

Is this bug 1107293 by any chance? 

(In reply to :Margaret Leibovic from comment #6)
> Note: We should remember to update the tip in the empty reading list panel
> to reflect whatever this new UI is.
> 
> antlam, I know I flagged you for info in bug 1123102, but I think we should
> just fix this add-to-reading-list UI issue here.


Sure, works for me. Thinking about this design right now
Component: Reader View → Reading List

Updated

3 years ago
Blocks: 1134361
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 38+

Comment 8

3 years ago
antlam shared a whiteboard sketch with me yesterday to include an "add to reading list" button in our menu, near the bookmark star.

antlam, do you have a mock-up for that?

Comment 9

3 years ago
(In reply to :Margaret Leibovic from comment #5)
> (In reply to Richard Newman [:rnewman] from comment #4)
> > Yeah, part of this bug will be undoing Bug 814587, which will address your
> > concern, Ed.
> 
> That bug was fixed, and then it was reverted by bug 872005. I think we
> should open a new bug to add that feature back, now that we're okay with
> having non-reader-modeable pages in reading list.

Filed bug 1134927 for that.
Summary: Expose discoverable, stable UI for adding pages and links to reading list → Add "Add to reading list" button in the browser menu
Created attachment 8568814 [details]
prev_addtoRL_mock1.png

Attaching the non-whiteboard version :)
Attachment #8567164 - Attachment is obsolete: true
Flags: needinfo?(alam)
Created attachment 8569976 [details]
icon_bookmarkstar.zip

Attaching assets for the bookmark star. 

Pressed state of each menu item (background) should be at ##D7D7DC, and could we also double check the dividers are #D7D9DB? they look a bit bluer to me... 

The other icons take from the Reader View updates in Bug 1120004 so we can reuse those ones there.
Created attachment 8569980 [details]
icon_RView.zip

Attaching zip from Bug 1120004 that contains Add to RL, Remove from RL, and Share icons.
Leaving a note here: hopefully we can loop in some of the UI clean up in here too from the rounded corners, and shadows from our doorhangers- UI consistency!

Updated

3 years ago
No longer blocks: 1103232
Created attachment 8572256 [details]
icon_bigshareplane.zip
Created attachment 8573397 [details] [diff] [review]
Part 0: Add strings for manipulating the reading list from the menu
Attachment #8573397 - Flags: review?(margaret.leibovic)
Lukas, we'd like to land and uplift the strings in comment 16 to 38 (Aurora) for this feature that adds the ability to add a page to the reading list from the toolbar menu (see comment 11 for a mock). Is that possible?
Flags: needinfo?(lukasblakk+bugs)
Note that the implementation is still underway so that this would be a pre-landing of strings.

Comment 19

3 years ago
Comment on attachment 8573397 [details] [diff] [review]
Part 0: Add strings for manipulating the reading list from the menu

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

r+, these are consistent with the existing strings for desktop/mobile reading list.
Attachment #8573397 - Flags: review?(margaret.leibovic) → review+
From what I understand, reading list is exempt from string-freeze and if that is still true then yes please nominate for uplift.
Flags: needinfo?(lukasblakk+bugs)
Comment on attachment 8573397 [details] [diff] [review]
Part 0: Add strings for manipulating the reading list from the menu

Approval Request Comment
[Feature/regressing bug #]: Preland strings for this bug

[User impact if declined]:
  This bug, a menu item to add to reading list, likely won't land

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]:
  For these strings, just the usual localization risks. 

[String/UUID change made/needed]: This patch
Attachment #8573397 - Flags: approval-mozilla-aurora?
Created attachment 8573689 [details]
MozReview Request: bz://1127445/mcomella

/r/4941 - Bug 1127445 - Part 0: Add strings for manipulating the reading list from the menu. r=margaret
/r/4943 - Bug 1127445 - Add new reading list, share, and bookmark icons. r=margaret
/r/4945 - Bug 1127445 - Add reading list item to menu. r=margaret
/r/4947 - Bug 1127445 - Add quick share bar (i.e. third row) to toolbar menu. r=margaret
/r/4949 - Bug 1127445 - Remove vertical dividers from toolbar menu & quick share. r=margaret

Pull down these commits:

hg pull review -r 767985af62363cec1991add36f3fbf069ccfcdb5
Attachment #8573689 - Flags: review?(margaret.leibovic)
Still TODO:
  * Make the "add to reading list" button do something
  * Styling w/ antlam
Created attachment 8573690 [details]
N4 menu

Anthony, what do you think?
Attachment #8573690 - Flags: feedback?(alam)
Created attachment 8573691 [details]
N4 no quick share menu

When we have no quick share items, I hide the quick share menu bar.
Created attachment 8573693 [details]
N4 1 quick share item

And I add the bar and additional items as necessary.

Looks a bit goofy with 1, but looks better with 2 - let me know if you want a build.
Created attachment 8573695 [details]
Pre L context menu

My commits had a side effect of removing the vertical dividers from the quick share context menus. This looks good on Lollipop where there are no dividers, but looks a bit fishy on prior versions.

What do you think, Anthony?
Attachment #8573695 - Flags: feedback?(alam)
Mike - Wow! Impressive. attachment 8573690 [details] (3 quickshare) and attachment 8573691 [details] (0 quickshare) look really awesome. attachment 8573693 [details] (1 quickshare) does look a little odd, but maybe left aligning it would be better?

Overall, I think these look great.
Comment on attachment 8573690 [details]
N4 menu

Aweeeeesome!!
Attachment #8573690 - Flags: feedback?(alam) → feedback+
Comment on attachment 8573695 [details]
Pre L context menu

I think no divider works still. as long as we space each of the 3 icons centered in its own 1/3 container like this.
Attachment #8573695 - Flags: feedback?(alam) → feedback+
Comment on attachment 8573693 [details]
N4 1 quick share item

Would it be possible to add "placeholder" / empty icons there?
Flags: needinfo?(michael.l.comella)
(In reply to Anthony Lam (:antlam) from comment #31)
> Would it be possible to add "placeholder" / empty icons there?

I can try - what icon would you want there? And should the non-placeholder icons left or right align?
Flags: needinfo?(michael.l.comella)
Attachment #8573397 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox38: --- → affected
status-firefox39: --- → affected
tracking-firefox38: --- → +
Flags: needinfo?(alam)
Actually, let's look at these "placeholder"s later. I need to think about the idea a bit more and make sure we cover all our bases. 

In the mean time, centered works for me. It may look a bit different but it also has the added benefit of being a larger hit areas for user's to quickly "re-share" something. Likewise, I'd expect two item situations to center the app icons within their own 1/2 space horizontally.
Flags: needinfo?(alam)
Comment on attachment 8573689 [details]
MozReview Request: bz://1127445/mcomella

/r/4941 - Bug 1127445 - Part 0: Add strings for manipulating the reading list from the menu. r=margaret
/r/4943 - Bug 1127445 - Add new reading list, share, and bookmark icons. r=margaret
/r/4945 - Bug 1127445 - Add reading list item to menu. r=margaret
/r/4947 - Bug 1127445 - Add quick share bar (i.e. third row) to toolbar menu. r=margaret
/r/4949 - Bug 1127445 - Remove vertical dividers from toolbar menu & quick share. r=margaret
/r/5027 - Bug 1127445 - Add functionality to reading list button in the toolbar menu. r=margaret

Pull down these commits:

hg pull review -r a45f6a29030301f92054a2ab020898f93f2a54bd
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=077e70cc80ba

The push in comment 34 is complete, with an exception for an inconsistency in how reading list items are handled - I'm investigating. This is noted in the final commit's message and repasted below for convenience:

Note: the reader mode "add to reading list" button has inconsistent behavior
with the "add to reading list" button just added. I believe this is because
LocalReadingListAccessor distinguishes between reader and non-reader urls while
the JS implmentation does not.
Comment on attachment 8573689 [details]
MozReview Request: bz://1127445/mcomella

/r/4941 - Bug 1127445 - Part 0: Add strings for manipulating the reading list from the menu. r=margaret
/r/4943 - Bug 1127445 - Add new reading list, share, and bookmark icons. r=margaret
/r/4945 - Bug 1127445 - Add reading list item to menu. r=margaret
/r/4947 - Bug 1127445 - Add quick share bar (i.e. third row) to toolbar menu. r=margaret
/r/4949 - Bug 1127445 - Remove vertical dividers from toolbar menu & quick share. r=margaret
/r/5027 - Bug 1127445 - Add functionality to reading list button in the toolbar menu. r=margaret
/r/5029 - Bug 1127445 - Set toolbar menu reader item title based on state. r=margaret

Pull down these commits:

hg pull review -r aa13648c6400c3cdcb11acfb49f7d13895dcb0c4
try in comment 35 broke on 2.3 devices - I'll fix it later.
Margaret, do you know how to fix the issue in comment 35?
Flags: needinfo?(margaret.leibovic)

Comment 39

3 years ago
https://reviewboard.mozilla.org/r/4943/#review4103

Make sure you pngcrush/trimmage/whatever these new icons.

Comment 40

3 years ago
https://reviewboard.mozilla.org/r/4945/#review4105

I think you missed adding this to /resources/menu/browser_app_menu.xml

Maybe that's the problem you were seeing with Gingerbread? r+ with that addressed.

Comment 41

3 years ago
https://reviewboard.mozilla.org/r/4947/#review4109

I, too, am not completely comfortable with the GeckoMenu code, but this seems reasonable to me, and I'm okay with the pattern matching. In the future it might be nice to rename some of these variables, since I think this code suffers from piece-meal evolution, but that's not something to worry about in a patch we want to uplift.

::: mobile/android/base/menu/GeckoMenu.java
(Diff revision 1)
> +        mQuickShareActionItems = new HashMap<GeckoMenuItem, View>();

The naming of these variables is confusing, but I think this code was already confusing before you got here :(

::: mobile/android/base/widget/GeckoActionProvider.java
(Diff revision 1)
> +    public View onCreateActionView(final int maxHistorySize, final boolean isForQuickShareBar) {

Don't love the boolean flag here, but this does seem like the most straightforward way to do this.

Comment 43

3 years ago
https://reviewboard.mozilla.org/r/5027/#review4115

Ship It!

::: mobile/android/base/BrowserApp.java
(Diff revision 1)
> +        final MenuItem reader = aMenu.findItem(R.id.reading_list);

Want to file a [good first bug] to make all these local variables final?

::: mobile/android/base/BrowserApp.java
(Diff revision 1)
> +            reader.setEnabled(false);

Yeah, this would cause a crash on 2.3 if you didn't include the item in the xml.

::: mobile/android/base/BrowserApp.java
(Diff revision 1)
> +                    Telemetry.sendUIEvent(TelemetryContract.Event.UNSAVE, TelemetryContract.Method.MENU, "reading_list");

Nice, good call to add telemetry now.

::: mobile/android/base/Tab.java
(Diff revision 1)
> +                mIsInReadingList = mDB.getReadingListAccessor().isReadingListItem(getContentResolver(), url);

Can you elaborate on the issue you were seeing here with isInReadingList? This seems to me like it should work...

However, we probably should add some logic to handle what this menu item will do when the user is in reader view. We could probably just get the original URL from the about:reader URL, and have this menu item act the same way that the reader view toolbar button works.

Updated

3 years ago
Attachment #8573689 - Flags: review?(margaret.leibovic) → review+

Comment 45

3 years ago
Comment on attachment 8573689 [details]
MozReview Request: bz://1127445/mcomella

https://reviewboard.mozilla.org/r/4939/#review4119

Ship It!

Comment 46

3 years ago
(In reply to Michael Comella (:mcomella) from comment #35)
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=077e70cc80ba
> 
> The push in comment 34 is complete, with an exception for an inconsistency
> in how reading list items are handled - I'm investigating. This is noted in
> the final commit's message and repasted below for convenience:
> 
> Note: the reader mode "add to reading list" button has inconsistent behavior
> with the "add to reading list" button just added. I believe this is because
> LocalReadingListAccessor distinguishes between reader and non-reader urls
> while
> the JS implmentation does not.

Let's move investigating this issue into a follow-up bug. I'd like us to land this work on Nightly sooner rather than later so that we can shake out bugs before uplift.
Flags: needinfo?(margaret.leibovic)
Created attachment 8575694 [details]
xlarge tablet no items

Note that the bookmarks star changed because of this.
Attachment #8575694 - Flags: feedback?(alam)
Attachment #8575694 - Attachment description: xlarge tablet layout → xlarge tablet no items
Comment on attachment 8573689 [details]
MozReview Request: bz://1127445/mcomella

/r/4941 - Bug 1127445 - Part 0: Add strings for manipulating the reading list from the menu. r=margaret
/r/4943 - Bug 1127445 - Add new reading list, share, and bookmark icons. r=margaret
/r/4945 - Bug 1127445 - Add reading list item to menu. r=margaret
/r/4947 - Bug 1127445 - Add quick share bar (i.e. third row) to toolbar menu. r=margaret
/r/4949 - Bug 1127445 - Remove vertical dividers from toolbar menu & quick share. r=margaret
/r/5027 - Bug 1127445 - Add functionality to reading list button in the toolbar menu. r=margaret
/r/5029 - Bug 1127445 - Set toolbar menu reader item title based on state. r=margaret
/r/5093 - Bug 1127445 - Fix Android 2.3 issues in toolbar menu. r=margaret
/r/5095 - Bug 1127445 - Fix inconsistencies between v11 and v14. r=margaret

Pull down these commits:

hg pull review -r acbd50b0ef611cee6aee900a1175a750e4558633
Attachment #8573689 - Flags: review+ → review?(margaret.leibovic)
https://reviewboard.mozilla.org/r/4945/#review4123

It's a little more complicated than that - see my latest review request.
Nick, can you test the changesets in comment 49 on a HC device? Just open the menu and make sure it either looks like the attachment in comment 48, or what you'd expect its GB equivalent to look like.
Flags: needinfo?(nalexander)
Comment on attachment 8575694 [details]
xlarge tablet no items

Looks good. 

What do you mean by "changed"?
Flags: needinfo?(michael.l.comella)
Attachment #8575694 - Flags: feedback?(alam) → feedback+
(In reply to Anthony Lam (:antlam) from comment #54)
> What do you mean by "changed"?

We use the new bookmarks asset in the toolbar (rather than in the menu as on smaller devices) - looks a bit off, imo.
Flags: needinfo?(michael.l.comella) → needinfo?(alam)
https://reviewboard.mozilla.org/r/5027/#review4203

> Want to file a [good first bug] to make all these local variables final?

https://bugzilla.mozilla.org/show_bug.cgi?id=1142192

> Can you elaborate on the issue you were seeing here with isInReadingList? This seems to me like it should work...
> 
> However, we probably should add some logic to handle what this menu item will do when the user is in reader view. We could probably just get the original URL from the about:reader URL, and have this menu item act the same way that the reader view toolbar button works.

https://bugzilla.mozilla.org/show_bug.cgi?id=1142196
(In reply to Michael Comella (:mcomella) from comment #55)
> We use the new bookmarks asset in the toolbar (rather than in the menu as on
> smaller devices) - looks a bit off, imo.

nvm - the asset (new_tablet_ic_menu_bookmark_*) hasn't changed - I got confused from the new tablet resource interaction.
Flags: needinfo?(alam)

Updated

3 years ago
Attachment #8573689 - Flags: review?(margaret.leibovic) → review+

Comment 59

3 years ago
Comment on attachment 8573689 [details]
MozReview Request: bz://1127445/mcomella

https://reviewboard.mozilla.org/r/4939/#review4215

Reviewed the interdiff between versions 3 and 4, looks good to me.

::: mobile/android/base/BrowserApp.java
(Diff revisions 3 - 4)
> -        final boolean shareEnabled = RestrictedProfiles.isAllowed(this, RestrictedProfiles.Restriction.DISALLOW_SHARE);
> +        final boolean shareVisible = RestrictedProfiles.isAllowed(this, RestrictedProfiles.Restriction.DISALLOW_SHARE);

Nice variable rename. One at a time, we can make this code more readable! :)
NI self to make sure this gets handled today.
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #53)
> Nick, can you test the changesets in comment 49 on a HC device? Just open
> the menu and make sure it either looks like the attachment in comment 48, or
> what you'd expect its GB equivalent to look like.

I downloaded a recent fx-team [1].  I installed it and definitely see the Reading List item in the menu [2], but I also see a blank line.  Perhaps Quick Share is not yet filled in?

[1] http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/fx-team-android-api-11/1426178767/fennec-39.0a1.en-US.android-arm.apk
[2] https://people.mozilla.org/~nalexander/screenshots/Honeycomb.Reading.List.Menu.Item.png
Flags: needinfo?(nalexander)
Lukas, I forgot to land the Strings-preland patch we initially flagged for uplift to Aurora so it never got uplifted - can this be uplifted? I'm going to let the feature bake on Nightly in the meantime. Note that the String patch actually landed with the feature in comment 60, but the attached patch should be the same.
Flags: needinfo?(lukasblakk+bugs)
Duplicate of this bug: 1136828
Duplicate of this bug: 1125365

Updated

3 years ago
Depends on: 1143034

Updated

3 years ago
Depends on: 1143032
Uplift okayed by lmandel via irc, landed in comment 67. (ty!)

NI self to bake and uplift the full feature.
Flags: needinfo?(lukasblakk+bugs)
Margaret, next time, please update also the tracking flags ;)
status-firefox38: affected → fixed

Comment 70

3 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #69)
> Margaret, next time, please update also the tracking flags ;)

I only uplifted the string changeset, not the rest, so we still need to uplift the rest of the changesets. We just want to give them some more time to bake on m-c and fix any fallout.
status-firefox38: fixed → affected
(Reporter)

Comment 71

3 years ago
Just tried this out on nightly... love it.  Amazing work people!
Tested using:
Device: Alcatel One Touch (Android 4.1.2) and Samsung Galaxy Tab (Android 4.0.4)
Build: Firefox for Android 38.0a1 (2015-03-17)

phone:
-add to reading list from browser menu: http://i.imgur.com/ycJuIGS.png
-remove from reading list from browser menu: http://i.imgur.com/esMgb3R.png
-menu UI:
*1 quick share item: http://i.imgur.com/dBw3pw0.png
*2 quick share items: http://i.imgur.com/Rs6rIPa.png
*3 quick share items: http://i.imgur.com/O0RtHkW.png

tablet:
-add to reading list from browser menu: http://i.imgur.com/uY85eFM.png
-remove from reading list from browser menu: http://i.imgur.com/tVNhrEa.png
-menu UI:
*1 quick share item; http://i.imgur.com/gsEeqnW.png
*2 quick share items: http://i.imgur.com/Fxxnmke.png
*3 quick share items: http://i.imgur.com/rsbaTGe.png

Is it expected to add to reading list the about:home page through the reading list icon from menu? or any "about:" pages. After opening "Nightly Home" from Reading list panel, the message  "loading..." appears and nothing happens.
Flags: needinfo?(michael.l.comella)
(In reply to Teodora Vermesan (:TeoVermesan) from comment #72)
> Is it expected to add to reading list the about:home page through the
> reading list icon from menu? or any "about:" pages.

Yes - we've had a discussion about the bookmarks button before and decided it's better not to add special-case code if it isn't necessary (e.g. if you want to bookmark about:home, well, then that's your problem).

> After opening "Nightly
> Home" from Reading list panel, the message  "loading..." appears and nothing
> happens.

However, I wasn't expecting ^. That's bad.

Margaret, I get "loading..." when I add non-reader pages to my reading list (e.g. apple.com), before the page actually loads. How easy would this be to fix? Options:
  1) Fix the "loading..." behavior, which may fix about:home
  2) Dig into the about:home code and find out why about:home doesn't load and is an exception (probably because HomePager.show() isn't called - maybe the same issue as above)
  3) Special case the "Add to reading list button"

Given our time constraints, I vote 3, but I'd prefer to not have non-reader pages display "loading...".
Flags: needinfo?(michael.l.comella) → needinfo?(margaret.leibovic)

Comment 74

3 years ago
So you're saying that you're just seeing the "Loading..." text appear forever in reader view? That should only be temporary until we either find an article or not... is there anything in the log? In the case where we fail to load an article in reader view, we should be bailing to just load the original URL.

If this only happens with about:home, I suspect the problem here is that our code to load the URL "about:home" from JS doesn't properly trigger showing the Java UI.

I hate to say it, but I think we should just disable this button in the menu when we're on about:home, since we already do that for a bunch of other buttons in the menu.
Flags: needinfo?(margaret.leibovic)
Created attachment 8579953 [details]
readermode.txt

Here are the logs while adding "about:firefox" to reading list and opened thw page from the Reader Mode panel.

Yes, "Loading..." text appears forever in reader view.
It happens with "about:home", "about:firefox", "about:downloads", "about:addons", "about:passwords" etc.

Comment 76

3 years ago
(In reply to Teodora Vermesan (:TeoVermesan) from comment #75)
> Created attachment 8579953 [details]
> readermode.txt
> 
> Here are the logs while adding "about:firefox" to reading list and opened
> thw page from the Reader Mode panel.
> 
> Yes, "Loading..." text appears forever in reader view.
> It happens with "about:home", "about:firefox", "about:downloads",
> "about:addons", "about:passwords" etc.

Can you file a separate bug for this? It seems like this needs more investigation.
bug 1145217 for the quick fix.

bug 1145234 for the full fix.
Comment on attachment 8573689 [details]
MozReview Request: bz://1127445/mcomella

Note: this is intended for 38 - I will handle the uplift myself because it's a bit messy. This must be uplifted with bug 1145217 and bug 1142196.

Approval Request Comment
[Feature/regressing bug #]: Reader mode being more prominent in FF38

[User impact if declined]:
  Users cannot directly add arbitrary pages to their reading list (i.e. they have to go through the long-press context menu action). about:reader pages can still be added to the reading list through reader mode.

[Describe test coverage new/current, TreeHerder]:
  None

[Risks and why]:
  We don't modify much existing code so I don't see many risks in breaking existing functionality. However, we do add some new code that could break the appearance of the browser menu. Also, the functionality we added uses new methods so there can be some edge cases in which this does not work. However, it's fairly simple ("Add to reading list" and "Remove from reading list") so we're likely to have already seen the edge cases. I'm not worrying too much about this change.

[String/UUID change made/needed]: Part 0 (already uplifted)
Flags: needinfo?(michael.l.comella)
Attachment #8573689 - Flags: approval-mozilla-aurora?
Attachment #8573689 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox38: affected → fixed
I found an issue where about:reader pages added to the reading list with an about:reader URL cannot be removed from the reading list due to bug 1142196, which strips the about:reader url from the page to be removed.

Adding an about:reader url directly to the reading list would only be possible after this bug landed, but before bug 1142196 landed. Since all of these changesets landed together on aurora, this issue *only affects Nightly users* (unless about:reader urls get added to the reading list some other way in the future).

bug 1127445 is a workaround fix and bug 1147027 would be a proper solution that would prevent future issues.
Tested using:
Device: Samsung S5 (Android 4.4) and Nexus 7 (Android 5.0)
Build: Firefox for Android 38.0a2 (2015-03-26)

"Add to reading list" button is present in the Custom menu. After tapping the icon, a notification is displayed "Page added to your reading list" and the garbage icon is present in the Custom Menu. After tapping this icon, a notification is displayed "Page removed from your reading list".
Duplicate of this bug: 1132746
Comment on attachment 8573689 [details]
MozReview Request: bz://1127445/mcomella
Attachment #8573689 - Attachment is obsolete: true
Attachment #8619242 - Flags: review+
Attachment #8619243 - Flags: review+
Attachment #8619244 - Flags: review+
Attachment #8619245 - Flags: review+
Attachment #8619246 - Flags: review+
Attachment #8619247 - Flags: review+
Attachment #8619248 - Flags: review+
Attachment #8619249 - Flags: review+
Attachment #8619250 - Flags: review+
Created attachment 8619242 [details]
MozReview Request: Bug 1127445 - Add functionality to reading list button in the toolbar menu. r=margaret
Created attachment 8619243 [details]
MozReview Request: Bug 1127445 - Fix Android 2.3 issues in toolbar menu. r=margaret
Created attachment 8619244 [details]
MozReview Request: Bug 1127445 - Fix inconsistencies between v11 and v14. r=margaret
Created attachment 8619245 [details]
MozReview Request: Bug 1127445 - Part 0: Add strings for manipulating the reading list from the menu. r=margaret
Created attachment 8619246 [details]
MozReview Request: Bug 1127445 - Add new reading list, share, and bookmark icons. r=margaret
Created attachment 8619247 [details]
MozReview Request: Bug 1127445 - Add reading list item to menu. r=margaret
Created attachment 8619248 [details]
MozReview Request: Bug 1127445 - Add quick share bar (i.e. third row) to toolbar menu. r=margaret
Created attachment 8619249 [details]
MozReview Request: Bug 1127445 - Remove vertical dividers from toolbar menu & quick share. r=margaret
Created attachment 8619250 [details]
MozReview Request: Bug 1127445 - Set toolbar menu reader item title based on state. r=margaret
You need to log in before you can comment on or make changes to this bug.