Bug 80499 - [Forms] label.form attribute doesn't work
Summary: [Forms] label.form attribute doesn't work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL: http://jsfiddle.net/gDcAG/3/
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-07 00:12 PST by yosin
Modified: 2012-03-16 06:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch 1 (6.71 KB, patch)
2012-03-14 00:35 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (20.51 KB, patch)
2012-03-14 03:11 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (19.91 KB, patch)
2012-03-15 20:57 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (19.87 KB, patch)
2012-03-15 23:23 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-03-07 00:12:25 PST
According to the standard[1], the "label" element has "form" attribute in IDL.
However, in sample URI(http://jsfiddle.net/gDcAG/3/), JS expression label1.form returns null instead of HTMLFormElement object,
even if HTMLLabelElement.idl[2] has "form" attribute.

"form" attribute is implemented in HTMLElement.h[3] as virtualForm/findFormAncestor in HTMLElement.cpp[4].
In this implementation, "label" elements outside "form" elements don't work as expected.

== References ==
[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-label-element
[2] http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLLabelElement.idl
[3] http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLElement.h
[4] http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLElement.cpp
Comment 1 yosin 2012-03-14 00:35:32 PDT
Created attachment 131799 [details]
Patch 1
Comment 2 Kent Tamura 2012-03-14 01:18:19 PDT
Comment on attachment 131799 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=131799&action=review

> Source/WebCore/html/HTMLLabelElement.cpp:88
> +{

The code is very similar to FormAssociated::insertedIntoTree() and FormAssociated::resetFormOwner(). Please share the code.

> LayoutTests/fast/forms/form-attribute-expected.txt:NaN
>  PASS document.getElementsByTagName("button")[0].form is owner

Please remove FIXME in this file.

> LayoutTests/fast/forms/label/label-form-attribute.html:1
> +<DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

As you know, we already have some tests for 'form' attribute.
I recommend add test cases for 'label' to form-attribute.html and form-attribute-nonexistence-form-id.html, and remove this file.

> LayoutTests/fast/forms/label/label-form-attribute.html:12
> +var html = '<dvi id=outer>';

typo: <dvi>
Comment 3 yosin 2012-03-14 03:11:35 PDT
Created attachment 131817 [details]
Patch 2
Comment 4 Kent Tamura 2012-03-15 17:49:16 PDT
Comment on attachment 131817 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=131817&action=review

Please rebase the patch.  http://trac.webkit.org/changeset/110918 conflicts with this.

> Source/WebCore/html/FormAssociatedElement.cpp:100
> +    HTMLElement* element = toHTMLElement(this);
> +    setForm(findAssociatedForm(element, m_form));

The variable "element" is not needed.  setForm(findAssociatedForm(toHTMLElement(this), m_form));

> Source/WebCore/html/FormAssociatedElement.cpp:155
> +    setForm(findAssociatedForm(element, m_form));

ditto.
Comment 5 yosin 2012-03-15 20:57:25 PDT
Created attachment 132187 [details]
Patch 3
Comment 6 Kent Tamura 2012-03-15 21:00:52 PDT
Comment on attachment 132187 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=132187&action=review

> Source/WebCore/html/HTMLLabelElement.cpp:33
> +#include "HTMLFormControlElement.h"
> +#include "HTMLFormElement.h"

Do we need to include HTMLFormControlElement.h and HTMLFormElement.h?
Comment 7 yosin 2012-03-15 23:23:55 PDT
Created attachment 132210 [details]
Patch 4
Comment 8 yosin 2012-03-15 23:24:35 PDT
(In reply to comment #6)
> (From update of attachment 132187 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132187&action=review
> 
> > Source/WebCore/html/HTMLLabelElement.cpp:33
> > +#include "HTMLFormControlElement.h"
> > +#include "HTMLFormElement.h"
> 
> Do we need to include HTMLFormControlElement.h and HTMLFormElement.h?

Removed. Thanks for catching this!
Comment 9 Kent Tamura 2012-03-15 23:46:48 PDT
Comment on attachment 132210 [details]
Patch 4

ok
Comment 10 WebKit Review Bot 2012-03-16 06:05:52 PDT
Comment on attachment 132210 [details]
Patch 4

Clearing flags on attachment: 132210

Committed r110996: <http://trac.webkit.org/changeset/110996>
Comment 11 WebKit Review Bot 2012-03-16 06:05:57 PDT
All reviewed patches have been landed.  Closing bug.