299462 - function gatherTextUnderNode ( root ) is not well written

Status: RESOLVED DUPLICATE of bug 274486

(Firefox :: General, enhancement)

Description

[web]

Here my solution (it's about the gatherTextUnderNode function in
/content/browser/utilityOverlay.js

// Gather all descendent text under given document node.
function gatherTextUnderNode ( root ) 
{
	if(!root) return;
  var text = "";
  var node = root.firstChild;
  var depth = 1;
  while ( node  && depth > 0) {
    // See if this node is text.
    if ( node.nodeName == "#text" ) {
      // Add this text to our collection.
      text += " " + node.data;
    } else if ( node.nodeType == Node.ELEMENT_NODE 
                && node.localName.toUpperCase() == "IMG" ) {
      // If it has an alt= attribute, use that.
      var altText = node.getAttribute( "alt" );
      if ( altText && altText != "" ) {
        text += altText; //was text = altText;
    //    break; //Why should you use here break;?
      }
    }
    // Find next node to test.
    // First, see if this node has children.
    if ( node.hasChildNodes() ) {
      // Go to first child.
      node = node.firstChild;
      depth++;
    } else {
      // No children, try next sibling.
      if ( node.nextSibling ) {
        node = node.nextSibling;
      } else {
        // Last resort is our next oldest uncle/aunt. ---> No it's not! Maybe
it's the uncle/aunt of our granpa of our ...
		  while(node.parentNode)
        {
	        node = node.parentNode;
	        depth--;
	        if(node.nextSibling)
	        {
	        	node = node.nextSibling;
	        	break;
	        }
	     }
      }
    }
  }
  // Strip leading whitespace.
  text = text.replace( /^\s+/, "" );
  // Strip trailing whitespace.
  text = text.replace( /\s+$/, "" );
  // Compress remaining whitespace.
  text = text.replace( /\s+/g, " " );
  return text;
}

Comment 1

[:Gavin Sharp [email: [e-mail]]]

What are the advantages of your function? What was broken in the current
implementation? Have you checked to make sure all it's callers aren't broken
with your solution?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #0)
>     } else if ( node.nodeType == Node.ELEMENT_NODE 
>                 && node.localName.toUpperCase() == "IMG" ) {

It's not good to rely on localName tests, the current revision of the file uses
a much more reliable and secure instanceof check.

http://lxr.mozilla.org/mozilla/source/browser/base/content/utilityOverlay.js#328

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Marking WONTFIX, that function has since been changed and none of your changes seem required. Please feel free to re-open if you still think there is a problem worth addressing.