1123102 - Reconsider low memory restriction on reading list and reader view

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Richard Newman [:rnewman]]

We currently disable reader mode and reading list on low-memory devices:

base/home/HomeConfigPrefsBackend.java:
77-        // We disable reader mode support on low memory devices. Hence the
78-        // reading list panel should not show up on such devices.
79:        if (!HardwareUtils.isLowMemoryPlatform()) {
80-            panelConfigs.add(createBuiltinPanelConfig(mContext, PanelType.READING_LIST));
81-        }

base/overlays/ui/ShareDialog.java:
220:        if (HardwareUtils.isLowMemoryPlatform()) {
221-            readinglistBtn.setVisibility(View.GONE);
222-            return;
223-        }


(I can't find where we suppress the toolbar button -- margaret?)


The number of entry points for this check is growing -- we'll also need to check whether to show/offer reading functionality in various doorhangers, and during FxA setup. And it becomes a little confusing for users to have a feature just... missing.

We should consider whether we should allow reading list on low-memory devices, but simply not automatically readerify pages on load (which is the expensive part), or not offer readability at all. That is, on low memory devices we would treat reading list as another variety of bookmark.

Opinions?

See also Bug 965335, Bug 872005.

Comment 1

[Nick Alexander :nalexander [he/him]]

Does this get better if we make "offer Reader Mode" an explicit configuration option that is perhaps defaulted off on low-memory device profiles?  Is it worse?

Comment 2

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #1)
> Does this get better if we make "offer Reader Mode" an explicit
> configuration option that is perhaps defaulted off on low-memory device
> profiles?  Is it worse?

Assuming I'm understanding you correctly, that's what I mean by "We should consider whether we should allow reading list on low-memory devices, but simply not automatically readerify pages on load".

Right now we couple reading list with reader mode, and thus we don't offer a cross-platform reading list -- just because we picked one particular implementation strategy for deciding to show the reader mode URL bar icon.

I think we should explore whether we can turn everything on everywhere by altering how/when we do the reader mode analysis (how does Safari do it?), or at the very least decouple reading list so that it works without requiring Readability.

We kinda need to do (or have done) the latter anyway to support sites added to RL that don't have a readable version (cf Bug 1044781).

Comment 3

[:Margaret Leibovic]

Thanks for bringing this up, Richard. I agree that we should show the reading list on all devices if we're offering a reading list service, and we'll need to continue to decouple reader mode/reading list to do this.

I think we can disable the parse-on-load behavior for low memory devices without disabling the reading list, but maybe we'll want to consider some UI for "force-reader-check" this page. Although maybe even forcibly doing it would not work well on these devices.

FYI, we "suppress" the toolbar button by just never checking to see if a page is reader-mode-able, so we never follow the code path to show the icon.

Comment 4

[Richard Newman [:rnewman]]

A rambling account of something I think we discussed with Anthony in Portland:

The reader mode toggle should be just that. We can effectively ignore the long-press functionality that adds to your list -- it's not discoverable.

Given that, we ought to have another way to add pages to the reading list: one that's more discoverable.

If we solve that -- allow a user to add a page to the reading list as easily as they can add to their bookmarks -- then, given that we already permit any kind of URL to be stored, the only difference on low-mem devices is that we don't offer reader mode. The other reading list entry points are all the same.

That might be enough to separate out your concern about how to request reader mode on these devices -- perhaps via the Page menu, or an affordance in the reading list itself (inverting the problem).

Comment 5

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

(In reply to Richard Newman [:rnewman] from comment #4)
> A rambling account of something I think we discussed with Anthony in
> Portland:
> 
> The reader mode toggle should be just that. We can effectively ignore the
> long-press functionality that adds to your list -- it's not discoverable.

This would be some sort of animation to re-enforce this nature of the "reading mode" to the user. (bug 1114708). 

> Given that, we ought to have another way to add pages to the reading list:
> one that's more discoverable.

I agree. We should keep looking at this. I'm hoping that the new reader mode controls would help with this (bug 1120004). But I think the accumulative effect of the controls and animation will help this the most once they're both done.

Comment 6

[Richard Newman [:rnewman]]

I have a strong opinion that it should not be necessary to invoke Reader Mode in order to quickly add a page to my Reading List. Improved controls inside Reader Mode are great, but I don't think they solve the overall problem.

Comment 7

[Edwin Wong [:edwong]]

+1 on decoupling RM and RL. Users want to add to RL before the page loads (esp on 3G)

Comment 8

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #5)

> > Given that, we ought to have another way to add pages to the reading list:
> > one that's more discoverable.
> 
> I agree. We should keep looking at this. I'm hoping that the new reader mode
> controls would help with this (bug 1120004). But I think the accumulative
> effect of the controls and animation will help this the most once they're
> both done.

This bug is for updating the controls that are in reader mode, so I don't think that helps us address the problem of how to add something to your reading list if reader mode isn't supported on your device.

antlam, can you help me think about what the over reading list experience would look like on a device where reader *mode* isn't supported?

As it is, we have a fallback to just load reading list items as normal webpages if they can't be parsed as articles, so we could do that for everything in your reading list. Is that acceptable for a start? The thing about this approach is that we don't show any reader mode controls, so there won't be an immediate way to remove something from your list after you open it (you would have to go back to the list).

Comment 9

[:Margaret Leibovic]

Another thing to consider here: the "tip" that we show in the empty state of the reading list panel talks about long pressing the book icon to add items to your reading list. We should get rid of this, or update this tip to be more generic, in the case that reader mode background parsing is disabled.

Isn't there another bug filed somewhere about replacing this long press behavior with a more obvious way to add items to your reading list? I suppose we could cover that in bug 1125365.

Comment 10

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #9)
> Another thing to consider here: the "tip" that we show in the empty state of
> the reading list panel talks about long pressing the book icon to add items
> to your reading list. We should get rid of this, or update this tip to be
> more generic, in the case that reader mode background parsing is disabled.

Good call!

> Isn't there another bug filed somewhere about replacing this long press
> behavior with a more obvious way to add items to your reading list? I
> suppose we could cover that in bug 1125365.

I'd intended Bug 1127445 for that. We should also update the tip to match whatever we add there.

Comment 11

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

Bug 1103232 maybe?

I've been thinking about this for sure so I'll leave this NI on for now. This also pertains to all the Share overlay work Mike's been doing too. 

+1 for updating empty state!

Comment 12

[:Margaret Leibovic]

> > Isn't there another bug filed somewhere about replacing this long press
> > behavior with a more obvious way to add items to your reading list? I
> > suppose we could cover that in bug 1125365.
> 
> I'd intended Bug 1127445 for that. We should also update the tip to match
> whatever we add there.

Thanks, that's the bug I had in mind.

Comment 13

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1123102/margaret

/r/3873 - Bug 1123102 - Always create reading list panel, regardless of device memory capabilities

Pull down this commit:

hg pull review -r 3f4e9eb103a8734a379d36fe4acdf87d15b52bd8

Comment 14

[:Margaret Leibovic]

I have a patch here to remove the low memory restriction for reading list, and it will migrate the user's panel config if they've customized it previously. I decided to just add the reading list panel to the end of the list on both tablets and phones, I think this is the right decision, given that the default config has it on the right on both.

Part of me feels like we should solve bug 1127445 before landing this, but if we're committed to solving that problem in the same release cycle, that shouldn't actually matter.

Comment 15

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/3873/#review3077

Nits, but nothing more.  Looks good!

::: mobile/android/base/home/HomeConfigPrefsBackend.java
(Diff revision 1)
> +                Log.e(LOGTAG, "Exception loading PanelConfig from JSON", e);

I always like to make it clear I'm ignoring an exception (if I am).

::: mobile/android/base/home/HomeConfigPrefsBackend.java
(Diff revision 1)
> +                                PanelType.READING_LIST, Position.BACK, Position.BACK);

This will not agree with the default configuration, which has REMOTE_TABS after READING_LIST on some devices.  (Right?)  I don't think it's a big deal, but perhaps worth noting.

I'd also like to add more context to the comment, something like:

// At one time, the Reading List panel was shown only to devices that were not considered "low memory".  Now, we expose the panel to all devices.  This migration should only occur for "low memory" devices.

::: mobile/android/base/overlays/ui/ShareDialog.java
(Diff revision 1)
>          // If we're a low memory device, just hide the reading list button. Otherwise, configure it.

Seems like this comment is out of date now.

::: mobile/android/base/util/HardwareUtils.java
(Diff revision 1)
> -    // Minimum memory threshold for a device to be considered

Wow, this was the only usage?  Go us!

Comment 16

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/3873/#review3145

Ship It!

Comment 17

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/78bb737b85ea

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78bb737b85ea

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8564414 [details]
MozReview Request: bz://1123102/margaret

This landed.

Comment 21

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1123102 - Always create reading list panel, regardless of device memory capabilities