Bug 41146 – Implement the .dataset DOM property

• Component: DOM
• Status: RESOLVED FIXED

[Antoine Quint]

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.

[Antoine Quint]

rdar://problem/8126069

[Sam Weinig]

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.

[Sam Weinig]

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.

[Early Warning System Bot]

Attachment 60087 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3313972

[WebKit Review Bot]

Attachment 60087 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3381024

[WebKit Review Bot]

Attachment 60087 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3372044

[Tab Atkins]

(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.

[Dimitri Glazkov (Google)]

(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!

[Sam Weinig]

(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)".

[Tab Atkins]

(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.

[Sam Weinig]

(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.

[Ian 'Hixie' Hickson]

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.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

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.

[Tab Atkins]

(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.

[Ian 'Hixie' Hickson]

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.

[Sam Weinig]

Created attachment 60143 [details]
Patch 3 - More tests

[Sam Weinig]

Landed in r62215.  I left it with the spec'd behavior of allowing dataset[""] to match data-.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62215 might have broken Qt Linux Release

[Adam Barth]

(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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

(Oops, Adam had the same thought it seems.)

[Adam Barth]

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.