Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot set data-* attribute on VirtualDOM. #28

Open
Bigdragon13th opened this issue Jun 20, 2016 · 5 comments · May be fixed by #29
Open

Cannot set data-* attribute on VirtualDOM. #28

Bigdragon13th opened this issue Jun 20, 2016 · 5 comments · May be fixed by #29

Comments

@Bigdragon13th
Copy link

Bigdragon13th commented Jun 20, 2016

Hi,

I try to set data-* attribute to element with no luck on virtual-dom

For example:
This working:

hx`<a href="${pagination.links.next}">Next</a>`

But this is not:

hx`<a href="#" data-href="${pagination.links.next}">Next</a>`

I've googled and found this https://github.com/Matt-Esch/virtual-dom/blob/master/docs/vnode.md#custom-attributes-data-
which maybe related to my issue.

@Bigdragon13th
Copy link
Author

Bigdragon13th commented Jun 22, 2016

For anyone who may facing the same issue, I found an easy workaround by assign data-* as a properties.attributes object after initialise a hx.

let element = hx`<a class="next" href="#">Next</a>`;
element.properties.attributes = {
    "data-href": pagination.links.next
}; 

@tswaters
Copy link

tswaters commented Jul 9, 2016

Interesting...

in #16 a test was added with data-* attributes, but I think the only reason this test is passing is because toString in min-document includes stray data-* attributes in the output... when returned as a dom node by a browser, (at least in Chrome, encountered this in an electron app), data attributes are missing.

There appears to be special handling for items in properties.attributes whereby they will get set via setAttribute. This would, I imagine, have the effect of setting the data-* attribute properly on the returned DOM node.

To fix this, when the tree is being populated with attributes: 1, 2 and 3; it needs to check if this is a data-* attribute and if so add it it to attributes and not on the root of the object. I can submit a PR for this.

tswaters added a commit to tswaters/hyperx that referenced this issue Jul 9, 2016
@tswaters tswaters linked a pull request Jul 9, 2016 that will close this issue
@Bigdragon13th
Copy link
Author

Bigdragon13th commented Jul 14, 2016

Thanks @tswaters for looking at this!

I've found another issue that seems related to this issue when trying to assign a "style" attribute like:

hx`<div class="box" style="background-color: red"></div>` 

This working fine in most browser except for MS Edge and Safari (didn't test on IE11 yet).

And stated in the doc: https://github.com/Matt-Esch/virtual-dom/blob/master/docs/vnode.md#propertiesstyle-vs-propertiesattributesstyle

properties.style expects an object, while properties.attributes.style expects a string.

So, to workaround this, I have to do something like:

const styleObj = {
    backgroundColor: "red"
};
hx`<div class="box" style=${styleObj}>`;

@arturi
Copy link

arturi commented Aug 12, 2016

This is an issue with aria (and thus accessibility) too, I’m currently struggling with it: since I have a nested tree of elements that I want to put attributes like aria-hidden on. Works fine with bel, but not virtual-dom.

Maybe we could really put this behind a supportVDomAttributes flag, as suggested in #29, or something?

return html`<div class="Uppy UppyTheme--default UppyDashboard ${isTouchDevice ? 'Uppy--isTouchDevice' : ''}"
                 aria-hidden="${modal.isHidden}"
                 aria-label="Uppy Dialog Window (Press escape to close)"
                 role="dialog">
    <div class="UppyDashboard-inner" tabindex="0">
      ...
    </div>
</div>

All attributes — aria, role, tabindex — except for class are missing from the resulting DOM.

tswaters added a commit to tswaters/hyperx that referenced this issue Sep 3, 2016
- enabled by passing {vdom: true} flag to the hyperx contructor options

- fixes data-*, aria-* and style as a string.  vdom expects these to be
  under an 'attributes' property under props and will gladly ignore them
  otherwise

fixes choojs#28
tswaters added a commit to tswaters/hyperx that referenced this issue Sep 3, 2016
- enabled by passing {vdom: true} flag to the hyperx contructor options

- fixes data-*, aria-* and style as a string.  vdom expects these to be
  under an 'attributes' property under props and will gladly ignore them
  otherwise

fixes choojs#28
tswaters added a commit to tswaters/hyperx that referenced this issue Sep 3, 2016
- enabled by passing {vdom: true} flag to the hyperx contructor options

- fixes data-*, aria-* and style as a string.  vdom expects these to be
  under an 'attributes' property under props and will gladly ignore them
  otherwise

fixes choojs#28
tswaters added a commit to tswaters/hyperx that referenced this issue Sep 3, 2016
- enabled by passing {vdom: true} flag to the hyperx contructor options

- fixes data-*, aria-* and style as a string.  vdom expects these to be
  under an 'attributes' property under props and will gladly ignore them
  otherwise

fixes choojs#28
tswaters added a commit to tswaters/hyperx that referenced this issue Sep 3, 2016
- enabled by passing {vdom: true} flag to the hyperx contructor options

- fixes data-*, aria-* and style as a string.  vdom expects these to be
  under an 'attributes' property under props and will gladly ignore them
  otherwise

fixes choojs#28
@tswaters
Copy link

tswaters commented Sep 3, 2016

Sorry for all the noise there, pushed up some force commits after I noticed some errors...

The PR has been adjusted to use a flag, {vdom: true} - this will fix data-, aria- and style being passed as a string. I noticed that tabindex and role both seemed to work without any explicit modifications, so maybe that was a bug upstream that is now fixed.

That reminds me - on the note of upstream bugs. Note that the tests relating to style as a string won't pass unless min-document v2.18.1 is present, due to this bug. This matches the version in this package.json so it should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants