Bug 41146 - Implement the .dataset DOM property
: Implement the .dataset DOM property
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To:
:
: HTML5, InRadar
:
: 32934
  Show dependency treegraph
 
Reported: 2010-06-24 04:17 PST by
Modified: 2010-06-30 22:29 PST (History)


Attachments
[WIP] Patch (48.24 KB, patch)
2010-06-29 16:08 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch (54.43 KB, patch)
2010-06-29 23:17 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
Patch 3 - More tests (61.39 KB, patch)
2010-06-30 13:01 PST, Sam Weinig
mitz: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-06-24 04:17:09 PST
HTML5 specifies a mechanism for custom HTML attributes (prefixed with data-) and DOM APIs to easily access the value of such attributes.

http://dev.w3.org/html5/spec/Overview.html#embedding-custom-non-visible-data

WebKit should support the .dataset DOM property such that one could easily and cleanly mark HTML elements with custom attributes.
------- Comment #1 From 2010-06-24 04:18:19 PST -------
rdar://problem/8126069
------- Comment #2 From 2010-06-29 16:08:51 PST -------
Created an attachment (id=60068) [details]
[WIP] Patch

I am throwing up my WIP patch so that I don't loose track of it.  Things that should be addressed before landing:

- The implementation of item() and contains() are unnecessarily slow and allocate an unnecessary String for the propertyNameMatchesAttributeName() check.
- I am not sure the behavior for an attribute called "data-" (nothing following the hyphen) is correct.  (With this patch, for <div data-="value">, div.dataset[""] == "value"). This should also be included in the test.
- I have not included build system changes for all the ports.
------- Comment #3 From 2010-06-29 23:17:26 PST -------
Created an attachment (id=60087) [details]
Updated patch

