Fixed Lists of Images Not Showing #738

Tuxy222 · 2022-03-09T18:06:42Z

This change allows for lists of images within the content of the page to be present in reader mode.

Related to: #697

niklasbaumgardner

I just have a few things to address. Thanks for working on this!

niklasbaumgardner · 2022-03-15T14:36:04Z

Readability.js

@@ -2109,6 +2109,7 @@ Readability.prototype = {
        var embedCount = 0;
        var embeds = this._getAllNodesWithTag(node, ["object", "embed", "iframe"]);

+


nit: We can remove this empty line

niklasbaumgardner · 2022-03-15T14:44:52Z

Readability.js

+              }
+            }
+            // Allow lists of images to remain, so long as img count doesn't exceed li count
+            if (img > 1 && img <= (li+100)) {


Why do we add 100 to li here?

niklasbaumgardner · 2022-03-15T14:44:52Z

Readability.js

+              }
+            }
+            // Allow lists of images to remain, so long as img count doesn't exceed li count
+            if (img > 1 && img <= (li+100)) {


Why do we add 100 to li here?

niklasbaumgardner · 2022-03-15T14:46:58Z

Readability.js

@@ -2136,6 +2137,29 @@ Readability.prototype = {
          (!isList && weight < 25 && linkDensity > 0.2) ||
          (weight >= 25 && linkDensity > 0.5) ||
          ((embedCount === 1 && contentLength < 75) || embedCount > 1);
+
+        if (haveToRemove) {
+          // Check for lists of images


Instead of having comments on each line, could we have one comment at the beginning explaining why we are doing this check?

niklasbaumgardner · 2022-03-15T14:46:58Z

Readability.js

@@ -2136,6 +2137,29 @@ Readability.prototype = {
          (!isList && weight < 25 && linkDensity > 0.2) ||
          (weight >= 25 && linkDensity > 0.5) ||
          ((embedCount === 1 && contentLength < 75) || embedCount > 1);
+
+        if (haveToRemove) {
+          // Check for lists of images


Instead of having comments on each line, could we have one comment at the beginning explaining why we are doing this check?

tigleym · 2022-03-14T17:19:29Z

Readability.js

@@ -2109,6 +2109,7 @@ Readability.prototype = {
        var embedCount = 0;
        var embeds = this._getAllNodesWithTag(node, ["object", "embed", "iframe"]);

+


Remove this extra newline

tigleym · 2022-03-23T19:56:20Z

Readability.js

+              if (node.children[x].localName == "li") {
+                if (node.children[x].children.length > 1) {
+                  return true;
+                }
+              } else {
+                return true;
+              }
+            }
+            li_count = node.getElementsByTagName("li").length;
+            if (img > 1 && img <= (li_count)) {
+              return false;
+            }


Suggested change

if (node.children[x].localName == "li") {

if (node.children[x].children.length > 1) {

return true;

}

} else {

return true;

}

}

li_count = node.getElementsByTagName("li").length;

if (img > 1 && img <= (li_count)) {

return false;

}

let child = node.children[x];

if (child.localName == "li" && child.children.length) {

return true;

}

li_count = node.getElementsByTagName("li").length;

if (img > 1 && img <= (li_count)) {

return false;

}

I think it would be a good idea to document here why we remove some images for this specific case. It's not immediately clear to me here.

tigleym · 2022-03-23T19:57:39Z

Readability.js

+              if (node.children[x].localName == "li") {
+                if (node.children[x].children.length > 1) {
+                  return true;
+                }
+              } else {
+                return true;
+              }
+            }
+            li_count = node.getElementsByTagName("li").length;
+            if (img > 1 && img <= (li_count)) {
+              return false;
+            }


I think it would be a good idea to document here why we remove some images for this specific case. It's not immediately clear to me here.

tigleym · 2022-03-25T16:58:17Z

It looks like we have one failing test here. Could you investigate it?

tigleym · 2022-03-30T15:22:47Z

Nice! Thanks @Tuxy222 . Do you think we can create a test case for this?

niklasbaumgardner

I just have a nit to fix. Otherwise, I think this is good

Readability.js

Co-authored-by: niklasbaumgardner <63427453+niklasbaumgardner@users.noreply.github.com>

tigleym

Thank you!

tholu · 2022-11-18T09:49:31Z

Will this be merged? Or why not?

gijsk · 2022-11-18T10:30:21Z

I think this just got lost. I just merged it.

gijsk · 2022-11-18T10:30:32Z

(Thanks for the ping!)

Tuxy222 added 3 commits March 3, 2022 13:10

Allowed lists of images to appear in ReaderMode

9458c38

Allowed for certain <ul> lists of images to pass the "Fishy" algoirthm

dde2df0

Allowed for certain <ul> lists of images to pass the "Fishy" algoirthm

6bd7d3e

tigleym requested review from tigleym and niklasbaumgardner March 10, 2022 16:47

niklasbaumgardner suggested changes Mar 15, 2022

View reviewed changes

tigleym reviewed Mar 15, 2022

View reviewed changes

Tuxy222 added 2 commits March 16, 2022 16:17

Allow for <ul> lists of images to remain

0e07f27

Allow for <ul> lists of images to remain

029245c

Tuxy222 requested review from niklasbaumgardner and tigleym March 16, 2022 20:24

tigleym reviewed Mar 23, 2022

View reviewed changes

tigleym suggested changes Mar 23, 2022

View reviewed changes

Allow for <ul> lists of images to remain

9b1f4b9

Allowed for certain lists of images to bypass the Fishy algoirthm

d1ad46e

Added test for list of images bug

e8c115c

Tuxy222 requested a review from tigleym April 15, 2022 00:37

niklasbaumgardner suggested changes Apr 20, 2022

View reviewed changes

Readability.js Outdated Show resolved Hide resolved

Readability.js Outdated Show resolved Hide resolved

Update Readability.js

1c9f818

Co-authored-by: niklasbaumgardner <63427453+niklasbaumgardner@users.noreply.github.com>

tigleym approved these changes Apr 21, 2022

View reviewed changes

niklasbaumgardner approved these changes May 4, 2022

View reviewed changes

gijsk merged commit 7c7f694 into mozilla:master Nov 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Lists of Images Not Showing #738

Fixed Lists of Images Not Showing #738

Tuxy222 commented Mar 9, 2022

niklasbaumgardner left a comment

niklasbaumgardner Mar 15, 2022

niklasbaumgardner Mar 15, 2022

niklasbaumgardner Mar 15, 2022

niklasbaumgardner Mar 15, 2022

niklasbaumgardner Mar 15, 2022

tigleym Mar 14, 2022

tigleym Mar 23, 2022

tigleym Mar 23, 2022

tigleym Mar 23, 2022

tigleym commented Mar 25, 2022

tigleym commented Mar 30, 2022

niklasbaumgardner left a comment

tigleym left a comment

tholu commented Nov 18, 2022

gijsk commented Nov 18, 2022

gijsk commented Nov 18, 2022

		@@ -2109,6 +2109,7 @@ Readability.prototype = {
		var embedCount = 0;
		var embeds = this._getAllNodesWithTag(node, ["object", "embed", "iframe"]);

Fixed Lists of Images Not Showing #738

Fixed Lists of Images Not Showing #738

Conversation

Tuxy222 commented Mar 9, 2022

niklasbaumgardner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigleym commented Mar 25, 2022

tigleym commented Mar 30, 2022

niklasbaumgardner left a comment

Choose a reason for hiding this comment

tigleym left a comment

Choose a reason for hiding this comment

tholu commented Nov 18, 2022

gijsk commented Nov 18, 2022

gijsk commented Nov 18, 2022