921834 - The "dataset" attribute on SVGElement is undefined

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[alex.gragnano]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.76 Safari/537.36

Steps to reproduce:

SVGElement:

<g class="button" data-key="123">...</g>



Javascript:

console.log(document.querySelector('.button[data-key="123"]'));
// returns SVGElement

console.log(document.querySelector('.button[data-key="123"]').dataset);
// returns undefined


Actual results:

dataset for SVGElement is undefined


Expected results:

dataset should be defined by data-* properties

Comment 1

[alex.gragnano]

Same problem on 24.0 branch

Comment 2

[Robert Longson [:longsonr]]

dataset is a propery of HTML elements only: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement

You'd need to get a specification change for this: try the w3c SVG group to begin http://lists.w3.org/Archives/Public/www-svg/

Comment 3

[alex.gragnano]

reply from [e-mail]:

---
WebKit and Blink indeed define it for Element and not for HTMLElement. Not sure of it was done on purpose or by mistake. We should include the relevant WGs to discuss if it is worth adding to Element itself. I assume this is DOM and HTML?

Greetings,
Dirk
---

So, my request should be 'Can you move dataset implementation from HTMLElement to Element super class'?

Comment 4

[Masatoshi Kimura [:emk]]

Anyway, please raise a spec discussion.

Comment 5

[alex.gragnano]

My job stop here sorry. If Mozilla waiting for his users to growing up, so stay down.
Thank you for your time.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Anne, is this something that's going to get pushed up to Element?

Comment 7

[Anne (:annevk)]

People have been asking about that. We could do it I suppose. Does that mean data-*="" are also "super" global attributes?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Seems pretty fishy to me, but is that what Blink/WebKit implement?

Comment 9

[Anne (:annevk)]

Per http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2889 it seems like they do.

Comment 10

[Chris Rebert]

The SVGElement.dataset property and de jure data-* attribute support were added to the current SVG2 spec draft circa January:
* https://github.com/w3c/svgwg/commit/1cb4ee9e165b3d777d33828da8fa757e67d019ff
* https://svgwg.org/svg2-draft/types.html#__svg__SVGElement__dataset
* http://stackoverflow.com/a/30519648

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK.  I checked the link from comment 9 in Blink and it does _not_ support .dataset on random elements (though WebKit still does).  Blink also doesn't support it on SVG elements, per <http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4370>, but does support it on HTML elements.

According to that same test, Edge also does not support .dataset on random elements or SVG elements, but does support it on HTML elements.

Given that, seems to me like we could add it to SVGElement per comment 10, but should NOT put it on Element.  Anne, any obvious objections?

Comment 12

[Chris Rebert]

> Blink also doesn't support it on SVG elements,

Hopefully that will change. See linked Chrome bug in the "See Also" field.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Add support for .dataset on SVGElement

Comment 14

[Anne (:annevk)]

Seems fine, see https://github.com/whatwg/dom/issues/280 for standards discussion around this.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8779877 [details] [diff] [review]
Add support for .dataset on SVGElement

Review of attachment 8779877 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.  Thanks!

::: dom/base/Element.h
@@ +1122,5 @@
> +   * Helpers for .dataset.  This is implemented on Element, though only some
> +   * sorts of elements expose it to JS as a .dataset property
> +   */
> +  // Getter, to be called from bindings.
> +  already_AddRefed<nsDOMStringMap> Dataset();  

nit: trailing whitespace

::: dom/html/nsDOMStringMap.h
@@ +10,5 @@
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsTArray.h"
>  #include "nsString.h"
>  #include "nsWrapperCache.h"
> +#include "mozilla/dom/Element.h"

Can't this just be a forward declaration?  The destructor is in the cpp file so you shouldn't need the full class def in order to have a RefPtr<Element>.

::: dom/webidl/SVGElement.webidl
@@ +15,5 @@
>  
>    [Constant]
>    readonly attribute SVGAnimatedString className;
> +  [SameObject]
> +  readonly attribute DOMStringMap dataset;

nit: Can we put the [SameObject] annotation on the same line?  Then it would be a direct copy/paste from the spec.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Can't this just be a forward declaration?

No, because the GetParentObject function depends on it being clear to the compiler that mElement can cast to nsINode*.  We _could_ out-of-line GetParentObject, forward-declare Element here, and make sure to include nsINode.h so that bindings can work with our parent object.  I'm not sure it's that much of a win.

Fixed the other two nits.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #16)
> No, because the GetParentObject function depends on it being clear to the
> compiler that mElement can cast to nsINode*.  We _could_ out-of-line
> GetParentObject, forward-declare Element here, and make sure to include
> nsINode.h so that bindings can work with our parent object.  I'm not sure
> it's that much of a win.

Sounds reasonable.  I'm fine with just leaving it.  I thought it was maybe left over some legacy thing that used to be inlined before.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da7e46336548
Add support for .dataset on SVGElement.  r=bkelly

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da7e46336548

Comment 20

[Eric Shepherd [:sheppy]]

I’ve updated the following pages:

https://developer.mozilla.org/en-US/docs/Web/API/SVGElement
https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Using_data_attributes (added mentions that SVG 2 supports this concept, without any detail)
https://developer.mozilla.org/en-US/Firefox/Releases/51#SVG

And I’ve added:

https://developer.mozilla.org/en-US/docs/Web/API/SVGElement/dataset
https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/data-*

This should be done now, but if anyone wants to double-check it that would be very much appreciated. If you see issues, please correct them or change this bug back to dev-doc-needed and note in a comment what’s wrong. Thanks!

Comment 21

[yair even or]

"50.1.0" here and getting the same "undefined" for "dataset" on a "g" element.
I've made an example page:

http://jsbin.com/goyiriq/edit?html,js,output

Thank you. <3 FF.

Comment 22

[Masatoshi Kimura [:emk]]

Please make sure you are using the latest version (currently 51.0.1) before making a comment.

Comment 23

[yair even or]

Sorry about that! I've stopped upgrading FF completely because Firebug isn't functioning on newer versions, which is a big NO for me. been using firebug since day one, and nothing is as good. Thanks for the quick response non-the-less!