Bug 80380

Summary: [Forms] The "meter" element should not be a form-associated element.
Product: WebKit Reporter: yosin
Component: FormsAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ojan, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 80392    
Bug Blocks: 80240    
Attachments:
Description Flags
Patch 1
none
Patch 2
none
Patch 3
none
Patch 4 none

Description yosin 2012-03-05 22:21:43 PST
According the specification, the "optgroup" element isn't a form-associate element.
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#form-associated-element

However, current implementation of HTMLMeterElement is derived from HTMLFormControlElement. We have unexpected dependency between HTMLOptGroupElement and HTMLFormControlElement and FormAssociatedElement. Also, We waste time in loop of HTMLFormElement::m_associatedElements.

For clear understanding and better maintainability, it is better that C++ class hierarchy should be similar to specified in the specification.
Comment 1 yosin 2012-03-13 21:09:18 PDT
Created attachment 131778 [details]
Patch 1
Comment 2 yosin 2012-03-13 21:14:35 PDT
Patch 1 contains changes for HTMLMeterElement itself and HTMLLabelElement for supporting LabelabelElement instead of HTMLFormControlElement.

This HTMLLabelElement changes is also required to change base class of the "progress" element to LabelableElement.
Comment 3 Kent Tamura 2012-03-13 21:41:45 PDT
Comment on attachment 131778 [details]
Patch 1

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

> Source/WebCore/html/HTMLElement.h:105
> +    virtual bool isLabelable() const { return false; }

The function means:
 * This HTMLElement can be casted to LabelableElement, AND
 * This LabelableElement actually supports "labels" property.

isClassName() functions of HTMLElement and Element usually have only the first role. isLabelable() in this patch will make other developers confused.

I recommend to rename the existing LabelableElement::isLabelable() to LabelableElement::supportLabels(), and restrict the role of HTMLElement::isLabelable().
Comment 4 yosin 2012-03-14 01:28:52 PDT
Created attachment 131805 [details]
Patch 2
Comment 5 Kent Tamura 2012-03-14 01:52:18 PDT
Comment on attachment 131805 [details]
Patch 2

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

> Source/WebCore/html/LabelableElement.h:50
> +    virtual bool isLabelable() const OVERRIDE { return true; }

This should be "isLabelable() const OVERRIDE FINAL { return true; }" because subclasses must not override this.

> LayoutTests/ChangeLog:14
> +        * fast/dom/HTMLMeterElement/meter-element-form-expected.txt: Removed.
> +        * fast/dom/HTMLMeterElement/meter-element-form.html: Removed.

Please do not remove the test.  We should change it so that it confirms HTMLMeterElement doesn't support "form" IDL attribute.
Comment 6 yosin 2012-03-14 03:24:16 PDT
Created attachment 131819 [details]
Patch 3
Comment 7 Kent Tamura 2012-03-14 07:35:38 PDT
Comment on attachment 131819 [details]
Patch 3

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

> LayoutTests/fast/dom/HTMLMeterElement/meter-element-form.html:16
> +if (document.getElementById('meter1').form == null)

Should it be undefined, not null, because HTMLMeterElement doesn't have form IDL attribute?
Comment 8 yosin 2012-03-14 18:52:34 PDT
Created attachment 131972 [details]
Patch 4
Comment 9 yosin 2012-03-14 18:53:28 PDT
(In reply to comment #7)
> (From update of attachment 131819 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131819&action=review
> 
> > LayoutTests/fast/dom/HTMLMeterElement/meter-element-form.html:16
> > +if (document.getElementById('meter1').form == null)
> 
> Should it be undefined, not null, because HTMLMeterElement doesn't have form IDL attribute?

You're right. I forgot to update IDL file which contains the "form" attribute of the "meter" lement.
Comment 10 Kent Tamura 2012-03-14 19:27:15 PDT
Comment on attachment 131972 [details]
Patch 4

ok
Comment 11 WebKit Review Bot 2012-03-15 18:49:23 PDT
Comment on attachment 131972 [details]
Patch 4

Clearing flags on attachment: 131972

Committed r110927: <http://trac.webkit.org/changeset/110927>
Comment 12 WebKit Review Bot 2012-03-15 18:49:28 PDT
All reviewed patches have been landed.  Closing bug.