Bug 80234 – [Forms] The "optgroup" element should not be a form-associated element.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

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 HTMLOptGroupElement 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.

Created attachment 130054 [details]
Patch 1

[Kent Tamura]

Comment on attachment 130054 [details]
Patch 1

Please take care of HTMLOptionElement::disabled().

Created attachment 130070 [details]
Patch 2

[Kent Tamura]

Comment on attachment 130070 [details]
Patch 2

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

> Source/WebCore/html/HTMLOptionElement.cpp:302
> -    return ownElementDisabled() || (parentNode() && static_cast<HTMLFormControlElement*>(parentNode())->disabled());
> +    return ownElementDisabled() || (parentNode() && static_cast<HTMLElement*>(parentNode())->disabled());

You had better check parentNode()->isHTMLElment().

Nit: we had better check hasTagName(optgroupTag).  However it's not your bug, and you don't need to add it in this patch.

Created attachment 130072 [details]
Patch 3

[Kent Tamura]

Comment on attachment 130072 [details]
Patch 3

ok.   Thank you!

[WebKit Review Bot]

Comment on attachment 130072 [details]
Patch 3

Clearing flags on attachment: 130072

Committed r109729: <http://trac.webkit.org/changeset/109729>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

If not all form controls will inherit from HTMLFormControlElement now, then we should rename HTMLFormControlElement to something more appropriate.

Or was OPTGROUP not really a form control in some other sense? This is confusing.

My patches will make subclasses of HTMLFormControlElement[1] to classes in HTMLFormControlCollection[2] only, for matching implementation and the specification[3].

[1] A comment of HTMLFormControlElement:
// HTMLFormControlElement is the default implementation of FormAssociatedElement,
// and form-associated element implementations should use HTMLFormControlElement
// unless there is a special reason.

[2] HTMLFormControlCollection
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#htmlformcontrolscollection

[3] http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#category-listed

HTMLFromControlCollection contains "listed elements" of form-associated elements:
1. button
2. fieldset
3. input
4. keygen
5. object
6. output
7. select
8. textarea

Note: All of form-associated elements except for the "label" element, are in HTMLFormControlCollection.

[Kent Tamura]

(In reply to comment #9)
> If not all form controls will inherit from HTMLFormControlElement now, then we should rename HTMLFormControlElement to something more appropriate.

I agree with you.  We should have a better name.
NonObjectFormAssociatedElement is a descriptive name though it isn't cool.

> Or was OPTGROUP not really a form control in some other sense? This is confusing.

We think the following elements should not inherit from HTMLFormControlElement.
- optgroup
- option
- meter
- progress

I checked all of isFormControlElement() usage in WebKit roughly, and I think it's reasonable to make them non-HTMLFormControlElement.

[Kent Tamura]

(In reply to comment #11)
> We think the following elements should not inherit from HTMLFormControlElement.
> - optgroup
> - option
> - meter
> - progress

legend, too.

Note: FormAssociatedElement::isEnumeratable method denotes whether an element into HTMLFormControlCollection or not.

[Alexey Proskuryakov]

I'm not sure if I understood this correctly. Would HTMLFormListedEleemnt be an appropriate name?

(In reply to comment #14)
> I'm not sure if I understood this correctly. Would HTMLFormListedEleemnt be an appropriate name?

Unfortunately, no for current implementation. As tkent@, proper name of current implementation is HTMLFormControlElementButObjectElement.

If we can implement HTMLFormListedElement (or AssociateFormListedElement) as mix-in (== not inherit from HTMLElement as FormAssociatedElement), we can use it for all of them including "object" element.

In this case, we can writer:

class HTMLButton : public HTMLElement, public AssociateFormListedElement { ... }
class HTMLObjectElement : public HTMLPulugInElement, public AssociateFormListedElement { ... }

It seems we may want to have ValidableElement. HTMLFormControlElement implements validity but HTMLObject doesn't need form validation.

So, I propose that break HTMLFormControlElement into mixable (= not inherit HTMLElement) classes.

This bug is part of HTMLFormControlElement re-factoring meta bug https://bugs.webkit.org/show_bug.cgi?id=80381