Closed Bug 1057667 Opened 10 years ago Closed 10 years ago

Expand selection of articles available for viewing in ReaderMode

Categories

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

ARM
Android
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(1 file)

I wonder if we'd consider removing the Experimental ReaderMode block in Readability.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js?rev=21c0c5f02e83&mark=706-713#704

The "contrastRatio" prevents articles from being able to be viewed in ReaderMode. News articles on "The Hill" for example are routinely blocked, as their ratios can range from ~55 - ~75 during testing.

Simply raising the ratio threshold would help, but not eliminate all false positives currently triggered.

I've patched the check out entirely, locally, and haven't yet found where this might be an issue. In the worst-case, we'd flag a page as being readerMode enabled. A user that then selects the option will have a page badly rendered I suppose, but that currently happens on some pages in any case.

Thoughts?
I guess I'll ni? this ... there may be a reason that this is a bad idea, but I still haven't run into one after a couple weeks of use ... I use reader mode a *lot* :-D
And then of course I didn't actually set the flag :(
Flags: needinfo?(lucasr.at.mozilla)
I added this in 782285. Removing it will probably cause more false positives. From IRC:

<lucasr> capella, maybe it's the case to see why the false negatives are happening?
<lucasr> and figure out a proper change to the algorithm?
<lucasr> a 'proper change' might mean replacing that experimental bit with something smarter
Flags: needinfo?(lucasr.at.mozilla)
Final bit from IRC:

12:47	<capella>	I'm thinking the need for this check has been mitigated
12:49	<lucasr>	capella, I'm fine with removing it if you post a little 'report' covering the cases that bug 782285 fixed back then
12:49	<lucasr>	if these cases are not an issue without the check, let's go head and remove it
Attached patch bug1057667.diffSplinter Review
As I mentioned earlier, the thought is that currently a good number of articles are false negatives / prevented from entering ReaderMode, due to the "experimental code".

I think the main reason for the experimental code was to address UI display on YouTube pages. Since the UI has changed (page actions, etc), navigating to that page:
   http://www.youtube.com/watch?v=hu2rRqFazuA

With or without the patch that page now looks acceptably as:
   https://www.dropbox.com/s/kqu1kbj0ke0k6xz/bug1057667.png?dl=0

So, that original reason seems to have been mitigated. Regarding the rest of the links mentioned throughout, I see the following behaviors, none of which seems un-reasonable to me:

http://www.nytimes.com/pages/technology/index.html
   Has Readermode icon, clicking displays a subset of text information, with pictures.

http://edition.cnn.com/WORLD/
   Has *no* Readermode icon, as some pages will still continue to fail Readermode eligibility due to lack of meaningful content check:
      http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js?rev=21c0c5f02e83&mark=692-692#687

http://en.m.wikipedia.org/wiki/Mozilla
   Has Readermode icon, clicking displays a subset of text information w/o pictures.

http://www.slate.com/posts/2012/06/22/new_law_threatens_mississippi_s_last_abortion_clinic.html
   (Gets to a 404 page currently, with suggested articles)
   Has Readermode icon, clicking displays articles / information w/o pictures.

https://www.google.com/search?q=query&ie=utf-8&oe=utf-8&channel=fs&gws_rd=ssl
   Has *no* Readermode icon, due to lack of meaningful content check.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8497957 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8497957 [details] [diff] [review]
bug1057667.diff

Given that http://www.nytimes.com/pages/technology/index.html was the only false positive, I'm fine with going ahead with this. However, I'd prefer us to land this patch in the beginning of the next cycle so that we have more time to bake it in Nightly. Thoughts?
Attachment #8497957 - Flags: review?(lucasr.at.mozilla) → review+
Fantastic :-) It looks like next release is 10-14 ...
so I'll plan to push to fx-team / m-c at around my b'day, 10-16.
https://hg.mozilla.org/mozilla-central/rev/7086c82a7c38
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.