Closed
Bug 1323861
Opened 7 years ago
Closed 7 years ago
Reader Mode displays "Failed to load article from page" on all Breitbart articles
Categories
(Toolkit :: Reader Mode, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: smartfon.reddit, Assigned: evanxd)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0 Build ID: 20161215061212 Steps to reproduce: Open any Breitbart article after 2016-12-13 (+-2 days) Actual results: Reader Mode displays a horizontal line that would normally indicate a beginning or end of a paragraph. Below the line it says "Failed to load article from page". The rest of the screen is blank. No vertical menu panel on the right. Expected results: It should load the article and display the vertical menu panel on the right.
Reporter | ||
Comment 1•7 years ago
|
||
Updated•7 years ago
|
Component: Untriaged → Reader Mode
Product: Firefox → Toolkit
Reporter | ||
Updated•7 years ago
|
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Assignee | ||
Comment 2•7 years ago
|
||
I can reproduce the issue on http://www.breitbart.com/tech/2016/12/22/neutral-snopes-fact-checker-david-emery-un-angry-trump-supporters/ .
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 3•7 years ago
|
||
I think this can also fix the Issue 334 on GitHub Issues. [1]: https://github.com/mozilla/readability/issues/334
Assignee | ||
Comment 4•7 years ago
|
||
Found the root cause, which seems to be the JSDOMParser issue, and added tests[1]. When I run the test for the page[2], it will cause the below error. ``` JSDOMParser error: expected '</head>' and got </scrip JSDOMParser error: expected '</html>' and got </scrip Parsing this DOM caused errors: expected '</head>' and got </scrip expected '</html>' and got </scrip 1) "before all" hook 6 passing (2s) 1 failing 1) Test pages breitbart JSDOMParser "before all" hook: TypeError: Cannot read property 'documentElement' of null at Readability.parse (Readability.js:1878:25) at Context.<anonymous> (test/test-readability.js:65:27) ``` [1]: https://github.com/evanxd/readability/commit/26c9d349292e03cc582e9de3a4a099416a8189a6 [2]: http://www.breitbart.com/tech/2016/12/22/neutral-snopes-fact-checker-david-emery-un-angry-trump-supporters/
Assignee | ||
Comment 5•7 years ago
|
||
Updated tests: https://github.com/mozilla/readability/pull/345/commits/a8b53ba65a6bd5d08709c98d3f335d92ab982cd5
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → evan
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•7 years ago
|
||
Fixed test failures[1] and discussed the solution with reviewer. [1]: https://github.com/mozilla/readability/pull/345/commits/4d6a5b168d82c751284913c7417ec89822f540cf
Assignee | ||
Comment 7•7 years ago
|
||
Wrote a patch[1] to fix #334 and tests. And the CI is good. It's on review process... [1]: https://github.com/mozilla/readability/pull/345/commits/ec5c19c5be3afca48fe98f0b1628f461a8fb8f29
Assignee | ||
Comment 8•7 years ago
|
||
Updated the patch for review comments and fixed all jsdomparser tests[1]. [1]: https://github.com/mozilla/readability/pull/345/commits/e53afded5efa16ec9bce946e73c8ebb43ed6dfe8
Assignee | ||
Comment 9•7 years ago
|
||
Continue to fix the rest of readability test failures[1]. [1]: https://travis-ci.org/mozilla/readability/jobs/200230979
Assignee | ||
Comment 10•7 years ago
|
||
Fixed `ars-1` test: https://github.com/mozilla/readability/pull/345/commits/6d37453adbdf7b3e6e1308fa4be33fe10b9d24dc
Assignee | ||
Comment 11•7 years ago
|
||
Fixed all readability test failures[1]. [1]: https://github.com/mozilla/readability/pull/345/commits/a01a6e2bf10c8047bce76e7f0e992f4279844e3b
Assignee | ||
Comment 12•7 years ago
|
||
Replied the review comments[1]. [1]: https://github.com/mozilla/readability/pull/345#discussion_r101474042
Assignee | ||
Comment 13•7 years ago
|
||
Fixed testing failures by replacing `<` with < in an embedded script without CDATA[1] except `bbc-1`, `buzzfeed-1`, `herald-sun-1`, and `liberation-1` tests. Continue to work on the rest of failures tomorrow... [1]https://github.com/mozilla/readability/pull/345/commits/66c7f42a2700179b48db7665e4a9eb1b5a6a1d89
Assignee | ||
Comment 14•7 years ago
|
||
Updated the patch to replace `<` with `<` to pass all tests: https://github.com/mozilla/readability/pull/345/commits/ea166e07ef09d2e133e51e09b6e3b5708151bf9c Really took a lot of time to do that...
Assignee | ||
Comment 15•7 years ago
|
||
Updated the patch for review comments: https://github.com/mozilla/readability/pull/345/commits/b8252e4a9973b746effbdd10c5f375b9197b0570
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
Hi Gijs, Let's update readability from github repo. Could you take a look? Thanks.
Assignee | ||
Comment 18•7 years ago
|
||
Ensure the try[1] is good(it is running) before we land it. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d7583215f6c
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8840334 [details] Bug 1323861, Bug 1322674, Bug 1217007 - Update readability from github repo, https://reviewboard.mozilla.org/r/114842/#review116328
Attachment #8840334 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 20•7 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e04079f3f386 Bug 1322674, Bug 1217007 - Update readability from github repo, r=Gijs
Assignee | ||
Comment 21•7 years ago
|
||
Hi Wes, Could you help land the patch(Comment 20) into m-c? Thanks.
Flags: needinfo?(wkocher)
I'm already out for the night. Tomcat has a better chance of getting this before I do.
Flags: needinfo?(wkocher) → needinfo?(cbook)
Assignee | ||
Comment 23•7 years ago
|
||
Thanks for helping, Wes.
Comment 24•7 years ago
|
||
(In reply to Pulsebot from comment #20) > Pushed by gijskruitbosch@gmail.com: > https://hg.mozilla.org/integration/autoland/rev/e04079f3f386 > Bug 1322674, Bug 1217007 - Update readability from github repo, r=Gijs This has already landed on m-c but wasn't marked for whatever reason. https://hg.mozilla.org/mozilla-central/rev/e04079f3f386
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
status-firefox54:
--- → fixed
Updated•7 years ago
|
Flags: needinfo?(cbook)
Comment 25•7 years ago
|
||
(In reply to :Gijs from comment #24) > (In reply to Pulsebot from comment #20) > > Pushed by gijskruitbosch@gmail.com: > > https://hg.mozilla.org/integration/autoland/rev/e04079f3f386 > > Bug 1322674, Bug 1217007 - Update readability from github repo, r=Gijs > > This has already landed on m-c but wasn't marked for whatever reason. > > https://hg.mozilla.org/mozilla-central/rev/e04079f3f386 because of https://bugzilla.mozilla.org/show_bug.cgi?id=1115608#c7 - the autoland push is currently not displayed on treeherder so wasn't able to mark it :( sorry
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e04079f3f386
You need to log in
before you can comment on or make changes to this bug.
Description
•