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

VERIFIED FIXED in Firefox 53

Status

()

Toolkit
Reader Mode
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: u583025, Assigned: aryx)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 verified)

Details

Attachments

(5 attachments)

(Reporter)

Description

2 years ago
str
Created attachment 8817126 [details]
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.
(Reporter)

Updated

2 years ago
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Comment 1

2 years ago
Created attachment 8817127 [details]
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
Blocks: 1102450
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

Comment 4

2 years ago
(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)
(Reporter)

Comment 5

2 years ago
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.
(Reporter)

Comment 6

2 years ago
Created attachment 8817343 [details]
Table in normal mode.png
(Reporter)

Comment 7

2 years ago
Created attachment 8817344 [details]
Table in Reader mode.png
Created attachment 8817594 [details] [review]
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)
(Reporter)

Comment 10

2 years ago
Thanks for directing me to the correct way, done - https://bugzilla.mozilla.org/show_bug.cgi?id=1322674
Flags: needinfo?(uncertainquark)

Updated

2 years ago
Attachment #8817594 - Flags: review?(gijskruitbosch+bugs)

Comment 11

2 years ago
(feedback's on github)
Committed as https://github.com/mozilla/readability/commit/5e9c7a39104093e1a50ce272fdad66900353f43e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Comment 13

2 years ago
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 → ---

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 15

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9cff6d389a
et al., update readability from github, rs=me

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d9cff6d389a
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox53: --- → fixed
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
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.