Closed Bug 921834 Opened 6 years ago Closed 3 years ago

The "dataset" attribute on SVGElement is undefined

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: alex.gragnano, Assigned: bzbarsky)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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
Same problem on 24.0 branch
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/
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
reply from www-svg@w3.org:

---
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'?
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Anyway, please raise a spec discussion.
Anne, is this something that's going to get pushed up to Element?
Component: Untriaged → DOM
Flags: needinfo?(annevk)
Product: Firefox → Core
Summary: Undefined dataset on SVGElement → The "dataset" attribute on SVGElement is undefined
People have been asking about that. We could do it I suppose. Does that mean data-*="" are also "super" global attributes?
Flags: needinfo?(annevk)
Seems pretty fishy to me, but is that what Blink/WebKit implement?
Flags: needinfo?(annevk)
Per http://software.hixie.ch/utilities/js/live-dom-viewer/saved/2889 it seems like they do.
Flags: needinfo?(annevk)
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
OS: Mac OS X → All
Hardware: x86 → All
Version: 23 Branch → Trunk
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?
Flags: needinfo?(annevk)
> Blink also doesn't support it on SVG elements,

Hopefully that will change. See linked Chrome bug in the "See Also" field.
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Seems fine, see https://github.com/whatwg/dom/issues/280 for standards discussion around this.
Flags: needinfo?(annevk)
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.
Attachment #8779877 - Flags: review?(bkelly) → review+
> 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.
(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.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da7e46336548
Add support for .dataset on SVGElement.  r=bkelly
https://hg.mozilla.org/mozilla-central/rev/da7e46336548
Status: ASSIGNED → RESOLVED
Closed: 6 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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!
"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.
Please make sure you are using the latest version (currently 51.0.1) before making a comment.
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!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.