Bug 80110 – [Forms] HTMLFieldSetElement.idl doesn't have elements attribute.

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

See http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-fieldset-element

elements attribute returns list of following elements as same as forms.elements:
 * button 
 * fieldset 
 * input 
 * keygen 
 * object 
 * output 
 * select 
 * textarea

[Rakesh]

I am planning to take up this implementation. Few thoughts:

Presently HTMLFormCollection is tightly coupled with HTMLFormElement. So first step would be to decouple these.

Can we have a class like FormControlsContainer and make both HTMLFormElement and HTMLFieldSetElement derive from it?
We can have separate implementations for eg. of length() for Form and FieldSet.

As HTMLFieldSetElement is set of form controls, can we have list of elements by  traversing all the children and checking if each element is a form control? AFAIK each form control is associated with its form element if any. Is there any mechanism to associate a form control with FieldSet?

Please do let me know if you have suggestions/better approach.

Thanks.

[Kent Tamura]

(In reply to comment #1)
Before implementing HTMLFieldSetElement::elements, we need to consider how to realize RadioNodeList [1].  HTMLFormControlsCollection needs to support it, but our HTMLFormElement::elements doesn't support it for now.

Can we add RadioNodeList support to HTMLFormCollection.*?  Can we add RadioNodeList support to HTMLCollection.*?


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

[Rakesh]

(In reply to comment #2)
> (In reply to comment #1)

Thanks for replying.

> Before implementing HTMLFieldSetElement::elements, we need to consider how to realize RadioNodeList [1].  HTMLFormControlsCollection needs to support it, but our HTMLFormElement::elements doesn't support it for now.
> 
> Can we add RadioNodeList support to HTMLFormCollection.*?  Can we add RadioNodeList support to HTMLCollection.*?

Yes surely, will check RadioNodeList and add support for that.

> 
> 
> [1]: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#radionodelist

[Rakesh]

(In reply to comment #2)
> Can we add RadioNodeList support to HTMLFormCollection.*?  Can we add RadioNodeList support to HTMLCollection.*?

As RadioNodeList support has landed, can we now think of refactoring HTMLFormCollection to be independent of HTMLFormElement?

[Rakesh]

Created attachment 143262 [details]
proposed patch

Added support for elements attribute in HTMLFieldSetElement.

[Kent Tamura]

Comment on attachment 143262 [details]
proposed patch

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

> Source/WebCore/html/HTMLFieldSetElement.cpp:96
> +    uint64_t docversion = document()->domTreeVersion();

should be docVersion, documentVersion, or domTreeVersion.

> Source/WebCore/html/HTMLFieldSetElement.cpp:105
> +    for (Element* node = firstElementChild(); node; node = node->nextElementSibling()) {

We need a test to confirm that fieldset.elements contains listed-elements in deeper places like
  <fieldset>
    <div>
       <input>
    </div>
  </fieldset>

> Source/WebCore/html/HTMLFieldSetElement.cpp:117
> +        if (node->hasTagName(imgTag)) {
> +            m_imageElements.append(static_cast<HTMLImageElement*>(node));
> +            continue;
> +        }

Why do you need to collect img elements?

> Source/WebCore/html/HTMLFieldSetElement.cpp:129
> +const Vector<HTMLImageElement*>& HTMLFieldSetElement::imageElements() const

ditto.

> Source/WebCore/html/HTMLFieldSetElement.h:45
> +    const Vector<HTMLImageElement*>& imageElements() const;

ditto.

> Source/WebCore/html/HTMLFieldSetElement.h:65
> +    mutable Vector<HTMLImageElement*> m_imageElements;

ditto.

> Source/WebCore/html/HTMLFieldSetElement.idl:29
>  
> +        readonly attribute HTMLCollection elements;
> +
>          readonly attribute DOMString type;

Please put 'elements' after 'type' in order to follow the IDL in the specification.

> Source/WebCore/html/RadioNodeList.cpp:87
> +    return equalIgnoringCase(testElement->getIdAttribute(), m_name) || equalIgnoringCase(testElement->getNameAttribute(), m_name);

This is not a new code, but equalIgnoringCase() looks wrong.  ID matching and name matching should be case-sensitive.

[Rakesh]

(In reply to comment #6)
Thanks for reviewing this patch.

> should be docVersion, documentVersion, or domTreeVersion.
I missed that, will do it.

> We need a test to confirm that fieldset.elements contains listed-elements in deeper places like
>   <fieldset>
>     <div>
>        <input>
>     </div>
>   </fieldset>
Will add the test, I think we need to traverse all children, only sibling traversing is not enough?

> 
> Why do you need to collect img elements?
Code in FormCollection uses image elements.
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLFormCollection.cpp#L137

> Please put 'elements' after 'type' in order to follow the IDL in the specification.
Will do this change.

> This is not a new code, but equalIgnoringCase() looks wrong.  ID matching and name matching should be case-sensitive.
Yes will make it case sensitive.

[Kent Tamura]

(In reply to comment #7)
> > Why do you need to collect img elements?
> Code in FormCollection uses image elements.
> http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLFormCollection.cpp#L137

It seems the specification doesn't ask to support img elements in HTMLFieldSetElement::elements.  How other browsers work for img elemnents in a form/fieldset element?


> > This is not a new code, but equalIgnoringCase() looks wrong.  ID matching and name matching should be case-sensitive.
> Yes will make it case sensitive.

It's better to make another bug and patch.

[Rakesh]

(In reply to comment #8)
http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLFormCollection.cpp#L137
> 
> It seems the specification doesn't ask to support img elements in HTMLFieldSetElement::elements.  How other browsers work for img elemnents in a form/fieldset element?
> 
Specs says excluding image buttons(http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#the-form-element). I am not sure if that means image elements also. In any case the form.elements behavior is inconsistent for image elements in webkit: 
1. form.elements[some number] does not give image element.
2. form.elements["imageElem"] or form.elements.namedItem("imageElem") gives the image element.
3. form.length does not include the image elements count.

Other browsers, firefox and opera do not consider image elements in form.elements.

> It's better to make another bug and patch.
ok

[Rakesh]

(In reply to comment #9)
> (In reply to comment #8)

Also listed elements(http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#category-listed) does not have image element specified, Do you think image elements support needs be removed from FormCollection?

[Kent Tamura]

(In reply to comment #10)
> Also listed elements(http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#category-listed) does not have image element specified, Do you think image elements support needs be removed from FormCollection?

We might want to remove the img element support in the future.  But it's not the scope of this bug, and we don't need to hurry.

As for HTMLFieldSetElement::elements, we don't need to support img elements at all.  So HTMLFormCollection::getNamedFormItem() may have

 if (base()->hasTagName(fieldsetTag))
     return 0;

before getting imageElementsArray?

[Rakesh]

Created attachment 143537 [details]
proposed patch

Modified as per comments

[Kent Tamura]

Comment on attachment 143537 [details]
proposed patch

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

> Source/WebCore/html/HTMLFieldSetElement.h:64
> +    OwnPtr<HTMLFormCollection> m_elementsCollection;
> +
> +    mutable Vector<FormAssociatedElement*> m_associatedElements;
> +
> +    mutable uint64_t m_documentVersion;

nit: these blank lines are not needed.

m_documentVersion should be renamed to m_documentVersionForAssociatedElements, or we should add a comment about the purpose.

> Source/WebCore/html/RadioNodeList.cpp:87
> +    return equalIgnoringCase(testElement->getIdAttribute(), m_name) || equalIgnoringCase(testElement->getNameAttribute(), m_name);

Please add a FIXME comment about the incorrectness, or fix the problem before this bug.

> Source/WebCore/html/RadioNodeList.cpp:95
>  {
> +    if (testElement->hasTagName(objectTag))
> +        return checkIfNameOrIdMatches(testElement);
> +
>      if (!testElement->isFormControlElement())

This change affects HTMLFormElement::elements too, right?
So, we should add <object> support before this bug.

[Rakesh]

(In reply to comment #13)
> Please add a FIXME comment about the incorrectness, or fix the problem before this bug.

Created https://bugs.webkit.org/show_bug.cgi?id=87369 for this problem.

> This change affects HTMLFormElement::elements too, right?
> So, we should add <object> support before this bug.

Created https://bugs.webkit.org/show_bug.cgi?id=87371 for this issue.

[Rakesh]

Created attachment 144037 [details]
patch

Updated patch

[Kent Tamura]

Comment on attachment 144037 [details]
patch

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

r- because of test coverage

> LayoutTests/fast/forms/fieldset/fieldset-elements.html:86
> +

Please test:
  owner.parentNode.removeChild(owner);
  then, adding/removing some form controls to/from 'owner' and owner.elements is correctly updated.

Also, we need a test to check if changing input type from/to 'image' updates HTMLFieldSetElement::elements correctly.

[Rakesh]

Created attachment 144387 [details]
updated patch

Added more test converage.

[Kent Tamura]

Comment on attachment 144387 [details]
updated patch

Looks good.  Thank you!

[WebKit Review Bot]

Comment on attachment 144387 [details]
updated patch

Clearing flags on attachment: 144387

Committed r118720: <http://trac.webkit.org/changeset/118720>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.