Improve Autofill heuristics when detecting labels from previous elements.

chrome/renderer/autofill/form_manager.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/autofill/form_manager.h"
 
 #include "base/logging.h"
 #include "base/memory/scoped_vector.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
@@ skipping 60 matching lines @@
   if (!element)
     return false;
 
   return element->isTextField() && !element->isPasswordField();
 }
 
 bool IsSelectElement(const WebFormControlElement& element) {
   return element.formControlType() == ASCIIToUTF16("select-one");
 }
 
-bool IsTextContainerElement(const WebElement& element) {
-  return
-      element.hasTagName("p") ||
-      element.hasTagName("b") ||
-      element.hasTagName("span") ||
-      element.hasTagName("font");
-}
-
 bool IsOptionElement(const WebElement& element) {
   return element.hasTagName("option");
 }
 
 bool IsScriptElement(const WebElement& element) {
   return element.hasTagName("script");
 }
 
 bool IsNoScriptElement(const WebElement& element) {
   return element.hasTagName("noscript");
 }
 
 bool HasTagName(const WebNode& node, const WebKit::WebString& tag) {
   return node.isElementNode() && node.toConst<WebElement>().hasTagName(tag);
 }
 
 bool IsAutofillableElement(const WebFormControlElement& element) {
   const WebInputElement* input_element = toWebInputElement(&element);
   return IsTextInput(input_element) || IsSelectElement(element);
 }
 
+// Appends |suffix| to |prefix| so that any intermediary whitespace is collapsed
+// to a single space.  If |force_whitespace| is true, then the resulting string
+// is guaranteed to have a space between |prefix| and |suffix|.  Otherwise, the
+// result includes a space only if |prefix| has trailing whitespace or |suffix|
+// has leading whitespace.

[dhollowa, 2011/08/01 22:16:54]

Some examples in this comment would help the casual reader.  The comment as it
stands is on par with directly reading the code.  ;-)

[Ilya Sherman, 2011/08/01 23:59:36]

Done.

+const string16 CombineAndCollapseWhitespace(const string16& prefix,
+                                            const string16& suffix,
+                                            bool force_whitespace) {
+  string16 prefix_trimmed;
+  TrimPositions prefix_trailing_whitespace =
+      TrimWhitespace(prefix, TRIM_TRAILING, &prefix_trimmed);
+
+  // Recursively compute the children's text.
+  string16 suffix_trimmed;
+  TrimPositions suffix_leading_whitespace =
+      TrimWhitespace(suffix, TRIM_LEADING, &suffix_trimmed);
+
+  if (prefix_trailing_whitespace || suffix_leading_whitespace ||
+      force_whitespace) {
+    return prefix_trimmed + ASCIIToUTF16(" ") + suffix_trimmed;
+  } else {
+    return prefix_trimmed + suffix_trimmed;
+  }
+}
+
 // This is a helper function for the FindChildText() function (see below).
 // Search depth is limited with the |depth| parameter.
 string16 FindChildTextInner(const WebNode& node, int depth) {
   if (depth <= 0 || node.isNull())
     return string16();
 
   // Skip over comments.
   if (node.nodeType() == WebNode::CommentNode)
     return FindChildTextInner(node.nextSibling(), depth - 1);
 
   if (node.nodeType() != WebNode::ElementNode &&
       node.nodeType() != WebNode::TextNode)
     return string16();
 
   // Ignore elements known not to contain inferable labels.
   if (node.isElementNode()) {
     const WebElement element = node.toConst<WebElement>();
     if (IsOptionElement(element) ||
         IsScriptElement(element) ||
-        IsNoScriptElement(element)) {
+        IsNoScriptElement(element) ||
+        (element.isFormControlElement() &&
+         IsAutofillableElement(element.toConst<WebFormControlElement>()))) {
       return string16();
     }
   }
 
   // Extract the text exactly at this node.
   string16 node_text = node.nodeValue();
-  TrimPositions node_trailing_whitespace =
-      TrimWhitespace(node_text, TRIM_TRAILING, &node_text);
 
   // Recursively compute the children's text.
   // Preserve inter-element whitespace separation.
   string16 child_text = FindChildTextInner(node.firstChild(), depth - 1);
-  TrimPositions child_leading_whitespace =
-      TrimWhitespace(child_text, TRIM_LEADING, &child_text);
-  if (node_trailing_whitespace || child_leading_whitespace ||
-      (node.nodeType() == WebNode::TextNode && node_text.empty())) {
-    node_text += ASCIIToUTF16(" ");
-  }
-  node_text += child_text;
-  node_trailing_whitespace =
-      TrimWhitespace(node_text, TRIM_TRAILING, &node_text);
+  bool add_space = node.nodeType() == WebNode::TextNode && node_text.empty();
+  node_text = CombineAndCollapseWhitespace(node_text, child_text, add_space);
 
   // Recursively compute the siblings' text.
   // Again, preserve inter-element whitespace separation.
   string16 sibling_text = FindChildTextInner(node.nextSibling(), depth - 1);
-  TrimPositions sibling_leading_whitespace =
-      TrimWhitespace(sibling_text, TRIM_LEADING, &sibling_text);
-  if (node_trailing_whitespace || sibling_leading_whitespace ||
-      (node.nodeType() == WebNode::TextNode && node_text.empty())) {
-    node_text += ASCIIToUTF16(" ");
-  }
-  node_text += sibling_text;
+  add_space = node.nodeType() == WebNode::TextNode && node_text.empty();
+  node_text = CombineAndCollapseWhitespace(node_text, sibling_text, add_space);
 
   return node_text;
 }
 
 // Returns the aggregated values of the descendants of |element| that are
 // non-empty text nodes.  This is a faster alternative to |innerText()| for
 // performance critical operations.  It does a full depth-first search so can be
 // used when the structure is not directly known.  However, unlike with
 // |innerText()|, the search depth and breadth are limited to a fixed threshold.
 // Whitespace is trimmed from text accumulated at descendant nodes.
