Closed Bug 1153384 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Reader Mode, defect)

38 Branch
defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: markus.popp, Assigned: Gijs)

References

Details

(Whiteboard: [do not verify before bug 1158228 has landed])

Attachments

(1 file, 1 obsolete file)

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.
(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?
Flags: needinfo?(markus.popp)
That's correct, I'm getting the same results in both Beta 7 and Nightly (of April 24th).
Flags: needinfo?(markus.popp)
Attached file MozReview Request: bz://1153384/Gijs (obsolete) —
/r/7681 - Bug 1153384 - improve isProbablyReaderable detection, r?margaret

Pull down this commit:

hg pull -r 9316c968b30d082bd268b0d043244bc9a940c481 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8598043 [details]
MozReview Request: bz://1153384/Gijs

See also: https://github.com/mozilla/readability/pull/188
Attachment #8598043 - Flags: review?(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.
Attachment #8598043 - Flags: review?(margaret.leibovic) → review+
Iteration: --- → 40.2 - 27 Apr
Points: --- → 5
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
OS: Linux → All
Hardware: x86_64 → All
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #8598043 - Flags: approval-mozilla-beta?
Attachment #8598043 - Flags: approval-mozilla-aurora?
Whiteboard: [do not verify before bug 1158228 has landed]
https://hg.mozilla.org/mozilla-central/rev/a6e46837c274
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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.
Attachment #8598043 - Flags: approval-mozilla-beta?
Attachment #8598043 - Flags: approval-mozilla-beta+
Attachment #8598043 - Flags: approval-mozilla-aurora?
Attachment #8598043 - Flags: approval-mozilla-aurora+
Depends on: 1160775
Attachment #8598043 - Attachment is obsolete: true
Attachment #8620022 - Flags: review+
You need to log in before you can comment on or make changes to this bug.