Bug 38688 – Support control attribute of HTMLLabelElement

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

[Yael]

From http://dev.w3.org/html5/spec/Overview.html#the-label-element, the control attribute should return a reference to the control associated with the label, but only if it is a labelable control.

[Yael]

Created attachment 55296 [details]
Patch adding support for the control attribute.

[WebKit Review Bot]

Attachment 55296 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2168027

[Yael]

Created attachment 55316 [details]
Attempt to fix Chromium build

[WebKit Review Bot]

Attachment 55316 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2174030

[Yael]

Created attachment 55332 [details]
Another try to fix the Chromium build

[WebKit Review Bot]

Attachment 55332 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2177026

[Yael]

Created attachment 55360 [details]
Another try to fix the Chromium build

[WebKit Review Bot]

Attachment 55360 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2232047

[Yael]

Created attachment 55369 [details]
Maybe this time it will compile?

[Yael]

With this change, the following form controls cannot be associated with a label: fieldset, legend, optgroup and option.

[Darin Adler]

Comment on attachment 55369 [details]
Maybe this time it will compile?

Looks good. But the multiple things you are doing need to be tested.

You changed the label element code so that it won't label other types of form control. So you need a test that shows that behavior change.

Your comment says that you "cleaned up" correspondingControl, but that's not what you did. You fixed a bug in it!

I'd also like to see the code refactored so the two branches of the function shared the code to check isFormControlElement rather than repeating the code twice. You could use the traverseElement code to make it even easier to share code.

> +bool HTMLFormControlElement::isControlLabelable() const

How about just "isLabelable" for this?
> +    // FIXME: Add meterTag and outputTag to the list once we support them.
> +    return hasTagName(buttonTag) || hasTagName(inputTag) || hasTagName(keygenTag)
> +#if ENABLE(PROGRESS_TAG)
> +    || hasTagName(progressTag)
> +#endif
> +    || hasTagName(selectTag) || hasTagName(textareaTag);

You should indent the continuation lines 4 spaces in.

> -HTMLElement* HTMLLabelElement::correspondingControl()
> +HTMLFormControlElement* HTMLLabelElement::correspondingControl() const

Does this really need to be a const member function? The need to const_cast in here is ugly.

> +        // the control must be "labelable form-associated element".

I see the "labelable" part here, but I don't see the "form-associated" part.

> +PassRefPtr<HTMLElement> HTMLLabelElement::control() const
> +{
> +    return correspondingControl();
> +}

There is no reason to return a PassRefPtr here. It should just be a raw pointer.

> -    HTMLElement* correspondingControl();
> +    HTMLFormControlElement* correspondingControl() const;

> +    PassRefPtr<HTMLElement> control() const;

I suggest renaming our correspondingControl function to control rather than adding a second function. And I see no reason to make this a const member function.

>      interface HTMLLabelElement : HTMLElement {
>          readonly attribute HTMLFormElement form;
> -        attribute [ConvertNullToNullString] DOMString accessKey;
> -        attribute [ConvertNullToNullString] DOMString htmlFor;
> +                 attribute [ConvertNullToNullString] DOMString accessKey;
> +                 attribute [ConvertNullToNullString] DOMString htmlFor;
> +        readonly attribute HTMLElement control;
>      };

I don't think we need to mimic the indenting of the HTML5 specification. I'd prefer to just have things indented one tab stop and not try to line things up.

Fix a few things and put it up for review again. Looks good.

[Yael]

Created attachment 55614 [details]
Patch for addressing review comments.

This patch addresses the comments and adds more tests, including a manual test for testing that the label properly propagates focus, activation and hover to the correct form control element, while ignoring form control elements which are not labelable.

[Darin Adler]

Comment on attachment 55614 [details]
Patch for addressing review comments.

> +static HTMLFormControlElement* nodeAsLabelableFormControl(Node* node)
> +{
> +    if (!node)
> +        return 0;
> +
> +    if (node->isHTMLElement() && static_cast<HTMLElement*>(node)->isFormControlElement()) {
> +        HTMLFormControlElement* formControlElement = static_cast<HTMLFormControlElement*>(node);
> +        if (formControlElement->isLabelable())
> +            return formControlElement;
> +    }
> +
> +    return 0;
> +}

