c3js / c3

:bar_chart: A D3-based reusable chart library
http://c3js.org
MIT License
9.35k stars 1.39k forks source link

SvgPathSeg is removed in chrome 48 #1465

Closed Karabur closed 8 years ago

Karabur commented 8 years ago

According to https://www.chromestatus.com/features#pathseg this interface was a part of svg1.1, but was removed. Soon charts will be broken in chrome. And perhaps it will be removed from other browsers too. Library should use other way to avoid reference to pathSegList property.

aendra-rininsland commented 8 years ago

Good catch, @Karabur! Confirming as a bug — Chrome Canary is now throwing a TypeError on the bar chart example.

This needs to be updated in the following places:

martingraham commented 8 years ago

Could this polyfill be bundled into c3? --> https://github.com/progers/pathseg

I suppose we could also add it individually to our projects in the meantime

aendra-rininsland commented 8 years ago

@martingraham Ugh, would really like to not bundle another polyfill unless totally necessary. SvgPathSeg is only used three times in the entire codebase, would prefer just finding an alternative implementation.

GerHobbelt commented 8 years ago

No polyfill required; the SVG path list is only used in a context where a straight bar block (rectangle) is expected (segment 0 and segment 1 are inspected in there to derive a corner coordinate); the above pullreq code keeps the existing code for those who still have SVG pathSegList support, while the alternative code works or Chrome Canary et al - at least it works for me. ;-)

aendra-rininsland commented 8 years ago

@GerHobbelt You, good sir, are a gentleman and a scholar. :+1: :tada:

Will review PR first opportunity. Many thanks!

aendra-rininsland commented 8 years ago

Apologies for the delay, will try to get a chance to look at this this week.

GerHobbelt commented 8 years ago

No worries. ;-) On Dec 1, 2015 1:39 PM, "Ændrew Rininsland" notifications@github.com wrote:

Apologies for the delay, will try to get a chance to look at this this week.

— Reply to this email directly or view it on GitHub https://github.com/masayuki0812/c3/issues/1465#issuecomment-160955331.

tracker-common commented 8 years ago

Hey @aendrew, just wanted to touch base and see if this bug is still in progress.

Thanks!

bsarkadi commented 8 years ago

Hi @aendrew, I'd also like to ask when the fix for this is expected to be released. Thanks!

Paulsky commented 8 years ago

Hello,

If anybody noticed; Chrome 48 was released yesterday: http://googlechromereleases.blogspot.be/2016/01/stable-channel-update_20.html. I've just updated Chrome, because I wanted to check if the click event (and my drilldown function) is still working. But, I can confirm that this does not work anymore. I'd also would like to ask when a fix for this is going to be released. Thank you in advance!

bottomline commented 8 years ago

We tried the other PR mentioned on this thread, but found it broke onclick handlers. We've instead created this PR: https://github.com/masayuki0812/c3/pull/1552

ghost commented 8 years ago

+1 SvgPathSeg is removed in chrome 48

Sweterings commented 8 years ago

Are there any updates on this? It has been marked as "in progress", but it has been for nearly 2 months now.

ghost commented 8 years ago

We look forward error correction!

rannmann commented 8 years ago

PR #1552 and #1564 both address this problem. I haven't tested either since I don't have Chrome 48 yet.

Can anyone confirm their stability?

Here's a link to a non-working version in production (tooltips missing). https://opskins.com/?loc=shop_view_item&item=9435946

LoneRifle commented 8 years ago

The gist for #1564 is known to work, since that involves dropping the referenced polyfill for the missing APIs (See comment 3 on this thread).

IMHO, #1564 should be discarded in favour of #1552, which in turn is identical to #1466. The lattermost PR has no polyfill, which is an approach that the team appears to favour more.

smithd1 commented 8 years ago

Hi, can anyone confirm that the #1564 fix works with Chrome 48? So far my tests fail. Thanks.

aendra-rininsland commented 8 years ago

@smithd1 I just ran through the test suite in Chrome 48 using the current HEAD in the dev branch, so can confirm it works.

Will keep this issue open until the above fix is merged into master. It's a known working fix so we'll keep it, perhaps later replacing it with #1466 next release.

smithd1 commented 8 years ago

@aendrew Thanks so much for checking. I discovered that I made a mistake on my end; it's working with Chrome 48 now. Thanks again!

masayuki0812 commented 8 years ago

Thank you for working on this issue. I merged dev to the release branch release/v.0.4.11, then let mark resolved maybe and let close when it's released as anedrew said.

masayuki0812 commented 8 years ago

This has been merged into master and releases as 0.4.11. Thank you!