Bug 38713

Summary: Support the labels attribute in labelable form controls
Product: WebKit Reporter: Yael <yael>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cjerdonek, dbates, eric, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 38688    
Bug Blocks: 19264    
Attachments:
Description Flags
Patch.
tkent: review-
Patch addressing issues found in comment #2.
none
Remove tabs.
tkent: review-
Patch, remove "htmlFor" from parseMappedAttribute. (Comment #6)
none
Update Visual Studio project file.
none
Patch, try again on the windows bot
tkent: review-
Remove un-needed argument tkent: review+

Description Yael 2010-05-06 18:51:01 PDT
The elements specified http://dev.w3.org/html5/spec/Overview.html#category-label should support the labels attribute.
Comment 1 Yael 2010-05-18 20:45:20 PDT
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.
Comment 2 Kent Tamura 2010-05-20 01:23:48 PDT
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.
Comment 3 Yael 2010-05-21 14:13:37 PDT
Created attachment 56745 [details]
Patch addressing issues found in comment #2.
Comment 4 WebKit Review Bot 2010-05-21 14:17:45 PDT
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.
Comment 5 Yael 2010-05-21 14:27:51 PDT
Created attachment 56747 [details]
Remove tabs.
Comment 6 Kent Tamura 2010-05-22 09:57:06 PDT
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.
Comment 7 Yael 2010-05-22 11:59:56 PDT
Created attachment 56783 [details]
Patch, remove "htmlFor" from parseMappedAttribute. (Comment #6)
Comment 8 WebKit Review Bot 2010-05-22 14:26:59 PDT
Attachment 56783 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2371041
Comment 9 Yael 2010-05-22 19:20:49 PDT
Created attachment 56805 [details]
Update Visual Studio project file.
Comment 10 Yael 2010-05-23 05:28:01 PDT
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 :-)
Comment 11 Yael 2010-05-23 05:30:45 PDT
Created attachment 56819 [details]
Patch, try again on the windows bot

No changes from previous patch, other than updaing my environment.
Comment 12 Yael 2010-05-23 17:11:33 PDT
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
Comment 13 Eric Seidel (no email) 2010-05-23 17:51:02 PDT
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).
Comment 14 Chris Jerdonek 2010-05-23 20:14:35 PDT
(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.
Comment 15 Yael 2010-05-24 04:30:01 PDT
(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).
Comment 16 Eric Seidel (no email) 2010-05-24 10:42:59 PDT
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.
Comment 17 Yael 2010-05-24 11:28:54 PDT
(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
Comment 18 Kent Tamura 2010-05-25 02:41:23 PDT
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.
Comment 19 Yael 2010-05-25 16:59:25 PDT
Created attachment 57054 [details]
Remove un-needed argument
Comment 20 Kent Tamura 2010-05-26 03:24:57 PDT
Comment on attachment 57054 [details]
Remove un-needed argument

OK.
Comment 21 Yael 2010-05-26 06:15:12 PDT
Committed r60232. <http://trac.webkit.org/changeset/60232>