Bug 80392 – [Forms] Introduce LabelableElement to share "labels" attribute implementation

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

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

At this time, "labels" attribute is implemented in HTMLFormControlElement. However, meter, and progress will be derived from HTMLElement instead of HTMLFormControlElement.

To share code of "labels" attribute implementation among HTMLFormControlElement, HTMLMeterElement and HTMLProgressElement, we introduce new base class LabeledElement as below:

class LabelableElement : public HTMLElement { ... PassRefPtr<NodeList> labels(); ... };
class HTMLFormControlElement : public LabelableElement, public FormAssociateElement { ... };
class HTMLMeterElement : public LabelableElement { ... };
class HTMLProgressElement : public LabelableElement { ... };

List of all labelable elements in the specification[1]:
1. button
2. input other than “hidden”
3. keygen
4. meter
5. output
6. progress
7. select
8. textarea

== References ==
[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#category-label

Created attachment 130334 [details]
Patch 1

Created attachment 130338 [details]
Patch 2

Created attachment 130340 [details]
Patch 3

Created attachment 130347 [details]
Patch 4

[Kent Tamura]

Comment on attachment 130347 [details]
Patch 4

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

> Source/WebCore/ChangeLog:11
> +        This patch introduces new class LabelableElement for base class of
> +        HTMLFormControlElement and moves isLabelable method to HTMLElement.
> +
> +        No new tests are required. No behavior changes.

You should explain a reason why you want to introduce LabelableElement in ChangeLog.

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

You don't need to define isLabelable() in HTMLElement.  Defining it in LabelableElement is enough.

> Source/WebCore/html/HTMLFormControlElement.cpp:-470
> -bool HTMLFormControlElement::isLabelable() const

Because you remove some code, you can remove some #include in this file.

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

The original code doesn't support HTMLOutputElement.  This is a behavior change and you need to write a test.

> Source/WebCore/html/LabelableElement.h:38
> +// LabelableElement is the default implementation of labels attribute.

This comment is confusing.  Usually an element is not an implementation of an element, not an attribute.
The comment should be something like the following:

LabelableElement represents "labelable element" defined in the HTML specification, and provides the implementation of the "labels" attribute.

> Source/WebCore/html/LabelableElement.h:48
> +} // namespace

The commet is confusing.  This doesn't close the anonymous namespace.
Please remove the comment, or make it "namespace WebCore".

[Kent Tamura]

(In reply to comment #6)
> This comment is confusing.  Usually an element is not an implementation of an element, not an attribute.

=> an element class is an implementation of an element, not...

Created attachment 130522 [details]
Patch 5

[Kent Tamura]

(In reply to comment #6)
> > Source/WebCore/ChangeLog:11
> > +        This patch introduces new class LabelableElement for base class of
> > +        HTMLFormControlElement and moves isLabelable method to HTMLElement.
> > +
> > +        No new tests are required. No behavior changes.
> 
> You should explain a reason why you want to introduce LabelableElement in ChangeLog.

Patch 5 has no update for this comment.

[Build Bot]

Comment on attachment 130522 [details]
Patch 5

Attachment 130522 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11836717

Created attachment 130538 [details]
Patch 6

[Build Bot]

Comment on attachment 130538 [details]
Patch 6

Attachment 130538 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11836748

Created attachment 130549 [details]
Patch 7

Take project.pbxproj generated by UI. Difference between UI generated and manually modified version are positions of entries. (><)

[Build Bot]

Comment on attachment 130549 [details]
Patch 7

Attachment 130549 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11832842

Created attachment 130560 [details]
Patch 8

Add settings = {ATTRIBUTES = (Private, );} for LabelableElement.h to project.pbxproj. It seems this attribute makes copying file to /WebKitBuild/Debug/WebCore.framework/PrivateHeaders

[Kent Tamura]

Comment on attachment 130560 [details]
Patch 8

ok

[WebKit Review Bot]

Comment on attachment 130560 [details]
Patch 8

Clearing flags on attachment: 130560

Committed r110055: <http://trac.webkit.org/changeset/110055>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.