nkbt / react-collapse

Component-wrapper for collapse animation with react-motion for elements with variable (and dynamic) height
MIT License
1.12k stars 113 forks source link

overflow: hidden is not removed from the component when it's opened at certain zoom levels #238

Closed covertbert closed 2 years ago

covertbert commented 6 years ago

In my application in Google Chrome if I zoom out to 75% and set isOpened to true overflow: hidden is not removed from the component.

Edit: To add to this, I've narrowed it down to an adjacent button having padding in it and the only way to fix it seems to be to remove this padding entirely

mlp73 commented 5 years ago

+1 We have the exact same behavior in our application when zooming down to 75%! It makes it impossible for the user to use nested dropdowns for example as the content is cut by the overflow:hidden on container.

mlp73 commented 5 years ago

It appears that the component nested in is kept in RESIZING mode. Without getting to the next state it never gets back to height:auto. See attached picture. It is easy to reproduce, would be great to get some follow up here. Skjermbilde 2019-06-05 kl  13 04 17

nkbt commented 5 years ago

v5 was a complete rewrite, I believe it should not have this issue (not published as latest though, but you can try it as @nkbt/react-collapse)

mlp73 commented 5 years ago

Thanks, I'll give it a try. :) When are you btw considering releasing v5 at latest?

nkbt commented 5 years ago

I'm using this version for a while in several projects without any issues, but don't really have time (read "want") to fill up all the documentation/migration guide (though it is almost drop-in replacement) for it, which is the main blocker for release.

mlp73 commented 5 years ago

Ok I see. It is maybe a bit difficult to get help from outside for that task (?) Let know here if there is any way we can help to make it happen. We cannot on our end use a non-officially-released version in production you imagine :)

nkbt commented 4 years ago

As new react-collapse@5.0.0 is released https://github.com/nkbt/react-collapse/releases/tag/v5.0.0 this issue has been most likely already solved.

Please feel free to re-open if the issue persists

mlp73 commented 4 years ago

Using react collapse 5.0.0 I can confirm that the issue persists.

overflow: hidden is set while the animation is going on, and set back to auto at the end. It calculates what will be the final height but because of it counting based on half-pixels because of devicePixelRatio it seems that it never finishes. Så overflow:hidden is then never removed.

I can reproduce everywhere react-collapse is used by using devicePixelRatio = 1.25. On my very screen it is with a 110% zoom. For some of our testers it is whit 75% zoom, so it depend on your resolution. (When you zoom +/-, the devicePixelRatio changes, and it happens always with devicePixelRatio = 1.25) To know which devicePixelRatio you have on your window, you can write window.devicePixelRatio in the console.

@nkbt Is it something you could try to reproduce?

nkbt commented 4 years ago

Reopened. Thanks for the report 👍

mlp73 commented 4 years ago

You are welcome. So far i use that (dirty) workaround. It kills a bit the animation but ensures the user can access the functionality. I could right now reproduce with deviceRatio 2.20, so I am unsure what exactly affects it.

handleReactCollapsePixelRatioIssue = (): void => {
    try {
      // gets all the react-collapse elements within filter
      const filterNode = ReactDOM.findDOMNode(this.filterOuterWrapperRef.current);
      const collapseContainers = (filterNode as HTMLElement).getElementsByClassName('ReactCollapse--collapse');

      // resets height and overflow
      for (let i = 0; i < collapseContainers.length; i++) {
        (collapseContainers[i] as HTMLElement).style.height = 'auto';
        (collapseContainers[i] as HTMLElement).style.overflow = 'initial';
      }
    } catch (e) {
      log('Filter: react-collapse fix for zoom level failed', e);
    }
  };

I pass handleReactCollapsePixelRatioIssue to the onRest method provided by react-collapse. I also call it with a listener on deviceRatio change, which is initialised on mount. See below:

 initPixelRatioMonitoring = () => {
    this.handleReactCollapsePixelRatioIssue();
    matchMedia('(resolution: 2.20dppx)').addListener(this.handleReactCollapsePixelRatioIssue);
  };
AndriiTsarenko commented 4 years ago

