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

Status: RESOLVED FIXED

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

Description

[Edwin Wong [:edwong]]

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!

Comment 1

[Richard Newman [:rnewman]]

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!

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Edwin Wong [:edwong]]

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.

Comment 4

[Richard Newman [:rnewman]]

Yeah, part of this bug will be undoing Bug 814587, which will address your concern, Ed.

Comment 5

[:Margaret Leibovic]

(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.

Comment 6

[:Margaret Leibovic]

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.

Comment 7

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

(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

Comment 8

[:Margaret Leibovic]

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

[:Margaret Leibovic]

(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.

Comment 10

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

Attachment: Whiteboard mockup

Comment 11

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

Attachment: prev_addtoRL_mock1.png

Attaching the non-whiteboard version :)

Comment 12

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

Attachment: 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.

Comment 13

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

Attachment: icon_RView.zip

Attaching zip from Bug 1120004 that contains Add to RL, Remove from RL, and Share icons.

Comment 14

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

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!

Comment 15

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

Attachment: icon_bigshareplane.zip

Comment 16

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

Attachment: Part 0: Add strings for manipulating the reading list from the menu

Comment 17

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

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?

Comment 18

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

Note that the implementation is still underway so that this would be a pre-landing of strings.

Comment 19

[:Margaret Leibovic]

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.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

From what I understand, reading list is exempt from string-freeze and if that is still true then yes please nominate for uplift.

Comment 21

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

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

Comment 22

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

Attachment: 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

Comment 23

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

Still TODO:
  * Make the "add to reading list" button do something
  * Styling w/ antlam

Comment 24

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

Attachment: N4 menu

Anthony, what do you think?

Comment 25

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

Attachment: N4 no quick share menu

When we have no quick share items, I hide the quick share menu bar.

Comment 26

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

Attachment: 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.

Comment 27

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

Attachment: 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?

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

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 29

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

Comment on attachment 8573690 [details]
N4 menu

Aweeeeesome!!

Comment 30

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

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.

Comment 31

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

Comment on attachment 8573693 [details]
N4 1 quick share item

Would it be possible to add "placeholder" / empty icons there?

Comment 32

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

(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?

Comment 33

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

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.

Comment 34

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

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

Comment 35

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

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 36

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

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

Comment 37

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

try in comment 35 broke on 2.3 devices - I'll fix it later.

Comment 38

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

Margaret, do you know how to fix the issue in comment 35?

Comment 39

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/4943/#review4103

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

Comment 40

[:Margaret Leibovic]

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

[:Margaret Leibovic]

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 42

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/4949/#review4113

Ship It!

Comment 43

[:Margaret Leibovic]

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.

Comment 44

[:Margaret Leibovic]

https://reviewboard.mozilla.org/r/5029/#review4117

Ship It!

Comment 45

[:Margaret Leibovic]

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

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

Ship It!

Comment 46

[:Margaret Leibovic]

(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.

Comment 47

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

Attachment: xlarge tablet no items

Note that the bookmarks star changed because of this.

Comment 48

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

Attachment: xlarge tablet 3 items

Comment 49

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

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

Comment 50

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

https://reviewboard.mozilla.org/r/4943/#review4121

histedit'ed and done.

Comment 51

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

https://reviewboard.mozilla.org/r/4945/#review4123

It's a little more complicated than that - see my latest review request.

Comment 52

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

https://reviewboard.mozilla.org/r/4947/#review4127

Hee hee: https://bugzilla.mozilla.org/show_bug.cgi?id=1140208

Comment 53

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

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.

Comment 54

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

Comment on attachment 8575694 [details]
xlarge tablet no items

Looks good. 

What do you mean by "changed"?

Comment 55

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

(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.

Comment 56

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

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

Comment 57

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

(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.

Comment 58

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

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

Comment 59

[:Margaret Leibovic]

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! :)

Comment 60

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/5453e0dba62a
remote:   https://hg.mozilla.org/integration/fx-team/rev/c8134ff4b35b
remote:   https://hg.mozilla.org/integration/fx-team/rev/583dba61396d
remote:   https://hg.mozilla.org/integration/fx-team/rev/aadbc0a67a2c
remote:   https://hg.mozilla.org/integration/fx-team/rev/9a4a85d08498
remote:   https://hg.mozilla.org/integration/fx-team/rev/4962ecc0a9c3
remote:   https://hg.mozilla.org/integration/fx-team/rev/ad8d8f341940
remote:   https://hg.mozilla.org/integration/fx-team/rev/fa9f4d788144
remote:   https://hg.mozilla.org/integration/fx-team/rev/b8a714b12790

I realized I forgot to pre-land the Strings - trying to talk to the right people about uplifting part 0 early.

Comment 61

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

NI self to make sure this gets handled today.

Comment 62

[Nick Alexander :nalexander [he/him]]

(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

Comment 63

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

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.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5453e0dba62a
https://hg.mozilla.org/mozilla-central/rev/c8134ff4b35b
https://hg.mozilla.org/mozilla-central/rev/583dba61396d
https://hg.mozilla.org/mozilla-central/rev/aadbc0a67a2c
https://hg.mozilla.org/mozilla-central/rev/9a4a85d08498
https://hg.mozilla.org/mozilla-central/rev/4962ecc0a9c3
https://hg.mozilla.org/mozilla-central/rev/ad8d8f341940
https://hg.mozilla.org/mozilla-central/rev/fa9f4d788144
https://hg.mozilla.org/mozilla-central/rev/b8a714b12790

Comment 67

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc2702ad5761

Comment 68

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

Uplift okayed by lmandel via irc, landed in comment 67. (ty!)

NI self to bake and uplift the full feature.

Comment 69

[Sylvestre Ledru [:Sylvestre]]

Margaret, next time, please update also the tracking flags ;)

Comment 70

[:Margaret Leibovic]

(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.

Comment 71

[Edwin Wong [:edwong]]

Just tried this out on nightly... love it.  Amazing work people!

Comment 72

[Teodora Vermesan (:TeoVermesan)]

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.

Comment 73

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

(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...".

Comment 74

[:Margaret Leibovic]

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.

Comment 75

[Teodora Vermesan (:TeoVermesan)]

Attachment: 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

[:Margaret Leibovic]

(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.

Comment 77

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

bug 1145217 for the quick fix.

bug 1145234 for the full fix.

Comment 78

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

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)

Comment 79

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/637170048f0c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/3319f2d57a8d
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5501134877bf
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d6177ac54122
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/8d29f7ea81e2
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c8532b69607b
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a8d54ccb5306
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/469cca05c231

Comment 80

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

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.

Comment 81

[Teodora Vermesan (:TeoVermesan)]

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".

Comment 84

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

Attachment: MozReview Request: Bug 1127445 - Add functionality to reading list button in the toolbar menu. r=margaret

Comment 85

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

Attachment: MozReview Request: Bug 1127445 - Fix Android 2.3 issues in toolbar menu. r=margaret

Comment 86

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

Attachment: MozReview Request: Bug 1127445 - Fix inconsistencies between v11 and v14. r=margaret

Comment 87

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

Attachment: MozReview Request: Bug 1127445 - Part 0: Add strings for manipulating the reading list from the menu. r=margaret

Comment 88

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

Attachment: MozReview Request: Bug 1127445 - Add new reading list, share, and bookmark icons. r=margaret

Comment 89

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

Attachment: MozReview Request: Bug 1127445 - Add reading list item to menu. r=margaret

Comment 90

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

Attachment: MozReview Request: Bug 1127445 - Add quick share bar (i.e. third row) to toolbar menu. r=margaret

Comment 91

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

Attachment: MozReview Request: Bug 1127445 - Remove vertical dividers from toolbar menu & quick share. r=margaret

Comment 92

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

Attachment: MozReview Request: Bug 1127445 - Set toolbar menu reader item title based on state. r=margaret