Bug 41146 - Implement the .dataset DOM property
: Implement the .dataset DOM property
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Macintosh Mac OS X 10.6
: P2 Normal
Assigned To: Nobody
: HTML5, InRadar
Depends on:
Blocks: 32934
  Show dependency treegraph
 
Reported: 2010-06-24 04:17 PDT by Antoine Quint
Modified: 2010-06-30 22:29 PDT (History)
15 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2010-06-24 04:17:09 PDT
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 Antoine Quint 2010-06-24 04:18:19 PDT
rdar://problem/8126069
Comment 2 Sam Weinig 2010-06-29 16:08:51 PDT
Created attachment 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 Sam Weinig 2010-06-29 23:17:26 PDT
Created attachment 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 Early Warning System Bot 2010-06-29 23:30:20 PDT
Attachment 60087 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3313972
Comment 5 WebKit Review Bot 2010-06-30 00:16:19 PDT
Attachment 60087 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3381024
Comment 6 WebKit Review Bot 2010-06-30 00:39:19 PDT
Attachment 60087 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3372044
Comment 7 Tab Atkins 2010-06-30 08:44:34 PDT
(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 Dimitri Glazkov (Google) 2010-06-30 08:49:45 PDT
(In reply to comment #6)
> Attachment 60087 [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 Sam Weinig 2010-06-30 09:04:26 PDT
(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 Tab Atkins 2010-06-30 09:05:45 PDT
(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 Sam Weinig 2010-06-30 09:06:36 PDT
(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 Ian 'Hixie' Hickson 2010-06-30 10:50:26 PDT
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 Maciej Stachowiak 2010-06-30 12:48:42 PDT
(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 Maciej Stachowiak 2010-06-30 12:49:57 PDT
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 Tab Atkins 2010-06-30 12:54:53 PDT
(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 Ian 'Hixie' Hickson 2010-06-30 12:56:16 PDT
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 Sam Weinig 2010-06-30 13:01:26 PDT
Created attachment 60143 [details]
Patch 3 - More tests
Comment 18 Sam Weinig 2010-06-30 16:17:37 PDT
Landed in r62215.  I left it with the spec'd behavior of allowing dataset[""] to match data-.
Comment 19 WebKit Review Bot 2010-06-30 16:50:36 PDT
http://trac.webkit.org/changeset/62215 might have broken Qt Linux Release
Comment 20 Adam Barth 2010-06-30 22:19:02 PDT
(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 Eric Seidel 2010-06-30 22:20:02 PDT
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 Eric Seidel 2010-06-30 22:20:29 PDT
(Oops, Adam had the same thought it seems.)
Comment 23 Adam Barth 2010-06-30 22:29:06 PDT
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.