Use ResizeObserver and MutationObserver to detect detach/attach/resize #7104

kurkle · 2020-02-14T18:39:19Z

This currently adds about 5kb, probably due to the ResizeObserver polyfill.

https://codepen.io/kurkle/pen/PoqZQxR
https://codepen.io/kurkle/full/ZEGQwrZ
https://codepen.io/kurkle/pen/WNvwKOm
https://codepen.io/kurkle/full/xxGVJmL

Fixes: #5805 #6145 #5158 #3979

benmccann · 2020-02-14T19:30:21Z

I don't actually see ResizeObserver or MutationObserver in the diff anywhere. Are you sure they're included? Or let me know where they are if I'm missing them...

Does this add 5kb to the minified or unminified build? In either case, I'd be in favor of it to get the simplification of not needing to worry about the CSS extraction and inclusion anymore

kurkle · 2020-02-14T19:33:54Z

src/platform/platform.dom.js

benmccann · 2020-02-14T19:40:45Z

Ah, thanks. It looked like that file was deleted.

There's a few places with commented out code and debug logging, but other than that it generally looks good to me. I'm not the most familiar with the resize stuff, so I don't have much opinion on whether this would work better/worse, but I do like the idea of removing the CSS stuff

kurkle · 2020-02-14T19:46:19Z

I think the only part that could be worse is attach detection.
That could be improved by allowing the dev to give a node under which the chart will be attached (thus reducing the tree to monitor)

~~Would need a complex test to see if that is an issue or not.~~

Another thing is the ResizeObserver polyfill. It supports < IE11, so I think that could be made thinner.

benmccann · 2020-02-16T04:29:09Z

rollup.config.js