This is now ready for review.  propertyNameMatchesAttributeName() is still unnecessarily dumb, but this can be fixed in a follow up patch since it is merely an optimization.
------- Comment #4 From 2010-06-29 23:30:20 PST -------
Attachment 60087 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3313972
------- Comment #5 From 2010-06-30 00:16:19 PST -------
Attachment 60087 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3381024
------- Comment #6 From 2010-06-30 00:39:19 PST -------
Attachment 60087 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3372044
------- Comment #7 From 2010-06-30 08:44:34 PST -------
(In reply to comment #2)
> - I am not sure the behavior for an attribute called "data-" (nothing following the hyphen) is correct.  (With this patch, for <div data-="value">, div.dataset[""] == "value"). This should also be included in the test.

"Custom data attributes" (the ones that should be included in .dataset) must have at least one character after the hyphen.  First paragraph of the relevant section of the spec, http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#embedding-custom-non-visible-data

@data- isn't a valid custom data attribute, and so shouldn't be included.  It's just a random unstandardized attribute.
------- Comment #8 From 2010-06-30 08:49:45 PST -------
(In reply to comment #6)
> Attachment 60087 [details] [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/3372044

Nate, can you help Sam with the V8 bindings? This is cool stuff!
------- Comment #9 From 2010-06-30 09:04:26 PST -------
(In reply to comment #7)
> (In reply to comment #2)
> > - I am not sure the behavior for an attribute called "data-" (nothing following the hyphen) is correct.  (With this patch, for <div data-="value">, div.dataset[""] == "value"). This should also be included in the test.
> 
> "Custom data attributes" (the ones that should be included in .dataset) must have at least one character after the hyphen.  First paragraph of the relevant section of the spec, http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#embedding-custom-non-visible-data
> 
> @data- isn't a valid custom data attribute, and so shouldn't be included.  It's just a random unstandardized attribute.

Interesting. In the section on "The algorithm for getting the list of name-value pairs" just below that it states "For each content attribute on the element whose first five characters are the string "data-" and whose remaining characters (if any)".
------- Comment #10 From 2010-06-30 09:05:45 PST -------
(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #2)
> > > - I am not sure the behavior for an attribute called "data-" (nothing following the hyphen) is correct.  (With this patch, for <div data-="value">, div.dataset[""] == "value"). This should also be included in the test.
> > 
> > "Custom data attributes" (the ones that should be included in .dataset) must have at least one character after the hyphen.  First paragraph of the relevant section of the spec, http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#embedding-custom-non-visible-data
> > 
> > @data- isn't a valid custom data attribute, and so shouldn't be included.  It's just a random unstandardized attribute.
> 
> Interesting. In the section on "The algorithm for getting the list of name-value pairs" just below that it states "For each content attribute on the element whose first five characters are the string "data-" and whose remaining characters (if any)".

Ah, that's a spec bug.  I'll get it corrected one way or the other.
------- Comment #11 From 2010-06-30 09:06:36 PST -------
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (In reply to comment #2)
> > > > - I am not sure the behavior for an attribute called "data-" (nothing following the hyphen) is correct.  (With this patch, for <div data-="value">, div.dataset[""] == "value"). This should also be included in the test.
> > > 
> > > "Custom data attributes" (the ones that should be included in .dataset) must have at least one character after the hyphen.  First paragraph of the relevant section of the spec, http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#embedding-custom-non-visible-data
> > > 
> > > @data- isn't a valid custom data attribute, and so shouldn't be included.  It's just a random unstandardized attribute.
> > 
> > Interesting. In the section on "The algorithm for getting the list of name-value pairs" just below that it states "For each content attribute on the element whose first five characters are the string "data-" and whose remaining characters (if any)".
> 
> Ah, that's a spec bug.  I'll get it corrected one way or the other.

Thanks Tab.
------- Comment #12 From 2010-06-30 10:50:26 PST -------
It's not a spec bug — don't confuse authoring conformance criteria with implementation conformance criteria. :-) Only parts of the spec that say what implementors must do are relevant here. Parts of the spec that say what is valid are irrelevant to implementors.
------- Comment #13 From 2010-06-30 12:48:42 PST -------
(In reply to comment #12)
> It's not a spec bug — don't confuse authoring conformance criteria with implementation conformance criteria. :-) Only parts of the spec that say what implementors must do are relevant here. Parts of the spec that say what is valid are irrelevant to implementors.

While it's not a direct contradiction, it seems kind of pointless to support "data-"[sic] in the implementation when it's not valid. Since this is a new feature, we could make what is supported closer to what is valid, unless there is a specific reason for this to be a special case.
------- Comment #14 From 2010-06-30 12:49:57 PST -------
On the other hand, I guess the "XML-compatible" and "no capital letters" constraints on validity are not ones that make sense to enforce in the implementation.
------- Comment #15 From 2010-06-30 12:54:53 PST -------
(In reply to comment #14)
> On the other hand, I guess the "XML-compatible" and "no capital letters" constraints on validity are not ones that make sense to enforce in the implementation.

The spec *does* mandate enforcing the "no capital letters" thing, though.  I can understand if enforcing XML-compatibility is overkill, but if we're enforcing no-capital-letters, surely we can enforce must-have-at-least-one-letter too, and bring author conformance and impl conformance closer together.

I don't see a good reason for this gap, seeing as there's no legacy content forcing us to process "data-" attributes even though they're invalid.
------- Comment #16 From 2010-06-30 12:56:16 PST -------
I'm happy to make it getting or setting the empty string throw a SYNTAX_ERR if you want, the only reason I hadn't done that was that it was easier to not bother. However, the total time now spent on discussing this exceeds the time it would have saved, so nevermind! :-) If you decide that's the way to go, just implement it that way, and mail the list saying you've done so and asking for the spec to change. I'll just stick a new step at the top of each of the three algorithms that raises SYNTAX_ERR if the input key is "".

The capital letter thing is enforced because of the mapping of capital letters to hyphen-lowercase, which we added recently based on author feedback.
------- Comment #17 From 2010-06-30 13:01:26 PST -------
Created an attachment (id=60143) [details]
Patch 3 - More tests
------- Comment #18 From 2010-06-30 16:17:37 PST -------
Landed in r62215.  I left it with the spec'd behavior of allowing dataset[""] to match data-.
------- Comment #19 From 2010-06-30 16:50:36 PST -------
http://trac.webkit.org/changeset/62215 might have broken Qt Linux Release
------- Comment #20 From 2010-06-30 22:19:02 PST -------
(In reply to comment #19)
> http://trac.webkit.org/changeset/62215 might have broken Qt Linux Release

06/30/10 16:15:15 (6 hours ago)

This appears to have broken fast/dom/Window/window-postmessage-clone.html on every platform.

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r62215%20(16637)/fast/dom/Window/window-postmessage-clone-pretty-diff.html
------- Comment #21 From 2010-06-30 22:20:02 PST -------
I think this broke a post-message test?

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r62222%20(16640)/fast/dom/Window/window-postmessage-clone-diffs.txt

--- /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug-tests/build/layout-test-results/fast/dom/Window/window-postmessage-clone-expected.txt    2010-06-30 18:27:21.000000000 -0700
+++ /Volumes/Big/WebKit-BuildSlave/leopard-intel-debug-tests/build/layout-test-results/fast/dom/Window/window-postmessage-clone-actual.txt    2010-06-30 18:27:21.000000000 -0700
@@ -2,7 +2,7 @@
 PASS: 'postMessage(cyclicObject)' threw TypeError: Cannot post cyclic structures.
 PASS: 'postMessage(cyclicArray)' threw TypeError: Cannot post cyclic structures.
 PASS: 'postMessage(reallyDeepArray)' threw RangeError: Maximum call stack size exceeded.
-PASS: 'postMessage(window)' threw TypeError: Cannot post cyclic structures.
+PASS: 'postMessage(window)' threw TypeError: Type error
 PASS: eventData is null of type object
 PASS: eventData is undefined of type undefined
 PASS: eventData is 1 of type number
------- Comment #22 From 2010-06-30 22:20:29 PST -------
(Oops, Adam had the same thought it seems.)
------- Comment #23 From 2010-06-30 22:29:06 PST -------
Landed new expectations in http://trac.webkit.org/changeset/62224

Insert nastygram about building and testing your patch before landing or at least watching the bots or listening to SheriffBot.  Please consider using the commit-queue to land your patches if you don't have time to build and run all the tests.