Bug 162369 – Align HTMLLabelElement.prototype.form with the HTML specification

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Align HTMLLabelElement.prototype.label with the HTML specification:
- https://html.spec.whatwg.org/#dom-label-form

Firefox and Chrome match the specification.

[Chris Dumez]

Created attachment 289506 [details]
Patch

[Build Bot]

Comment on attachment 289506 [details]
Patch

Attachment 289506 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2122464

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/form-control-infrastructure/form.html

[Build Bot]

Created attachment 289512 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 289513 [details]
Patch

[Build Bot]

Comment on attachment 289513 [details]
Patch

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

New failing tests:
fast/images/move-image-to-new-document.html

[Build Bot]

Created attachment 289520 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Chris Dumez]

(In reply to comment #6)
> Created attachment 289520 [details]
> Archive of layout-test-results from ews104 for mac-yosemite-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

(In reply to comment #5)
> Comment on attachment 289513 [details]
> Patch
> 
> Attachment 289513 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/2123128
> 
> New failing tests:
> fast/images/move-image-to-new-document.html

This test seems flaky, it also failed on my other patch.

[Alexey Proskuryakov]

Caused by r206244, rolling it out.

[Alexey Proskuryakov]

Rolled out in r206249.

[Chris Dumez]

(In reply to comment #8)
> Caused by r206244, rolling it out.

Thanks Alexey!

[Chris Dumez]

Created attachment 289619 [details]
Patch

[Chris Dumez]

Anyone interested in reviewing? The new implementation matches the specification exactly. Also this aligns our behavior with both Chrome and Firefox, thus improving interoperability.

[Alex Christensen]

Comment on attachment 289619 [details]
Patch

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

> Source/WebCore/html/HTMLLabelElement.h:35
>      WEBCORE_EXPORT LabelableElement* control();
> -    WEBCORE_EXPORT HTMLFormElement* form() const;
> +    WEBCORE_EXPORT HTMLFormElement* form();

We should make control const instead of form non-const.

[Chris Dumez]

(In reply to comment #13)
> Comment on attachment 289619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289619&action=review
> 
> > Source/WebCore/html/HTMLLabelElement.h:35
> >      WEBCORE_EXPORT LabelableElement* control();
> > -    WEBCORE_EXPORT HTMLFormElement* form() const;
> > +    WEBCORE_EXPORT HTMLFormElement* form();
> 
> We should make control const instead of form non-const.

I don't mind either way but you do realize this means I'll have to add a const_cast inside control(), right? In the past, you have complained about those.

[Ryosuke Niwa]

Comment on attachment 289619 [details]
Patch

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

>>> Source/WebCore/html/HTMLLabelElement.h:35
>>> +    WEBCORE_EXPORT HTMLFormElement* form();
>> 
>> We should make control const instead of form non-const.
> 
> I don't mind either way but you do realize this means I'll have to add a const_cast inside control(), right? In the past, you have complained about those.

I think the patch is fine as is but change const-ness if Alex feels strongly either way.

[Chris Dumez]

(In reply to comment #15)
> Comment on attachment 289619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289619&action=review
> 
> >>> Source/WebCore/html/HTMLLabelElement.h:35
> >>> +    WEBCORE_EXPORT HTMLFormElement* form();
> >> 
> >> We should make control const instead of form non-const.
> > 
> > I don't mind either way but you do realize this means I'll have to add a const_cast inside control(), right? In the past, you have complained about those.
> 
> I think the patch is fine as is but change const-ness if Alex feels strongly
> either way.

The issue with making form() const is that it requires control() to be const as well. However, control() uses descendantsOfType() internally which uses a const iterator if |this| is const. Therefore, to make control() const I would need to either const_cast |this| before calling descendantsOfType() or const_cast the Element pointed to by the const_iterator returned by descendantsOfType().

[Chris Dumez]

Comment on attachment 289619 [details]
Patch

Clearing flags on attachment: 289619

Committed r206332: <http://trac.webkit.org/changeset/206332>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.