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

Prevent infinite resize when vertical scrollbar appears #6011

Merged
merged 5 commits into from
Jan 30, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Jan 23, 2019

A workaround to the infinte resize problem.

Pens to demonstrate:
master
master - 3 columns

THIS PR:(840d11a)
single
3 rows
3 columns
collapse rows
collapse columns

You get the effect (in master) by resizing window so that scrollbar just appears.

Fixes: #2127

if (container && container.clientWidth !== w && chart.canvas) {
// If the container size changed during chart resize, we can assume scrollbar appeared.
// So let's resize again, with the scrollbar visible
ret = listener(createEvent('resize', chart));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how the 2nd resize doesn't cause the scrollbar to go away

Copy link
Member Author

@kurkle kurkle Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I think happens: We are doing the resize in a onScroll event. We resize, notice it made scrollbars visible and resize again. Scrollbars are hidden in the end of this scroll event. And I suppose browsers won't fire another scroll until the previous is done.
And throttled() prevents multiple onScroll's in same frame.

Initially I was storing the width, but accidentally found out, it was not really needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, that's a good find. If throttled() can prevent the double event then I'm ok with testing this out further

etimberg
etimberg previously approved these changes Jan 24, 2019
benmccann
benmccann previously approved these changes Jan 25, 2019
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still getting a jumpy behavior (not infinite) just before the scrollbar become visible or hidden. I think it's better than master but still unexpected and will certainly be reported as a bug.

Anyway, I'm a bit worried about this PR because we are considering that a container width change during resize is due to scrollbar visibility change. But what about animations (via JS or CSS) or similar? can you setup a pen that resize the container with a CSS animation (expand / collapse for example)?

Also, can we test this behavior with multiple charts on the same page (same row or not)?

src/platforms/platform.dom.js Outdated Show resolved Hide resolved
src/platforms/platform.dom.js Outdated Show resolved Hide resolved
src/platforms/platform.dom.js Outdated Show resolved Hide resolved
src/platforms/platform.dom.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented Jan 25, 2019

I'm still getting a jumpy behavior (not infinite) just before the scrollbar become visible or hidden. I think it's better than master but still unexpected and will certainly be reported as a bug.

This is due to the 2nd resize that's done while scrollbar is visible (and thus reducing width).
The width is a bit smaller (scrollbar width -1 or so) than just before, so it jumps a little.
I thought about adding the math to compensate that part, but I think its not worth the effort and prone to buggy situations.

Anyway, I'm a bit worry about this PR because we are considering that a container width change during resize is due to scrollbar visibility change. But what about animations (via JS or CSS) or similar? can you setup a pen that resize the container with a CSS animation (expand / collapse for example)?

Good points, I'll do some more pens.

Also, can we test this behavior with multiple charts on the same page (same row or not)?

I've tested it with multiple charts on different lines and they end up being different sizes some times. Still better than infinite resize IMO. I'll add pens for this too.

@simonbrunel
Copy link
Member

simonbrunel commented Jan 25, 2019

I thought about adding the math to compensate that part, but I think its not worth the effort and prone to buggy situations.

Agreed, and again, we can't assume that the container width has changed because the scrollbar visibility change. Also, we would need to handle cases where the scrollbars are not in the body but on any parent container.

I've tested it with multiple charts on different lines and they end up being different sizes some times. Still better than infinite resize IMO.

"Better" if that happens only instead of the infinite resize. What I'm worried about is that it could happen in other scenarios than scrollbars.

@kurkle kurkle dismissed stale reviews from benmccann and etimberg via fe90e53 January 25, 2019 13:54
@simonbrunel
Copy link
Member

simonbrunel commented Jan 25, 2019

Thanks @kurkle for the pens, I can barely test the columns ones because my screen is not high enough (laptop) :). 3 columns jumps a lot vertically when resizing, I'm wondering if it's the same issue with master? Else size animations don't seem to be impacted by this workaround.

@kurkle
Copy link
Member Author

kurkle commented Jan 25, 2019

@simonbrunel change the aspectRatio to suit your screen better :)

@simonbrunel
Copy link
Member

Yeah, that's what I did but can't test the pen in "Full page view", but it was enough to see the "obvious issue here". Having the full Chart.js code in the pen makes the pen really painful to manipulate, I'm wondering if there is not another way to easily integrate WIP builds.

@simonbrunel simonbrunel changed the title simple workaround for infinite resize Prevent infinite resize when vertical scrollbar appears Jan 25, 2019
@simonbrunel
Copy link
Member

simonbrunel commented Jan 25, 2019

I think the 3 columns jumping behavior is due to the display: inline-block which aligns content on bottom, so it should be the same issue with master.

I noticed that when resizing down the window, the vertical scrollbar breaks as soon as the workaround applies. I guess this is better than the current behavior, I just hope that we are not going to break use cases that depend on the container resizing.

@kurkle
Copy link
Member Author

kurkle commented Jan 26, 2019

Limited the 2nd resize to when container size shrinks in the first one. This removes some jumpiness (and is logical).

Updated the comment to contain all relevant information (I'm not native in English, so happy to reword).

All pens updated and also added a master - 3 columns.
In the ones with columns, I added vertical-align: top; to css.

@simonbrunel simonbrunel merged commit e6d5896 into chartjs:master Jan 30, 2019
simonbrunel pushed a commit that referenced this pull request Jan 30, 2019
If the container size shrank during chart resize, let's assume scrollbar appeared. So we resize again with the scrollbar visible effectively making chart smaller and the scrollbar hidden again. Because we are inside `throttled`, and currently `ticking`, scroll events are ignored during this whole 2 resize process. If we assumed wrong and something else happened, we are resizing twice in a frame (potential performance issue)
@simonbrunel simonbrunel added this to the Version 2.8 milestone Feb 1, 2019
@kurkle kurkle deleted the infinite-scoll branch May 5, 2019 06:48
kurkle added a commit to kurkle/Chart.js that referenced this pull request Feb 17, 2020
kurkle added a commit to kurkle/Chart.js that referenced this pull request Feb 17, 2020
@kurkle kurkle mentioned this pull request Feb 17, 2020
etimberg pushed a commit that referenced this pull request Feb 17, 2020
#7104)

* Use Resize/MutationObserver to detect detach/attach/resize
* Cleanup
* Review update
* Restore infinite resize detection (#6011)
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
If the container size shrank during chart resize, let's assume scrollbar appeared. So we resize again with the scrollbar visible effectively making chart smaller and the scrollbar hidden again. Because we are inside `throttled`, and currently `ticking`, scroll events are ignored during this whole 2 resize process. If we assumed wrong and something else happened, we are resizing twice in a frame (potential performance issue)
@ashley-mort
Copy link

ashley-mort commented Feb 29, 2024

I still get the infinite scroll event just before my Y axis scrollbar appears. Any additional tips on how to avoid this since this fix was made? I'm on Chart.js 2.9.4.

UPDATE: I got around my issue by setting maintainAspectRatio:false; on all my charts and then setting aspect-ratio:2; in the css of my charts' parent containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Chartjs-v2.0] Infinite .resize() when scrollbar appears
5 participants