-string16 FindChildText(const WebElement& element) {
-  WebNode child = element.firstChild();
+string16 FindChildText(const WebNode& node) {
+  if (node.isTextNode())
+    return node.nodeValue();
+
+  WebNode child = node.firstChild();
 
   const int kChildSearchDepth = 10;
-  string16 element_text = FindChildTextInner(child, kChildSearchDepth);
-  TrimWhitespace(element_text, TRIM_ALL, &element_text);
-  return element_text;
+  string16 node_text = FindChildTextInner(child, kChildSearchDepth);
+  TrimWhitespace(node_text, TRIM_ALL, &node_text);
+  return node_text;
 }
 
 // Helper for |InferLabelForElement()| that infers a label, if possible, from
-// a previous sibling of |element|.
+// a previous sibling of |element|,
+// e.g. Some Text <input ...>
+// or   Some <span>Text</span> <input ...>
+// or   <p>Some Text</p><input ...>
+// or   <label>Some Text</label> <input ...>
+// or   Some Text <img><input ...>
+// or   <b>Some Text</b><br/> <input ...>.
 string16 InferLabelFromPrevious(const WebFormControlElement& element) {
   string16 inferred_label;
-  WebNode previous = element.previousSibling();
-  if (previous.isNull())
-    return string16();
+  WebNode previous = element;
+  while (true) {
+    previous = previous.previousSibling();
+    if (previous.isNull())
+      break;
 
-  // Check for text immediately before the |element|.
-  if (previous.isTextNode()) {
-    inferred_label = previous.nodeValue();
-    TrimWhitespace(inferred_label, TRIM_ALL, &inferred_label);
-  }
+    WebNode::NodeType node_type = previous.nodeType();
+    if (node_type != WebNode::TextNode &&
+        node_type != WebNode::ElementNode &&
+        node_type != WebNode::CommentNode)
+      break;
 
-  // If we didn't find text, check for an immediately preceding text container,
-  // e.g. <p>Some Text</p><input ...>
-  // Note the lack of whitespace between <p> and <input> elements.
-  if (inferred_label.empty() && previous.isElementNode()) {
-    WebElement previous_element = previous.to<WebElement>();
-    if (IsTextContainerElement(previous_element))
-      inferred_label = FindChildText(previous_element);
-  }
+    // Coalesce any text contained in multiple consecutive plain text nodes or
+    // on of a few HTML elements that are essentially equivalent to text nodes.
+    if (previous.isTextNode() ||
+        HasTagName(previous, "b") || HasTagName(previous, "strong") ||
+        HasTagName(previous, "span") || HasTagName(previous, "font")) {
+      string16 value = FindChildText(previous);
+      // A text node's value will be empty if it is for a line break.
+      bool add_space = previous.isTextNode() && value.empty();
+      inferred_label =
+          CombineAndCollapseWhitespace(value, inferred_label, add_space);
+    } else if (previous.isElementNode()) {

[dhollowa, 2011/08/01 22:16:54]

Instead of "else if", it would be more readable IMO if you pulled the |break|
into subsequent "if" blocks.  More like the original code.  And then pull the
"All other elements are..." comment up above the second "if".
 

[Ilya Sherman, 2011/08/01 23:59:36]

Done.

+      // All other elements are only allowed if we have not yet found any
+      // candidate label text.
+      string16 trimmed_label;
+      TrimWhitespace(inferred_label, TRIM_ALL, &trimmed_label);
+      if (trimmed_label.empty()) {
+        // <img> and <br> tags often appear between the input element and its
+        // label text, so skip over them.
+        if (HasTagName(previous, "img") || HasTagName(previous, "br"))
+          continue;
 
-  // If we didn't find one immediately preceding, check for a text container
-  // separated from this node only by whitespace,
-  // e.g. <p>Some Text</p>   <input ...>
-  // Note the whitespace between <p> and <input> elements.
-  if (inferred_label.empty() && previous.isTextNode()) {
-    WebNode sibling = previous.previousSibling();
-    if (!sibling.isNull() && sibling.isElementNode()) {
-      WebElement previous_element = sibling.to<WebElement>();
-      if (IsTextContainerElement(previous_element))
-        inferred_label = FindChildText(previous_element);
+        // We expect <p> and <label> tags to contain the full label text, so
+        // only allow these if we have not yet found any candidate label text.
+        if (HasTagName(previous, "p") || HasTagName(previous, "label"))
+          inferred_label = FindChildText(previous);
+      }
+
+      break;
     }
   }
 
-  // Look for a text node prior to <img> or <br> tags,
-  // e.g. Some Text<img/><input ...> or Some Text<br/><input ...>
-  while (inferred_label.empty() && !previous.isNull()) {
-    if (previous.isTextNode()) {
-      inferred_label = previous.nodeValue();
-      TrimWhitespace(inferred_label, TRIM_ALL, &inferred_label);
-    } else if (previous.isElementNode()) {
-      WebElement previous_element = previous.to<WebElement>();
-      if (IsTextContainerElement(previous_element))
-        inferred_label = FindChildText(previous_element);
-      else if (!HasTagName(previous, "img") && !HasTagName(previous, "br"))
-        break;
-    } else {
-      break;
-    }
-
-    previous = previous.previousSibling();
-  }
-
-  // Look for a label node prior to the <input> tag,
-  // e.g. <label>Some Text</label><input ...>
-  while (inferred_label.empty() && !previous.isNull()) {
-    if (previous.isTextNode()) {
-      inferred_label = previous.nodeValue();
-      TrimWhitespace(inferred_label, TRIM_ALL, &inferred_label);
-    } else if (HasTagName(previous, "label")) {
-      inferred_label = FindChildText(previous.to<WebElement>());
-    } else {
-      break;
-    }
-
-    previous = previous.previousSibling();
-  }
-
+  TrimWhitespace(inferred_label, TRIM_ALL, &inferred_label);
   return inferred_label;
 }
 
 // Helper for |InferLabelForElement()| that infers a label, if possible, from
 // enclosing list item,
 // e.g. <li>Some Text<input ...><input ...><input ...></tr>
 string16 InferLabelFromListItem(const WebFormControlElement& element) {
   WebNode parent = element.parentNode();
   while (!parent.isNull() && parent.isElementNode() &&
          !parent.to<WebElement>().hasTagName("li")) {
     parent = parent.parentNode();
   }
 
   if (!parent.isNull() && HasTagName(parent, "li"))
-    return FindChildText(parent.to<WebElement>());
+    return FindChildText(parent);
 
   return string16();
 }
 
 // Helper for |InferLabelForElement()| that infers a label, if possible, from
 // surrounding table structure,
 // e.g. <tr><td>Some Text</td><td><input ...></td></tr>
 // or   <tr><th>Some Text</th><td><input ...></td></tr>
 // or   <tr><td><b>Some Text</b></td><td><b><input ...></b></td></tr>
 // or   <tr><th><b>Some Text</b></th><td><b><input ...></b></td></tr>
 string16 InferLabelFromTableColumn(const WebFormControlElement& element) {
   WebNode parent = element.parentNode();
   while (!parent.isNull() && parent.isElementNode() &&
          !parent.to<WebElement>().hasTagName("td")) {
     parent = parent.parentNode();
   }
 
   if (parent.isNull())
     return string16();
 
   // Check all previous siblings, skipping non-element nodes, until we find a
   // non-empty text block.
   string16 inferred_label;
   WebNode previous = parent.previousSibling();
   while (inferred_label.empty() && !previous.isNull()) {
     if (HasTagName(previous, "td") || HasTagName(previous, "th"))
-      inferred_label = FindChildText(previous.to<WebElement>());
+      inferred_label = FindChildText(previous);
 
     previous = previous.previousSibling();
   }
 
   return inferred_label;
 }
 
 // Helper for |InferLabelForElement()| that infers a label, if possible, from
 // surrounding table structure,
 // e.g. <tr><td>Some Text</td></tr><tr><td><input ...></td></tr>
 string16 InferLabelFromTableRow(const WebFormControlElement& element) {
   WebNode parent = element.parentNode();
   while (!parent.isNull() && parent.isElementNode() &&
          !parent.to<WebElement>().hasTagName("tr")) {
     parent = parent.parentNode();
   }
 
   if (parent.isNull())
     return string16();
 
   // Check all previous siblings, skipping non-element nodes, until we find a
   // non-empty text block.
   string16 inferred_label;
   WebNode previous = parent.previousSibling();
   while (inferred_label.empty() && !previous.isNull()) {
     if (HasTagName(previous, "tr"))
-      inferred_label = FindChildText(previous.to<WebElement>());
+      inferred_label = FindChildText(previous);
 
     previous = previous.previousSibling();
   }
 
   return inferred_label;
 }
 
 // Helper for |InferLabelForElement()| that infers a label, if possible, from
 // a surrounding div table,
 // e.g. <div>Some Text<span><input ...></span></div>
 // e.g. <div>Some Text</div><div><input ...></div>
 string16 InferLabelFromDivTable(const WebFormControlElement& element) {
   WebNode node = element.parentNode();
-  while (!node.isNull() && node.isElementNode() &&
-         !node.to<WebElement>().hasTagName("div") &&
-         // If the element is in a table, its label most likely is too.
-         !node.to<WebElement>().hasTagName("table")) {
-    node = node.parentNode();
-  }
+  bool looking_for_parent = true;
 
-  if (node.isNull() || !HasTagName(node, "div"))
-    return string16();
-
-  // Search the siblings while we cannot find label.
+  // Search the sibling and parent <div>s until we find a candidate label.
   string16 inferred_label;
   while (inferred_label.empty() && !node.isNull()) {
-    if (HasTagName(node, "div"))
-      inferred_label = FindChildText(node.to<WebElement>());
+    if (HasTagName(node, "div")) {
+      looking_for_parent = false;
+      inferred_label = FindChildText(node);
+    } else if (looking_for_parent &&
+               (HasTagName(node, "table") || HasTagName(node, "fieldset"))) {
+      // If the element is in a table or fieldset, its label most likely is too.
+      break;
+    }
 
-    node = node.previousSibling();
+    if (node.previousSibling().isNull()) {
+      // If there are no more siblings, continue walking up the tree.
+      looking_for_parent = true;
+    }
+
+    if (looking_for_parent)
+      node = node.parentNode();
+    else
+      node = node.previousSibling();
   }
 
   return inferred_label;
 }
 
 // Helper for |InferLabelForElement()| that infers a label, if possible, from
 // a surrounding definition list,
 // e.g. <dl><dt>Some Text</dt><dd><input ...></dd></dl>
 // e.g. <dl><dt><b>Some Text</b></dt><dd><b><input ...></b></dd></dl>
 string16 InferLabelFromDefinitionList(const WebFormControlElement& element) {
   WebNode parent = element.parentNode();
   while (!parent.isNull() && parent.isElementNode() &&
          !parent.to<WebElement>().hasTagName("dd"))
     parent = parent.parentNode();
 
   if (parent.isNull() || !HasTagName(parent, "dd"))
     return string16();
 
   // Skip by any intervening text nodes.
   WebNode previous = parent.previousSibling();
   while (!previous.isNull() && previous.isTextNode())
     previous = previous.previousSibling();
 
   if (previous.isNull() || !HasTagName(previous, "dt"))
     return string16();
 
-  return FindChildText(previous.to<WebElement>());
+  return FindChildText(previous);
 }
 
 // Infers corresponding label for |element| from surrounding context in the DOM,
 // e.g. the contents of the preceding <p> tag or text element.
 string16 InferLabelForElement(const WebFormControlElement& element) {
   string16 inferred_label = InferLabelFromPrevious(element);
   if (!inferred_label.empty())
     return inferred_label;
 
   // If we didn't find a label, check for list item case.
@@ skipping 251 matching lines @@
 
 // static
 string16 FormManager::LabelForElement(const WebFormControlElement& element) {
   // Don't scrape labels for elements we can't possibly autofill anyway.
   if (!IsAutofillableElement(element))
     return string16();
 
   WebNodeList labels = element.document().getElementsByTagName("label");
   for (unsigned i = 0; i < labels.length(); ++i) {
     WebLabelElement label = labels.item(i).to<WebLabelElement>();
-    DCHECK(label.hasTagName("label"));
-    if (label.correspondingControl() == element)
+    WebElement corresponding_control = label.correspondingControl();
+    if (corresponding_control == element ||
+        (corresponding_control.isNull() &&
+         label.getAttribute("for") == element.nameForAutofill())) {
       return FindChildText(label);
+    }
   }
 
   // Infer the label from context if not found in label element.
   return InferLabelForElement(element);
 }
 
 // static
 bool FormManager::WebFormElementToFormData(const WebFormElement& element,
                                            RequirementsMask requirements,
                                            ExtractMask extract_mask,
@@ skipping 64 matching lines @@
   // Loop through the label elements inside the form element.  For each label
   // element, get the corresponding form control element, use the form control
   // element's name as a key into the <name, FormField> map to find the
   // previously created FormField and set the FormField's label to the
   // label.firstChild().nodeValue() of the label element.
   WebNodeList labels = element.getElementsByTagName("label");
   for (unsigned i = 0; i < labels.length(); ++i) {
     WebLabelElement label = labels.item(i).to<WebLabelElement>();
     WebFormControlElement field_element =
         label.correspondingControl().to<WebFormControlElement>();
-    if (field_element.isNull() ||
+
+    string16 element_name;
+    if (field_element.isNull()) {
+      // Sometimes site authors will incorrectly specify the corresponding
+      // field element's name rather than its id, so we compensate here.
+      element_name = label.getAttribute("for");
+    } else if (
         !field_element.isFormControlElement() ||
-        field_element.formControlType() == WebString::fromUTF8("hidden"))
+        field_element.formControlType() == WebString::fromUTF8("hidden")) {
       continue;
+    } else {
+      element_name = field_element.nameForAutofill();
+    }
 
-    std::map<string16, FormField*>::iterator iter =
-        name_map.find(field_element.nameForAutofill());
+    std::map<string16, FormField*>::iterator iter = name_map.find(element_name);
     // Concatenate labels because some sites might have multiple label
     // candidates.
     if (iter != name_map.end())
       iter->second->label += FindChildText(label);
   }
 
   // Loop through the form control elements, extracting the label text from
   // the DOM.  We use the |fields_extracted| vector to make sure we assign the
   // extracted label to the correct field, as it's possible |form_fields| will
   // not contain all of the elements in |control_elements|.
@@ skipping 317 matching lines @@
     if (element_name == form.name && action == form.action) {
       *form_element = *form_iter;
       return true;
     }
   }
 
   return false;
 }
 
 }  // namespace autofill