Hey! In one case we have a similar issue 😞 . We are rendering a page with an opened collapse component and the content of the collapse is hidden. It's a floating issue and reproduces only in one case but without any zooming or scaling the page. react-collapse version is 5.0.1 Browser: chrome 85.0.4183.83. image

nkbt commented 3 years ago

Tried pretty hard and cannot reproduce in my set of examples. Would really appreciate if you can create a codepen with an example that breaks.

antonstjernquist commented 3 years ago

This is still a issue for us.

It feels like some sort of rounding error based on pixels since if we add 2-3px of padding when it happens the issue is resolved.

Otherwise a timeout prop would solve the issue :D But I guess you don't want to re-introduce that.

nkbt commented 3 years ago

Yeah that must be some rounding issue. I cannot reproduce it myself, so cannot fix the issue for good.

combination of these might render that odd pixel.

For example, we could solve it by some rounding like 1-2px would be considered "done", but only after debounce "step" (so we can tell it is stable).

But that might be an issue by itself, because the case when we collapse element without hiding from 1-2px to auto is quite legit. Which could be solved by letting you choose "settle tolerance" option or smth

tldr: it's complicated :(

okhomenko commented 2 years ago

Just got reports from my colleagues about this issue. Their vision is degrading and they zoom in a lot of websites, ours including.

After a little digging I noticed that computed height of the element is always larger than the height we set. And zoom in/out skews the difference even more. So it may happen

in the mdn doc on clientHeight I found this:

Note: This property will round the value to an integer. If you need a fractional value, use element.getBoundingClientRect().

100% scale

142955511-94aea74d-bf28-4aaf-9c19-8722272239af

110%

Screen Shot 2021-11-22 at 5 26 17 PM
nkbt commented 2 years ago

@okhomenko should we just do Math.floor() and give it 1px tolerance for both checks?

As in

isFullyOpened = isOpened && Math.abs(contentHeight - containerHeght) <= 1

Bounding rect is kind of slow to use, I would rather sacrifice that precision for speed...

okhomenko commented 2 years ago

Reproducible jsfiddle: https://jsfiddle.net/d5knv0ym/45/ Mac, Monterey, Chrome 96.0.4664.55 (Official Build) (x86_64)

Screen Shot 2021-11-22 at 6 28 21 PM
okhomenko commented 2 years ago

@okhomenko should we just do Math.floor() and give it 1px tolerance for both checks?

As in

isFullyOpened = isOpened && Math.abs(contentHeight - containerHeght) <= 1

Bounding rect is kind of slow to use, I would rather sacrifice that precision for speed...

I like the idea with 1px tolerance

nkbt commented 2 years ago

@okhomenko which browser are you using because I cannot reproduce the issue in Opera with 110% zoom anyway =)

okhomenko commented 2 years ago

Mac, Monterey, Chrome 96.0.4664.55 (Official Build) (x86_64)

nkbt commented 2 years ago

Gotcha. Missed it in the prev message, sorry. I checked in chrome and it indeed has the issue.

okhomenko commented 2 years ago

I assume each browser has it's own math for computing client heights/width, based on DPI, zoom level etc.

Thanks for looking into this and lmk if I can help

nkbt commented 2 years ago

If you can try to reproduce the issue with updated https://nkbt.github.io/react-collapse/bundle.js that would be great!

okhomenko commented 2 years ago

When I use bundle.js instead of react-collapse.min.js I get this error: "Uncaught ReferenceError: ReactCollapse is not defined".

nkbt commented 2 years ago

Ah ok, forgot that the demo site has example baked in and does not export ReactCollapse.

My testing did not show any downsides to that solution so I will publish patch version and lets see how it goes then

okhomenko commented 2 years ago

That's awesome, thanks so much @nkbt. I can test it right away on my dev environment once package is published

nkbt commented 2 years ago

Published react-collapse@5.1.1

okhomenko commented 2 years ago

@nkbt tested on my environment and everything works amazing.

Thanks a lot. My colleagues much appreciate your hustle 😄

Screen Shot 2021-11-23 at 5 10 47 PM
nkbt commented 2 years ago

@okhomenko amazing! thank you very much for the detailed report, that made a world of difference