Closed Bug 1322327 Opened 5 years ago Closed 5 years ago

Reader mode doesn't show single image in div if it's not a descendant of a figure

Categories

(Toolkit :: Reader Mode, defect)

50 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox53 --- verified

People

(Reporter: u583025, Assigned: aryx)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Attached image Normal mode.png
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.
OS: Unspecified → All
Hardware: Unspecified → All
Attached image Reader mode.png
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.
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attached file pull request, v1
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.
Flags: needinfo?(aryx.bugmail)
Attachment #8817594 - Flags: review?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(uncertainquark)
Thanks for directing me to the correct way, done - https://bugzilla.mozilla.org/show_bug.cgi?id=1322674
Flags: needinfo?(uncertainquark)
Attachment #8817594 - Flags: review?(gijskruitbosch+bugs)
(feedback's on github)
Committed as https://github.com/mozilla/readability/commit/5e9c7a39104093e1a50ce272fdad66900353f43e
Status: ASSIGNED → RESOLVED
Closed: 5 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 → ---
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9cff6d389a
et al., update readability from github, rs=me
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/2d9cff6d389a
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
QA Whiteboard: [good first verify]
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.