AutoFill: Parse HTMLLabel elements for form control element labels.

AutoFill: Parse HTMLLabel elements for form control element labels.

BUG=40306
TEST=FormManagerTest.Labels

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43761

[dhollowa, 2010-04-06 02:11:50]

http://codereview.chromium.org/1622004/diff/1/2
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/1622004/diff/1/2#newcode136
chrome/renderer/form_manager.cc:136: name_map[field->name()] = field;
field->name() is not guaranteed to be unique.  It also may be an empty string. 
I'm thinking there are a few important cases to consider:
  (1) When no 'name' or 'id' attributes are set in the <input> element.
  (2) When we violate uniqueness of 'name' attributes.  I.e. two <inputs> have
the same 'name' attribute.
  (3) When we violate the uniqueness of 'id' attributes.  I.e. two <inputs> have
the same 'id'.
  (4) When the 'name' of one <input> element is the same as the 'id' of a
different <input> element's.

I would think that we would only want to add to the map when we have an 'id' for
the field.  (See comments below.)

http://codereview.chromium.org/1622004/diff/1/2#newcode159
chrome/renderer/form_manager.cc:159:
name_map.find(field_element.nameForAutofill());
The 'for' attribute on a <label> should only match the 'id' of the <input> not
the 'name' attribute, which is possible if we use the |nameForAutofill()|
string.

[James Hawkins, 2010-04-06 03:39:09]

http://codereview.chromium.org/1622004/diff/1/2
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/1622004/diff/1/2#newcode136
chrome/renderer/form_manager.cc:136: name_map[field->name()] = field;
On 2010/04/06 02:11:51, dhollowa wrote:
> field->name() is not guaranteed to be unique.  It also may be an empty string.

> I'm thinking there are a few important cases to consider:
>   (1) When no 'name' or 'id' attributes are set in the <input> element.
>   (2) When we violate uniqueness of 'name' attributes.  I.e. two <inputs> have
> the same 'name' attribute.
>   (3) When we violate the uniqueness of 'id' attributes.  I.e. two <inputs>
have
> the same 'id'.
>   (4) When the 'name' of one <input> element is the same as the 'id' of a
> different <input> element's.
> 
> I would think that we would only want to add to the map when we have an 'id'
for
> the field.  (See comments below.)

Long story short, we can't handle pretend to handle invalid forms.  The worst
case we're considering here is when a form control element has an empty
nameForAutofill() (no id and no name).  In this case,
label.correspondingControl() will be NULL (isNull() technically) and we don't
add the label (because there can be no label for this form control element.) 
I've added a test case to show this, FormManagerTest.InvalidLabels.

http://codereview.chromium.org/1622004/diff/1/2#newcode159
chrome/renderer/form_manager.cc:159:
name_map.find(field_element.nameForAutofill());
On 2010/04/06 02:11:51, dhollowa wrote:
> The 'for' attribute on a <label> should only match the 'id' of the <input> not
> the 'name' attribute, which is possible if we use the |nameForAutofill()|
> string.

Well we don't have an id() method for elements, so we use nameForAutofill() :-) 
If there is an id, nameForAutofill() returns the id.  If there's not an id,
nameForAutofill() returns the name.  In the latter case, correspondingElement()
will be isNull() anyway, so the point is moot.

[dhollowa, 2010-04-06 04:33:18]

http://codereview.chromium.org/1622004/diff/1/2
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/1622004/diff/1/2#newcode136
chrome/renderer/form_manager.cc:136: name_map[field->name()] = field;
On 2010/04/06 03:39:09, James Hawkins wrote:
> On 2010/04/06 02:11:51, dhollowa wrote:
> > field->name() is not guaranteed to be unique.  It also may be an empty
string.
> 
> > I'm thinking there are a few important cases to consider:
> >   (1) When no 'name' or 'id' attributes are set in the <input> element.
> >   (2) When we violate uniqueness of 'name' attributes.  I.e. two <inputs>
have
> > the same 'name' attribute.
> >   (3) When we violate the uniqueness of 'id' attributes.  I.e. two <inputs>
> have
> > the same 'id'.
> >   (4) When the 'name' of one <input> element is the same as the 'id' of a
> > different <input> element's.
> > 
> > I would think that we would only want to add to the map when we have an 'id'
> for
> > the field.  (See comments below.)
> 
> Long story short, we can't handle pretend to handle invalid forms.  The worst
> case we're considering here is when a form control element has an empty
> nameForAutofill() (no id and no name).  In this case,
> label.correspondingControl() will be NULL (isNull() technically) and we don't
> add the label (because there can be no label for this form control element.) 
> I've added a test case to show this, FormManagerTest.InvalidLabels.

