1144441 - topCandidate code in Readability.js does silly loop manipulations which leave half the elements

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[:Gijs (he/him)]

Like bug 1127778:

      var topCandidate = topCandidates[0] || null;

      // If we still have no top candidate, just use the body as a last resort.
      // We also have to copy the body node so it is something we can modify.
      if (topCandidate === null || topCandidate.tagName === "BODY") {
        // Move all of the page's children into topCandidate
        topCandidate = doc.createElement("DIV");
        var children = page.childNodes;
        for (var i = 0; i < children.length; ++i) {
          this.log("Moving children:", children[i]);
          topCandidate.appendChild(children[i]);
        }

        page.appendChild(topCandidate);

        this._initializeNode(topCandidate);
      }

this loop is sad, because it will move out every other kid (and leave the others) because the appendChild call will remove the kid.

That's not good. A naive fix unfortunately didn't immediately fix everything, because we then created an extra div layer for the simple testcase from bug 1127778, which seems unnecessary?

Comment 1

[:Gijs (he/him)]

Attachment: Github PR

Comment 2

[:Margaret Leibovic]

Comment on attachment 8579067 [details] [review]
Github PR

Nice, merged.