872005 - Reader Mode: Able to add pages to reading list on devices incapable of accessing Reader Mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This kind of slipped by. One can long-tap and hold on a link and 'Add to Reading List' on devices that don't meet the minimum-bar requirement (see bug 792603 for details).

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#420
--
Samsung Nexus S (Android 4.1)
Nightly (05/14)

Comment 1

[Aaron Train [:aaronmt]]

So the deal here is, a user can access the saved item in the reading list and is able to enter Reader Mode. There would be confusion because they're unable to access Reader Mode elsewhere via an article being parsed. Backdoor access to reader-mode. Should this be?

Comment 2

[Ian Barlow (:ibarlow)]

I'm seeing this on devices that support Reader Mode as well, and when you try to add something that can't be parsed by our Reader you get a fail message. It makes for pretty crummy UX.

We should pull it on lower end devices, and also on higher end devices until we have a better UX designed for adding things that Reader Mode can't parse.

Comment 3

[:Margaret Leibovic]

Attachment: patch to remove context menu item

If I'm understanding Ian's last comment correctly, we want to get rid of this context menu item altogether until we can figure out a better UX for things we can't parse.

I didn't touch the string so that we can uplift this without string churn, but I can make another patch that gets rid of it for trunk.

Comment 4

[:Margaret Leibovic]

This context menu item was only added in 23... why did we mark this bug as status-firefox21/22:affected?

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee75d69fe879

Comment 6

[:Margaret Leibovic]

Comment on attachment 750767 [details] [diff] [review]
patch to remove context menu item

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 814587
User impact if declined: users can add articles to their reading list on devices where we don't support reader mode
Testing completed (on m-c, etc.): tested locally
Risk to taking this patch (and alternatives if risky): low-risk, removes context menu item
String or IDL/UUID changes made by this patch: n/a

Comment 7

[:Margaret Leibovic]

needinfo? to Aaron to ask about the status flags.

Comment 8

[Aaron Train [:aaronmt]]

(In reply to :Margaret Leibovic from comment #4)
> This context menu item was only added in 23... why did we mark this bug as
> status-firefox21/22:affected?

Confirmed. My mistake.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee75d69fe879

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

flagging relnote to remind myself to put this in on or before we go to beta with 23.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/66a6991a3146

Comment 12

[Paul Feher]

Aurora 24.0a2 (2013-07-02)
Firefox for Android 23.0b2 (2013-07-02)
Device: LG Optimus 4x 
OS:Android 4.1.2