Closed Bug 1144441 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Iteration:
39.2 - 23 Mar

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
Margaret
: review+
Details | Review
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?
Blocks: 792366
Attached file Github PR
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #8579067 - Flags: review?(margaret.leibovic)
Iteration: --- → 39.2 - 23 Mar
Points: --- → 2
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
Blocks: 1132074
Comment on attachment 8579067 [details] [review]
Github PR

Nice, merged.
Attachment #8579067 - Flags: review?(margaret.leibovic) → review+
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.