I think this would read even better if it used early returns exclusively.

> +        // Search the children of the label element for a form element.

This searches the descendants of the label element, not just the children.

> +            if (HTMLFormControlElement* formControlElement = nodeAsLabelableFormControl(node))
> +                return formControlElement;

As I said before, I see how this checks for "labelable" elements, but not "form-associated". Maybe that's the same thing as checking isFormControlElement?

> +class HTMLFormControlElement;    
> +
>  class HTMLLabelElement : public HTMLElement {
>  public:
>      HTMLLabelElement(const QualifiedName&, Document*);
> @@ -46,7 +48,7 @@ public:
>      // Overridden to either click() or focus() the corresponding control.
>      virtual void defaultEventHandler(Event*);
>  
> -    HTMLElement* correspondingControl();
> +    HTMLElement* control();

Did you mean to change the type here to HTMLFormControlElement? If not, then why the forward declaration?

> +        readonly attribute HTMLElement control;

Was the type here supposed to be HTMLFormControlElement?

I don't entirely understand why the manual test you made is a manual test. Automated tests can both activate and focus, although perhaps for hover you need a manual test.

I'm going to say review- mainly because of the unused forward declaration in HTMLLabelElement.h that clearly seems to be in error.

[Yael]

(In reply to comment #13)
> As I said before, I see how this checks for "labelable" elements, but not "form-associated". Maybe that's the same thing as checking isFormControlElement?
Sorry, I forgot to mention that form-associated elements are defined here http://dev.w3.org/html5/spec/Overview.html#form-associated-element, and are a super-set of labelable elements.

[Yael]

Created attachment 55650 [details]
Patch addressing comment #13.

[Darin Adler]

Comment on attachment 55650 [details]
Patch addressing comment #13.

> -HTMLElement* HTMLLabelElement::correspondingControl()
> +HTMLElement* HTMLLabelElement::control()

Why isn't the return type of this HTMLFormControlElement*? There are a lot of places in this patch that use HTMLElement* but instead should use HTMLFormControlElement.

> +        // Search the children and descendants of the label element for a form element.
> +        // per http://dev.w3.org/html5/spec/Overview.html#the-label-element
> +        // the form element must be "labelable form-associated element".

I think the comment should just leave out the link and say something more like this:

    // HTML5 says to search our descendants for the first labelable form-associated element
    // in document order.

OK as-is, though.

[Yael]

(In reply to comment #16)
> (From update of attachment 55650 [details])
> > -HTMLElement* HTMLLabelElement::correspondingControl()
> > +HTMLElement* HTMLLabelElement::control()
> 
> Why isn't the return type of this HTMLFormControlElement*? There are a lot of places in this patch that use HTMLElement* but instead should use HTMLFormControlElement.
I was going to do that for correspondingControl(), but since control() is defined by the HTML5 spec to be of type HTMLElement, I thought that I cannot do that.

[Darin Adler]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 55650 [details] [details])
> > > -HTMLElement* HTMLLabelElement::correspondingControl()
> > > +HTMLElement* HTMLLabelElement::control()
> > 
> > Why isn't the return type of this HTMLFormControlElement*? There are a lot of places in this patch that use HTMLElement* but instead should use HTMLFormControlElement.
> I was going to do that for correspondingControl(), but since control() is defined by the HTML5 spec to be of type HTMLElement, I thought that I cannot do that.

We can do it.

There are three angles on this:

    1) The DOM implementation itself can always use a more-specific return type. The binding mechanism supports that. And this gives us more type checking and in some cases can even lead to better code in the DOM implementation, so is probably worth doing.

    2) The JavaScript binding can't see the return type of a function, so it also doesn't matter exactly what object type is specified in the IDL file. So we probably could get away with using this type in the IDL file too.

    3) The reason HTML5 doesn't specify this is that HTMLFormControlElement is an implementation detail of WebKit and generally should not be exposed. It's an abstract base class. I do think, however, that it may be something you can see by looking at the prototype chains of form control elements. This has nothing to do with the return type of the function, though.

[Yael]

Landed in r59228  <http://trac.webkit.org/changeset/59228>