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

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[u583025]

Attachment: 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.

Comment 1

[u583025]

Attachment: Reader mode.png

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 4

[:Gijs (out for now; he/him)]

(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?

Comment 5

[u583025]

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.

Comment 6

[u583025]

Attachment: Table in normal mode.png

Comment 7

[u583025]

Attachment: Table in Reader mode.png

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: 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.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(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.

Comment 10

[u583025]

Thanks for directing me to the correct way, done - https://bugzilla.mozilla.org/show_bug.cgi?id=1322674

Comment 11

[:Gijs (out for now; he/him)]

(feedback's on github)

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Committed as https://github.com/mozilla/readability/commit/5e9c7a39104093e1a50ce272fdad66900353f43e

Comment 13

[:Gijs (out for now; he/him)]

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.

Comment 14

[:Gijs (out for now; he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=44e82702577f92b638bd9c5b919cf3581eea9ab4

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9cff6d389a
et al., update readability from github, rs=me

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2d9cff6d389a

Comment 17

[Mohammad Maruf Rahman]

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]

Comment 18

[Mohammad Maruf Rahman]

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]