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

dangerouslySetInnerHTML not working in SVG elements on Edge browser #7506

Closed
jasford opened this issue Aug 15, 2016 · 13 comments
Closed

dangerouslySetInnerHTML not working in SVG elements on Edge browser #7506

jasford opened this issue Aug 15, 2016 · 13 comments

Comments

@jasford
Copy link

jasford commented Aug 15, 2016

This appears to be a bug in React with the Microsoft Edge browser. This was previously a bug in older Internet Explorer browsers as well, but appears to have been fixed by #6982 – unfortunately the fix did not resolve the issue in Edge.

I modified the codepen from the original issue to use React 15.3.0, which includes the above-referenced pull request, and demonstrates the bug in Edge:
http://codepen.io/anon/pen/QEYoLV

Here is the output of that Codepen in IE 11 – this is the expected behavior as innerHTML is not available in Internet Explorer, yet React 15.3 is still able to function:

image

And here is the output of the same Codepen in Edge 38:

image

The problem seems to be that Edge has innerHTML, so it does not use the IE workaround code, yet Edge does not appear to be refreshing the DOM after updating – at least I can see the SVG contents added to the DOM using Inspect Element, but they are not visible in the browser.

@aweary
Copy link
Contributor

aweary commented Aug 16, 2016

Thanks for the report @jasford, at first glance it seems like a potential regression in Edge. I tested in Microsoft Edge 34.14300.1000.0 and it rendered successfully:

screen shot 2016-08-15 at 11 25 12 pm

Do you have the exact build number for the Edge version you're using, just in case? I'm trying to update to 38 to verify but it's taking its sweet time

@jasford
Copy link
Author

jasford commented Aug 23, 2016

Looks like I took that screenshot from 38.14393.0.0

@lgra
Copy link

lgra commented Aug 30, 2016

I've seen this issue too.
Wasn't able to find a clever test to detect when to use the workaround except adding a browser detection (window.navigator.userAgent.indexOf('Edge/') > 0). Not really proud of it, but in the meantime, it's working.

@aweary
Copy link
Contributor

aweary commented Aug 30, 2016

I think Edge regressed somewhere between 34 and 38. I'm going to see if I can verify that with someone from the Edge team.

@zpao
Copy link
Member

zpao commented Aug 30, 2016

FWIW IE11 is actually still broken too. Need to fix that.

@patrickkettner - perhaps you know somebody who can look into this from the Edge side since it seems like there's a regression on that side of things.

@patrickkettner
Copy link

Happy to. Will dive in tomorrow morning

On Aug 30, 2016 4:38 PM, "Paul O’Shannessy" notifications@github.com
wrote:

FWIW IE11 is actually still broken too. Need to fix that.

@patrickkettner https://github.com/patrickkettner - perhaps you know
somebody who can look into this from the Edge side since it seems like
there's a regression on that side of things.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7506 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAcaBj0xFe3_G5aGx7EVU1sOazb-6qSYks5qlF9WgaJpZM4Jkos2
.

@patrickkettner
Copy link

So, a few things.

  1. Edge's versions are annoyingly confusing because of reasons. We have seperate numbers for the Edge app, and EdgeHTML. The difference being the chrome around the browser (url bar, bookmark sync, etc - thats Edge app), and the thing that actually renders the code (thats EdgeHTML). Here is a blog post with more explanation.
    Basically - the latest release is Edge 14, not 38
  2. There was a regression :[
  3. But! this was actually fixed in our latest builds

So what to do depends on what y'all want to support, @zpao. You could have a super small feature detect ('innerHTML' in SVGElement.prototype), and fallback to something like DOMParser and setChildren on older/broken versions.

Happy to write anything or create a PR, just lemme know what you would like

@zpao
Copy link
Member

zpao commented Sep 2, 2016

Thanks for getting back to us @patrickkettner! Totally get confusing versions (Gecko & Firefox versioning being independent for so long taught me things).

Couple followup questions:

  1. Does the regression range include public versions or just nightly/beta builds?
  2. Did the fix make it to stable versions yet?
  3. Assuming the regression was a in a stable version, how aggressive is the uptake on new versions?

The set of people affected here is really small, especially if its already fixed in Edge so I'm just trying to figure out if it's worth taking on another line of code that really only needs to live for a month or 2 but will realistically still be there in 2022. Like you said, we have the ability to handle this thanks to other browsers so it's not a huge deal.

@mariuszdev
Copy link

Try not using dangerouslySetInnerHTML. It'll work if you change xlink:href to xlinkHref.

@aweary
Copy link
Contributor

aweary commented Sep 29, 2016

ping @patrickkettner! no rush, but when you get a chance could you take a look at @zpao's follow up questions? Thanks 😄

@patrickkettner
Copy link

super sorry @zpao @aweary, last update got lost in my inbox

  1. it would have shipped to public versions
  2. no, it has not
  3. Uptake has been great on new releases, since edge is tied to windows updates

it would be on the order of months, almost certainly less than a year that this would affect Edge users on stable builds

@olegafx
Copy link

olegafx commented Sep 30, 2016

Got the same issue. @Pilaas's advice works as expected, thanks!

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2017

Seems like it was a temporary issue and so there's nothing to do on our side.

@gaearon gaearon closed this as completed Oct 4, 2017
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

No branches or pull requests

8 participants