Closed Bug 1322327 Opened 6 years ago Closed 6 years ago
Reader mode doesn't show single image in div if it's not a descendant of a figure
307.72 KB, image/png
178.33 KB, image/png
137.13 KB, image/png
113.04 KB, image/png
47 bytes, text/x-github-pull-request
|Details | Review|
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0 Build ID: 20161130094630 Firefox for Android Steps to reproduce: Open this link - https://android-developers.blogspot.in/2016/12/saving-data-reducing-the-size-of-app-updates-by-65-percent.html Click the reader mode button to open reader view Actual results: The GIF file in the article doesn't show up. Check attachments. Nor do embedded YouTube videos usually. Expected results: The GIF should have appeared in Reader mode and played.
Thank you for the report. This is a regression introduced in https://github.com/mozilla/readability/commit/f8d37e427627561870ce0ec599f434310180b2bc which kicks out divs which aren't a descendent of a figure. Updating > (img > p && !this._hasAncestorTag(node, "figure")) to > (img > 1 && img > p && !this._hasAncestorTag(node, "figure")) fixes it.
Assignee: nobody → aryx.bugmail
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → Reader Mode
Ever confirmed: true
Product: Firefox → Toolkit
Hi Gijs, can you provide information on two questions, please? 1. Running the tests shows 6 errors, all about jsdom not extracting the expected content, e.g. 3) Test pages bug-1255978 jsdom should extract expected content: AssertionError: expected 'p.(dnd-caption-wrapper)' to deeply equal 'div.(dnd-atom-rendered)' + expected - actual -p.(dnd-caption-wrapper) +div.(dnd-atom-rendered) at Assertion.assertEql (node_modules/chai/lib/chai/core/assertions.js:473:10) at Assertion.ctx.(anonymous function) [as eql] (node_modules/chai/lib/chai/utils/addMethod.js:40:25) at test/test-readability.js:101:32 at traverseDOM (test/test-readability.js:39:10) at Context.<anonymous> (test/test-readability.js:95:7) Known issue? 2. Running > node test/generate-testcase.js div-single-img-without-figure-keep https://android-developers.blogspot.in/2016/12/saving-data-reducing-the-size-of-app-updates-by-65-percent.html returns > No content generated by readability, not going to write expected.html! Any idea what I should do different? Thanks.
Summary: Reader mode doesn't support GIF files → Reader mode doesn't show single image in div if it's not a descendant of a figure
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #3) > Hi Gijs, can you provide information on two questions, please? > > 1. Running the tests shows 6 errors, all about jsdom not extracting the > expected content, e.g. I don't understand the question. You changed stuff and now the tests fail? Sounds like you broke something. Fix the code or fix the test. In any case, the code is on github, so a PR / code changes should live there. The tests pass on github tip, so if they don't for you when run on vanilla code, I dunno why that would be. > 2. Running > > node test/generate-testcase.js div-single-img-without-figure-keep https://android-developers.blogspot.in/2016/12/saving-data-reducing-the-size-of-app-updates-by-65-percent.html > returns > > No content generated by readability, not going to write expected.html! > Any idea what I should do different? Thanks. This means readability couldn't find anything to reader-ize. I don't know off-hand why, if it works when using it in the browser. One option is that the DOM isn't being parsed because it's not valid XHTML. You might need to update the source.html file (will be kept after running the line you copy/pasted) to be valid XHTML. Another option is that the actual content is being fetched via XHR, so just loading the main .html file doesn't have the content yet, you'd need to export it from the browser instead. Does that help clarify?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(aryx.bugmail)
On the same link (https://android-developers.blogspot.in/2016/12/saving-data-reducing-the-size-of-app-updates-by-65-percent.html), Reader mode doesn't display the table properly. The first column is missing.
The test failures mentioned earlier got shown because I didn't save the revert (doh). Saving the source.html to disk and using that for the test generation worked, but I dropped the test because existing ones already covered it.
(In reply to uncertainquark from comment #5) > On the same link > (https://android-developers.blogspot.in/2016/12/saving-data-reducing-the- > size-of-app-updates-by-65-percent.html), Reader mode doesn't display the > table properly. The first column is missing. Please file a new bug for this (one bug == one issue). Thank you.
Thanks for directing me to the correct way, done - https://bugzilla.mozilla.org/show_bug.cgi?id=1322674
(feedback's on github)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
We don't have an automated way of merging the github repo into m-c, so this should stay open until that change is merged to m-c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9cff6d389a et al., update readability from github, rs=me
I have reproduced this bug with Nightly 53.0a1 (2016-12-06) (64-bit) on Windows 8.1 , 64 Bit! This bug's fix is verified with latest Beta! Build ID : 20170320143328 User Agent : Mozilla/5.0 (WindowsNT 6.3; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0 [bugday-20170322]
I have reproduced this bug with Nightly 53.0a1 (2016-12-06) (64-bit) on Ubuntu 16.04.2 LTS 64-Bit! This bug's fix is verified wit latest Beta! Build ID : 20170330190824 User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0 [bugday-20170329]
You need to log in before you can comment on or make changes to this bug.