@@ -29,13 +28,12 @@ module.exports = [
 			resolve(),
 			commonjs(),
 			babel(),
-			stylesheet({


Maybe we should add something to the migration guide about the stylesheet and disableCSSInjection flag no longer being available?

etimberg

I'm not sure this is the right change to make. It took a lot of time to get to the current solution that worked in all cases for all browsers. I haven't looked at the implementation of the ResizeObserver polyfill but if it's not doing what we current do with two large divs, it likely doesn't work in all scenarios.

src/core/core.controller.js

src/platform/platform.dom.js

kurkle · 2020-02-16T15:20:10Z

but if it's not doing what we current do with two large divs..

Not sure what you mean? (both pens are of use cases master can't handle)

etimberg · 2020-02-16T15:39:55Z

I mean in terms of detecting a resize, how does the resize observer polyfill do it? The last time I researched this topic, the only two ways to do it performantly were the large divs + scroll events or using a hidden iframe.

kurkle · 2020-02-16T15:45:38Z

I haven't looked at it close either. It uses MutationObserver and works in these 2 cases in IE.

Not sure if its possible to run IE in karma?

etimberg · 2020-02-16T15:48:16Z

Looks like it might be possible using https://www.npmjs.com/package/karma-ie-launcher. Not sure how brittle it is

kurkle · 2020-02-16T17:02:27Z

Looks like it might be possible using https://www.npmjs.com/package/karma-ie-launcher. Not sure how brittle it is

Can't get it to work. Needed to add babel() to karman properosessors, but then the utils is undefined, so it can't find acquireChart etc.

benmccann · 2020-02-16T18:56:16Z

@etimberg there's some details here: https://github.com/que-etc/resize-observer-polyfill#observation-strategy It talks about why it didn't use the scroll strategy and also says it has no more than a 2-3% performance impact. Assuming that's true (and I don't have any reason to doubt it), I'd be pretty strongly in favor of this change. Even if it was much worse, I think we can take a bit of a performance hit on IE11 since it's an older browser and not as widely used

etimberg · 2020-02-16T19:26:27Z

Thanks for linking that. Really helps to understand why this is a better choice.

etimberg

Implementation looks OK.

rollup.config.js

benmccann · 2020-02-17T04:55:23Z

src/core/core.controller.js


+		if (height === undefined) {


should we check width === undefined || height === undefined ?

Maybe. I just tested the last parameter to detect missing params.

benmccann · 2020-02-17T04:56:53Z

src/platform/platform.dom.js

- * Injects CSS styles inline if the styles are not already present.
- * @param {Node} rootNode - the HTMLDocument|ShadowRoot node to contain the <style>.
- * @param {string} css - the CSS to be injected.
+ * @param {Element} canvas


Should this be an HTMLCanvasElement ?

kurkle · 2020-02-17T14:19:00Z

Found out regression to infinite resize in this PR and applied the same workaround.
For real use cases it would be best to use overflow-y: scroll in the window or other suitable parent.

benmccann · 2020-02-17T14:32:59Z

@kurkle the tests seem to be failing now

kurkle · 2020-02-17T14:36:03Z

Made 2 mistakes. Fixed.

kurkle force-pushed the responsive branch from f614445 to 64c4344 Compare February 14, 2020 21:49

benmccann mentioned this pull request Feb 15, 2020

Add tests for rendering to offscreen canvas #7029

Closed

benmccann reviewed Feb 16, 2020

View reviewed changes

kurkle force-pushed the responsive branch 2 times, most recently from bb71df1 to b81c8a7 Compare February 16, 2020 09:35

Use Resize/MutationObserver to detect detach/attach/resize

9877eb5

kurkle force-pushed the responsive branch from b81c8a7 to 9877eb5 Compare February 16, 2020 09:52

kurkle marked this pull request as ready for review February 16, 2020 09:58

etimberg reviewed Feb 16, 2020

View reviewed changes

src/core/core.controller.js Outdated Show resolved Hide resolved

src/platform/platform.dom.js Show resolved Hide resolved

etimberg reviewed Feb 16, 2020

View reviewed changes

etimberg previously approved these changes Feb 16, 2020

View reviewed changes

Cleanup

ac0b7c1

kurkle dismissed etimberg’s stale review via ac0b7c1 February 16, 2020 21:25

benmccann reviewed Feb 17, 2020

View reviewed changes

rollup.config.js Show resolved Hide resolved

benmccann reviewed Feb 17, 2020

View reviewed changes

Review update

fb2a0b6

benmccann previously approved these changes Feb 17, 2020

View reviewed changes

etimberg previously approved these changes Feb 17, 2020

View reviewed changes

kurkle dismissed stale reviews from etimberg and benmccann via da9d329 February 17, 2020 14:16

Restore infinite resize detection (chartjs#6011)

203afdc

kurkle force-pushed the responsive branch from da9d329 to 203afdc Compare February 17, 2020 14:34

benmccann approved these changes Feb 17, 2020

View reviewed changes

etimberg approved these changes Feb 17, 2020

View reviewed changes

etimberg merged commit bfe3421 into chartjs:master Feb 17, 2020

etimberg linked an issue Feb 17, 2020 that may be closed by this pull request

With max-height set on UL class on page, canvas never stop growing up #3979

Closed

etimberg mentioned this pull request Feb 17, 2020

With max-height set on UL class on page, canvas never stop growing up #3979

Closed

etimberg linked an issue Feb 17, 2020 that may be closed by this pull request

[BUG] Charts are not displayed when nested under <details> (in Firefox) #5158

Closed

etimberg mentioned this pull request Feb 17, 2020

[BUG] Charts are not displayed when nested under <details> (in Firefox) #5158

Closed

etimberg linked an issue Feb 17, 2020 that may be closed by this pull request

Responsiveness does not play nice with Bootstrap layouts #6145

Closed

etimberg mentioned this pull request Feb 17, 2020

Responsiveness does not play nice with Bootstrap layouts #6145

Closed

kurkle deleted the responsive branch February 19, 2020 18:22

benmccann mentioned this pull request Feb 19, 2020

Remove css from deploy scripts #7131

Merged

benmccann mentioned this pull request Feb 28, 2020

Chart.js no longer needs CSS leeoniya/uPlot#136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ResizeObserver and MutationObserver to detect detach/attach/resize #7104

Use ResizeObserver and MutationObserver to detect detach/attach/resize #7104

kurkle commented Feb 14, 2020 •

edited

benmccann commented Feb 14, 2020

kurkle commented Feb 14, 2020

benmccann commented Feb 14, 2020

kurkle commented Feb 14, 2020 •

edited

benmccann Feb 16, 2020

etimberg left a comment

kurkle commented Feb 16, 2020 •

edited

etimberg commented Feb 16, 2020

kurkle commented Feb 16, 2020

etimberg commented Feb 16, 2020

kurkle commented Feb 16, 2020

benmccann commented Feb 16, 2020

etimberg commented Feb 16, 2020

etimberg left a comment

benmccann Feb 17, 2020

kurkle Feb 17, 2020

benmccann Feb 17, 2020

kurkle commented Feb 17, 2020 •

edited

benmccann commented Feb 17, 2020

kurkle commented Feb 17, 2020

Use ResizeObserver and MutationObserver to detect detach/attach/resize #7104

Use ResizeObserver and MutationObserver to detect detach/attach/resize #7104

Conversation

kurkle commented Feb 14, 2020 • edited

benmccann commented Feb 14, 2020

kurkle commented Feb 14, 2020

benmccann commented Feb 14, 2020

kurkle commented Feb 14, 2020 • edited

benmccann Feb 16, 2020

Choose a reason for hiding this comment

etimberg left a comment

Choose a reason for hiding this comment

kurkle commented Feb 16, 2020 • edited

etimberg commented Feb 16, 2020

kurkle commented Feb 16, 2020

etimberg commented Feb 16, 2020

kurkle commented Feb 16, 2020

benmccann commented Feb 16, 2020

etimberg commented Feb 16, 2020

etimberg left a comment

Choose a reason for hiding this comment

benmccann Feb 17, 2020

Choose a reason for hiding this comment

kurkle Feb 17, 2020

Choose a reason for hiding this comment

benmccann Feb 17, 2020

Choose a reason for hiding this comment

kurkle commented Feb 17, 2020 • edited

benmccann commented Feb 17, 2020

kurkle commented Feb 17, 2020

kurkle commented Feb 14, 2020 •

edited

kurkle commented Feb 14, 2020 •

edited

kurkle commented Feb 16, 2020 •

edited

kurkle commented Feb 17, 2020 •

edited