Bug 38713 – Support the labels attribute in labelable form controls

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

[Yael]

The elements specified http://dev.w3.org/html5/spec/Overview.html#category-label should support the labels attribute.

[Yael]

Created attachment 56449 [details]
Patch.

This patch adds a new cache type to NodeRareData. The cache is used for the labels list for each form control and is created on demand.

[Kent Tamura]

Comment on attachment 56449 [details]
Patch.

WebCore/dom/Node.cpp:900
 +  void Node::notifyLocalNodeListsLabelChanged()
The content of this function is identical to notifyLocalNodeListsChildrenChanged().
Don't you want to invalidate just NodeListsNodeData::m_labelsNodeListCache?



WebCore/dom/Node.cpp:2532
 +          markDOMObjectWrapper(markStack, globalData, nodeLists->m_labelsNodeListCache.get());
No tests for this behavior. 


WebCore/html/HTMLLabelElement.cpp:177
 +          document()->notifyLocalNodeListsLabelChanged();
This part should be moved to parseMappedAttribute(). because we need to call it for "for" HTML attribute change too.
We need to have tests for label.setAttribute("for", ...) and label.removeAttribute("for").


LayoutTests/ChangeLog:8
 +          * fast/form-controls: Added.
Why you need to have new directory?  Isn't fast/forms enough?


LayoutTests/ChangeLog:20
 +          * fast/form-controls/labels-remove-parent-label.html: Added.
It seems we can write these tests in script-tests format.

[Yael]

Created attachment 56745 [details]
Patch addressing issues found in comment #2.

[WebKit Review Bot]

Attachment 56745 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/dom/Node.cpp:903:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/dom/Node.cpp:904:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 56747 [details]
Remove tabs.

[Kent Tamura]

Comment on attachment 56747 [details]
Remove tabs.

WebCore/html/HTMLLabelElement.cpp:178
 +      if (attribute->name() == forAttr || attribute->name() == "htmlFor") {
You don't need to handle "htmlFor".  HTML5 doesn't define such HTML attribute.


Other parts look ok.

[Yael]

Created attachment 56783 [details]
Patch, remove "htmlFor" from parseMappedAttribute. (Comment #6)

[WebKit Review Bot]

Attachment 56783 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2371041

[Yael]

Created attachment 56805 [details]
Update Visual Studio project file.

[Yael]

I have no idea why WebCore.vcproj did not apply cleanly. Is applied and the patch did build on my windows machine.
Is it possible to get the rej file from that machine?
In the meantime, I am just going to try again :-)

[Yael]

Created attachment 56819 [details]
Patch, try again on the windows bot

No changes from previous patch, other than updaing my environment.

[Yael]

This might be already known, but the windows bot is rejecting every patch that modifies WebCore.vcproj, not only this patch.

For example these have the same problem:
https://bugs.webkit.org/show_bug.cgi?id=39574
https://bugs.webkit.org/show_bug.cgi?id=39557

[Eric Seidel (no email)]

Sounds like a bug in the EWS bot.  Or possibly svn-apply, but probably something is just funny about our win-ews configuration if "webkit-patch apply-attachment 56819 [details]" works fine off the bot (I have not tested).

[Chris Jerdonek]

(In reply to comment #13)
> Sounds like a bug in the EWS bot.  Or possibly svn-apply, but probably something is just funny about our win-ews configuration if "webkit-patch apply-attachment 56819 [details]" works fine off the bot (I have not tested).

Might this have something to do with .vcproj files using carriage returns?

See this report, for instance:

https://bugs.webkit.org/show_bug.cgi?id=38149

Maybe carriage returns are getting deleted somewhere along the line, causing the patch not to apply.

[Yael]

(In reply to comment #14)
> (In reply to comment #13)
> > Sounds like a bug in the EWS bot.  Or possibly svn-apply, but probably something is just funny about our win-ews configuration if "webkit-patch apply-attachment 56819 [details] [details]" works fine off the bot (I have not tested).
> 
> Might this have something to do with .vcproj files using carriage returns?
> 
> See this report, for instance:
> 
> https://bugs.webkit.org/show_bug.cgi?id=38149
> 
> Maybe carriage returns are getting deleted somewhere along the line, causing the patch not to apply.

It probably is a carriage return issue. But there is probably also a SVN configuration issue on that machine. 
I am saying that because the patch did apply on my windows machine (inside a cygwin shell).

[Eric Seidel (no email)]

The EWS machines use Git, they apply patches via svn-apply.  If we believe this is an SVN or EWS bug we should get a bug on file.

[Yael]

(In reply to comment #16)
> The EWS machines use Git, they apply patches via svn-apply.  If we believe this is an SVN or EWS bug we should get a bug on file.

Filed https://bugs.webkit.org/show_bug.cgi?id=39607

[Kent Tamura]

Comment on attachment 56819 [details]
Patch, try again on the windows bot

WebCore/dom/Node.cpp:940
 +  void Node::removeCachedLabelsNodeList(DynamicNodeList* list, Node* /*node*/)
The Node* parameter is not used. We have no reason to keep it.

[Yael]

Created attachment 57054 [details]
Remove un-needed argument

[Kent Tamura]

Comment on attachment 57054 [details]
Remove un-needed argument

OK.

[Yael]

Committed r60232. <http://trac.webkit.org/changeset/60232>