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

fixing IE 11 crash on multiple selectMenu #1338

Closed
wants to merge 4 commits into from

Conversation

vparpoil
Copy link
Contributor

overriding redraw by render function + applying filters manually

overriding redraw by render function + applying filters manually
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Sep 13, 2017

It's weird that this is necessary, because render calls redraw under the hood. Could you try

_chart._doRedraw = _chart._doRender;

instead?

EDIT: no, of course that would just go into an infinite loop. Hrrrmmm, unclear why render would not select items, since it calls redraw.

I think we need a more precise fix, so I don't think I'll merge this as-is. But I'm really glad we have this workaround for anyone who is struggling with the IE bug. Thanks @vparpoil!

@vparpoil
Copy link
Contributor Author

@gordonwoodhull Thanks for your remarks, I updated the code for a cleaner fix. Do you think this can be merged ?

@gordonwoodhull
Copy link
Contributor

That looks really good, thank you!

The only thing I'd still like to see is a check for IE on the redraw = render line. Because this will be slightly less efficient.

Unfortunately (or fortunately ;) I don't think we have any browser checks in dc.js currently, so I don't know the best technique.

In another project we used some checks based on the comprehensive detection documented here:

https://codepen.io/gapcode/pen/vEJNZN

Has anyone tested this on IE<11? I'd be inclined to just detect Trident in the user agent and call it a day.

@vparpoil
Copy link
Contributor Author

Thanks for your feedback
IE 10 and IE 9 don't have the crash issue. Lower versions don't display anything on the dc.js example for select.
I tested the proposed fix on IE 10 and 9, it behaves as expected (but the fix is not needed since these version doesn't have the issue)

The codepen you gave detects IE 11 and IE 10. Lower versions are not supported by codepen but the code in it is still valid (see here for a list of IE user agents)

I updated the PR to add the test on Trident, I hope this fits your expectations :)

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Sep 15, 2017
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Sep 15, 2017

This looks excellent, thank you for iterating on this! I'll just confirm if automated testing on Win8 helps any, and then merge this for 2.0.*

Duh, there was no selectMenu in 2.0 - this will go into 2.1.9 only.

@gordonwoodhull gordonwoodhull modified the milestones: v2.0, v2.1 Sep 18, 2017
@gordonwoodhull
Copy link
Contributor

Apologies for the delay. I had hoped to repro the problem in automated testing but I think this would require much more extensive tests than we currently have (like duplicating the example into a test). This is doable but I don't have the time for it, so I've simply cut the release 2.1.9.

Thanks @vparpoil!

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 this pull request may close these issues.

None yet

2 participants