1153384 - Reader Mode no longer triggers on many pages, including ones that worked earlier

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Markus Popp]

Here are some Austrian news sites (and articles) where Reader Mode doesn't work. On many of this pages it worked a few weeks ago:

http://derstandard.at/2000014110146/Entwurf-zu-Rauchverbot-steht (Failed to load article from page)

http://orf.at/stories/2272872/ (no Reader icon appears)

http://kurier.at/politik/weltchronik/usa-selbstmordanschlag-auf-militaerbasis-in-kansas-vereitelt/124.364.599 (also no Reader icon)

Some articles do work, for example:

http://derstandard.at/2000014152981/Mailand-zittert-der-Expo-entgegen

Don't know what's different with this one. Looks pretty arbitrary to me which articles work with Reader Mode and which don't.

Comment 1

[:Gijs (he/him)]

(In reply to Markus Popp from comment #0)
> Here are some Austrian news sites (and articles) where Reader Mode doesn't
> work. On many of this pages it worked a few weeks ago:
> 
> http://derstandard.at/2000014110146/Entwurf-zu-Rauchverbot-steht (Failed to
> load article from page)

This now works in beta (and presumably Nightly)

> http://orf.at/stories/2272872/ (no Reader icon appears)

This is still broken.

> http://kurier.at/politik/weltchronik/usa-selbstmordanschlag-auf-
> militaerbasis-in-kansas-vereitelt/124.364.599 (also no Reader icon)

This seems to now work as well?


Can you confirm this, or are you still seeing the same issues?

Comment 2

[Markus Popp]

That's correct, I'm getting the same results in both Beta 7 and Nightly (of April 24th).

Comment 3

[:Gijs (he/him)]

Attachment: MozReview Request: bz://1153384/Gijs

/r/7681 - Bug 1153384 - improve isProbablyReaderable detection, r?margaret

Pull down this commit:

hg pull -r 9316c968b30d082bd268b0d043244bc9a940c481 https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[:Gijs (he/him)]

Comment on attachment 8598043 [details]
MozReview Request: bz://1153384/Gijs

See also: https://github.com/mozilla/readability/pull/188

Comment 5

[:Margaret Leibovic]

Comment on attachment 8598043 [details]
MozReview Request: bz://1153384/Gijs

https://reviewboard.mozilla.org/r/7679/#review6459

::: toolkit/components/reader/ReaderMode.jsm:121
(Diff revision 1)
> +    return new Readability(uri, doc).isProbablyReaderable(this.isNodeVisible.bind(this, utils));

Maybe add a comment here explaining why we need to pass in this helper function?

I also wonder if passing a helper function like this to Readability.parse() could also be a better way of removing hidden elements like I was trying to do in this pull request... https://github.com/mozilla/readability/pull/186

But then again, having `isProbablyReaderable` depend on some gecko functionality feels more acceptable than having `parse` depend on it.

Comment 6

[Pulsebot]

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

Comment 7

[:Gijs (he/him)]

Comment on attachment 8598043 [details]
MozReview Request: bz://1153384/Gijs

Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: reader mode not available for pages where it should be
[Describe test coverage new/current, TreeHerder]: has tests in github repo
[Risks and why]: medium-low, low because only affects github repo, medium-ish because it changes when we show the reader mode button
[String/UUID change made/needed]: nope.

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8598043 [details]
MozReview Request: bz://1153384/Gijs

Approving for uplift to aurora and to beta (from discussion with Sylvestre). 
We want this to be in the  38.0.5 but not in 38.0, since Reader Mode is disabled for 38.0 on Desktop.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ab4d0b26019

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/edf69409dc26

Comment 13

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1153384 - improve isProbablyReaderable detection, r?margaret