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

goog.dom.dataset.set does not work on IE11 sometimes #396

Closed
liolick opened this issue Jan 13, 2015 · 9 comments
Closed

goog.dom.dataset.set does not work on IE11 sometimes #396

liolick opened this issue Jan 13, 2015 · 9 comments

Comments

@liolick
Copy link

liolick commented Jan 13, 2015

IE11 has support of 'dataset' property on Element.
But sometimes it is not working properly, while old setAttribute works fine.
goog.dom.dataset.set uses 'dataset' attribute first, so it is also working bad.

The case is:

  1. Have some CSS class, which uses data-attr:
    .inputHint:before
    {
    content: attr(data-content);
    }
  2. Change data-attr from js:
    var testSpan = document.getElementById("spanTest");
    test.testSpan['content'] = 'some val';
    // test.testSpan['content'] = '';
    Nothing changes in IE11 here.

Test: http://jsbin.com/pumejubowu

@Ubehebe
Copy link
Contributor

Ubehebe commented Jan 13, 2015

@liolick Interesting. I can confirm this is happening on IE11 (but not, for example, Chrome).

I'd like to get a better understanding of the IE11 bug before patching Closure. Stepping through your example in a debugger, the dataset API is always being used—it's just not propagating to the CSS for some reason.

A quick web search didn't turn up an existing IE bug. Could you dig around some more?

@nicksay
Copy link

nicksay commented Jan 21, 2015

This is an interesting problem. Assuming we force a repaint by toggling a unique generated CSS class on the element, we can work around the issue, but that may have an unwanted performance impact in applications that make heavy use of datasets. We will have to test the speed of doing that.

I think there are 3 solutions we can consider:

1: Check for IE via User Agent and toggle a class
This limits the impact of the change to just IE, but introduces an extra dependency on the User Agent code in goog.dom.dataset. Some of the extra dependency impact could be mitigated by setting compile-time flags for IE vs other browers.

2. Use a independent define and toggle a class
This limits the impact of the change to users who explicitly set a compile-time flag, but reduces the scope of the fix.

3. Add a documentation warning, do nothing and have users implement their own fix.
This introduces no overhead and limits the impact to those users relying on data attribute base CSS rule, but reduces the scope of the fix.

@nicksay
Copy link

nicksay commented Jan 22, 2015

After some investigation, toggling an unrelated unique class does not cause IE to propagate the dataset values, so options 1 and 2 are out. Doing nothing somewhat defeats the purpose of using a library, so option 3 is out. I think the best solution is:

4: Restrict IE to setAttribute and provide a @define to override

This returns functional behavior to IE. The performance hit is relatively minor and can be avoided if desired by overriding the define during compilation. In my opinion, those users seeking the highest possible performance will/should opt for using other non-DOM-based data storage anyway.

nicksay added a commit to nicksay/closure-library that referenced this issue Jan 22, 2015
In IE (up to and including IE 11), setting `element.dataset` in JS does not
propagate the values to CSS.  This breaks CSS expressions such as
`content: attr(data-content)` that would otherwise work.  Ensure that IE uses
the `element.setAttribute` code path instead.  Provide a define
(`ALWAYS_USE_DOM_STRING_MAP`) to override this behavior and always allow.

Closes google#396.
nicksay added a commit to nicksay/closure-library that referenced this issue Jan 22, 2015
In IE (up to and including IE 11), setting `element.dataset` in JS does not
propagate the values to CSS.  This breaks CSS expressions such as
`content: attr(data-content)` that would otherwise work.  Ensure that IE uses
the `element.setAttribute` code path instead.  Provide a define
(`ALWAYS_USE_DOM_STRING_MAP`) to override this behavior and always allow.

Closes google#396.
@jonathansampson
Copy link

I work on the Internet Explorer team and noticed this behavior some time back; recently I seeded Stack Overflow with a question and answer to assist others running into this issue.

We're aware of the issue and have a bug filed internally for tracking it. I'll add this Issue to that ticket as an indicator that this is impacting live sites.

With regards to the work-around, I'd suggest avoiding navigator.msPointerEnabled and instead (if you're going to take this route) look at window.PointerEvent. The former may not stick around much longer.

@nicksay
Copy link

nicksay commented Jan 28, 2015

@jonathansampson I was originally looking for a "current but not future" property in the hopes that the bug is fixed in IE by the time the property disappears. Do you have an expected relative timeline for the removal of msPointerEnabled vs fixing the dataset behavior?

@jonathansampson
Copy link

On internal builds we have already removed support for msPointerEnabled. We will likely see this change surface in the public before Windows 10 RTMs. We tweeted this news just now. The dataset issue is likely to take a bit longer given our present backlog for Project Spartan.

nicksay added a commit to nicksay/closure-library that referenced this issue Feb 4, 2015
In IE (up to and including IE 11), setting `element.dataset` in JS does not
propagate the values to CSS.  This breaks CSS expressions such as
`content: attr(data-content)` that would otherwise work.  Ensure that IE uses
the `element.setAttribute` code path instead.

Closes google#396.
@nicksay
Copy link

nicksay commented Feb 4, 2015

Thanks @jonathansampson. We'll avoid using the msPointerEnabled and similar workarounds in favor of a standard check.

@nanaze nanaze closed this as completed in ff4954c Feb 7, 2015
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.

5 participants