Agreed.
We can rule out case (1) because no label elements can refer to it.
We can rule out cases (2) and (3) because it is really invalid input.
But case (4) is valid input that I believe we may generate incorrect results
for.  For example:
  <form>
    <label for='a'>a</label><input id='a' name='b' .../>
    <label for='b'>b</label><input id='b' name='a' .../>
    ...
  </form>

Will, I believe, swap labels "a" and "b".

http://codereview.chromium.org/1622004/diff/1/2#newcode159
chrome/renderer/form_manager.cc:159:
name_map.find(field_element.nameForAutofill());
On 2010/04/06 03:39:09, James Hawkins wrote:
> On 2010/04/06 02:11:51, dhollowa wrote:
> > The 'for' attribute on a <label> should only match the 'id' of the <input>
not
> > the 'name' attribute, which is possible if we use the |nameForAutofill()|
> > string.
> 
> Well we don't have an id() method for elements, so we use nameForAutofill()
:-) 
> If there is an id, nameForAutofill() returns the id.  If there's not an id,
> nameForAutofill() returns the name.  In the latter case,
correspondingElement()
> will be isNull() anyway, so the point is moot.

From my read of the code (WebInputElement.cpp:100) it looks like 'name' is
preferred over 'id'.  Hence the bug.

[James Hawkins, 2010-04-06 04:49:57]

http://codereview.chromium.org/1622004/diff/1/2
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/1622004/diff/1/2#newcode136
chrome/renderer/form_manager.cc:136: name_map[field->name()] = field;
On 2010/04/06 04:33:18, dhollowa wrote:
> On 2010/04/06 03:39:09, James Hawkins wrote:
> > On 2010/04/06 02:11:51, dhollowa wrote:
> > > field->name() is not guaranteed to be unique.  It also may be an empty
> string.
> > 
> > > I'm thinking there are a few important cases to consider:
> > >   (1) When no 'name' or 'id' attributes are set in the <input> element.
> > >   (2) When we violate uniqueness of 'name' attributes.  I.e. two <inputs>
> have
> > > the same 'name' attribute.
> > >   (3) When we violate the uniqueness of 'id' attributes.  I.e. two
<inputs>
> > have
> > > the same 'id'.
> > >   (4) When the 'name' of one <input> element is the same as the 'id' of a
> > > different <input> element's.
> > > 
> > > I would think that we would only want to add to the map when we have an
'id'
> > for
> > > the field.  (See comments below.)
> > 
> > Long story short, we can't handle pretend to handle invalid forms.  The
worst
> > case we're considering here is when a form control element has an empty
> > nameForAutofill() (no id and no name).  In this case,
> > label.correspondingControl() will be NULL (isNull() technically) and we
don't
> > add the label (because there can be no label for this form control element.)

> > I've added a test case to show this, FormManagerTest.InvalidLabels.
> 
> Agreed.
> We can rule out case (1) because no label elements can refer to it.
> We can rule out cases (2) and (3) because it is really invalid input.
> But case (4) is valid input that I believe we may generate incorrect results
> for.  For example:
>   <form>
>     <label for='a'>a</label><input id='a' name='b' .../>
>     <label for='b'>b</label><input id='b' name='a' .../>
>     ...
>   </form>
> 
> Will, I believe, swap labels "a" and "b".

This is such a rare corner case that it's not worthing blocking this CL on it. 
We can add a TODO comment and implement id() in WebFormControlElement.

http://codereview.chromium.org/1622004/diff/1/2#newcode159
chrome/renderer/form_manager.cc:159:
name_map.find(field_element.nameForAutofill());
On 2010/04/06 04:33:18, dhollowa wrote:
> On 2010/04/06 03:39:09, James Hawkins wrote:
> > On 2010/04/06 02:11:51, dhollowa wrote:
> > > The 'for' attribute on a <label> should only match the 'id' of the <input>
> not
> > > the 'name' attribute, which is possible if we use the |nameForAutofill()|
> > > string.
> > 
> > Well we don't have an id() method for elements, so we use nameForAutofill()
> :-) 
> > If there is an id, nameForAutofill() returns the id.  If there's not an id,
> > nameForAutofill() returns the name.  In the latter case,
> correspondingElement()
> > will be isNull() anyway, so the point is moot.
> 
> From my read of the code (WebInputElement.cpp:100) it looks like 'name' is
> preferred over 'id'.  Hence the bug.
> 

See above comment.

[dhollowa, 2010-04-06 16:11:25]

LGTM.

[dhollowa, 2010-04-06 18:09:16]

SLGTM.