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 :(
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
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
Created attachment 8497957 [details] [diff] [review] bug1057667.diff 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.
push to try-server https://tbpl.mozilla.org/?tree=Try&rev=aaf9f1b4fd76
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.