Bug 87834 – form.elements should not contain image elements.

• Component: Forms
• Status: NEW

[Rakesh]

Image elements should not appear in the form.elements as image element is not mentioned in listed elements of specs http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#category-listed.

[Rakesh]

Created attachment 144760 [details]
Proposed patch

[Kent Tamura]

Comment on attachment 144760 [details]
Proposed patch

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

I know the standard and other browsers don't have this behavior.  But I'm not sure it's ok to remove this because this is an incompatible change.


> Source/WebCore/html/HTMLFormElement.h:113
>      const Vector<FormAssociatedElement*>& associatedElements() const { return m_associatedElements; }
> -    const Vector<HTMLImageElement*>& imageElements() const { return m_imageElements; }
>  

You can remove m_imageElements, registerImgElement(), and removeImgElement().
Also, you can remove HTMLImageElement::m_form and HTMLImageElement::removedFrom()

[Rakesh]

(In reply to comment #2)
Thanks for your inputs.

> I know the standard and other browsers don't have this behavior.  But I'm not sure it's ok to remove this because this is an incompatible change.
> 
> You can remove m_imageElements, registerImgElement(), and removeImgElement().
The specs has some point as follows:
When a form element is indexed for named property retrieval .. :

2. If candidates is empty, let candidates be a live RadioNodeList object containing all the img elements that are descendants of the form element and that have either an id attribute or a name attribute equal to name, in tree order.

May be m_imageElement might be useful here but yes it is better to remove those and specs compatibility can be done in another bug.

> Also, you can remove HTMLImageElement::m_form and HTMLImageElement::removedFrom()

Yes, as img element does not fall under form-associated elements category, we may have to remove above code also.

[Rakesh]

Created attachment 144788 [details]
Updated patch

Removed the Form element related code from HTMLImageElement

[WebKit Review Bot]

Comment on attachment 144788 [details]
Updated patch

Attachment 144788 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12844934

New failing tests:
fast/forms/form-image-access-by-name.html
fast/forms/reparented-image-as-property.html

[WebKit Review Bot]

Created attachment 144866 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexey Proskuryakov]

> fast/forms/form-image-access-by-name.html
> fast/forms/reparented-image-as-property.html

These both pass in Firefox.

[Darin Adler]

(In reply to comment #2)
> I know the standard and other browsers don't have this behavior.

According to Alexey’s test results, Firefox has at least some of “this behavior”.

[Rakesh]

(In reply to comment #7)

> These both pass in Firefox.
Thanks Alexey for your inputs.
> 
> According to Alexey’s test results, Firefox has at least some of “this behavior”.

Thanks Darin.
It looks like the issue may be the way we handle nameGetter of form object. We use the same HTMLFormCollection:::namedItems functions for both the FormCollection's nameGetter and as well as HTMLFormElement's nameGetter.
May be we should handle them differently.

[Ryosuke Niwa]

Comment on attachment 144788 [details]
Updated patch

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

r- per comments #7 #9. Please update a new patch that doesn't regress the compatibility with IE and FF.

> Source/WebCore/ChangeLog:8
> +        Removed support for image elements from HTMLFormCollection  as image elements

Nit: two spaces before "as".

[Kent Tamura]

Ah, I misunderstood the situation.
  formElement.<name> should support img elements.  It's defined by the standard.
  formElement.elements.<name> or formElement.elements.namedItem(<name>) works in WebKit, but it's not defined byt the standard and other browsers don't support.

[Rakesh]

Created attachment 145288 [details]
Updated patch

Handled regressions, now formElement.<name> will support img elements

[WebKit Review Bot]

Comment on attachment 145288 [details]
Updated patch

Attachment 145288 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12861704

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html
fast/forms/form-collection-elements.html

[WebKit Review Bot]

Created attachment 145339 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rakesh]

(In reply to comment #13)
> New failing tests:
> http/tests/media/media-source/video-media-source-event-attributes.html
Not sure why this is failing, will check.

> fast/forms/form-collection-elements.html
Missed updating expected.txt, will do that.

[Rakesh]

Created attachment 145528 [details]
Updated patch

Updated form-collection-elements-expected.txt. http/tests/media/media-source/video-media-source-event-attributes.html is not failing on my build, may be it will pass now.

[Rakesh]

ping! review?

[Sam Sneddon [:gsnedders]]

*** Bug 223712 has been marked as a duplicate of this bug. ***

[Ahmad Saleem]

*** Bug 249852 has been marked as a duplicate of this bug. ***

[Ahmad Saleem]

Created attachment 466290 [details]
GitHub Patch Screenshot

Test Case - https://jsfiddle.net/7dfsnp23/show (from bug 249852)

^ This test passes if I do attached screenshots (GitHub Desktop) changes on local and compile the build. I haven't run all tests to see if it causes any other issue. Happy to do "Draft" PR to run through EWS, if it has any impact.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #20)
> Created attachment 466290 [details]
> GitHub Patch Screenshot
> 
> Test Case - https://jsfiddle.net/7dfsnp23/show (from bug 249852)
> 
> ^ This test passes if I do attached screenshots (GitHub Desktop) changes on
> local and compile the build. I haven't run all tests to see if it causes any
> other issue. Happy to do "Draft" PR to run through EWS, if it has any impact.

I did this PR - https://github.com/WebKit/WebKit/pull/13613

I think I will not be able to fix remaining failing tests so I will close my PR after this EWS run.

If someone want to take a jab and fix remaining test cases, it would be great.

form-associated-element.html <- Test taken from Chromium / Blink (if I am not mistaken)

image-named-access.html

reparented-image-as-property.html

[Radar WebKit Bug Importer]

<rdar://problem/122588036>