Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Bug]: Offset by 1 issue for canScrollNext when tab size has fractional width #1096

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

yasuhiro-yamamoto
Copy link
Contributor

@yasuhiro-yamamoto yasuhiro-yamamoto commented Dec 28, 2024

The issue was caused by rounding differences between actual slide widths and offsetWidth calculations:

  • When using NodeRects for size calculations, offsetWidth returns integer values even when actual slide widths have decimal points
  • This integer rounding caused a 1px discrepancy in the scroll possibility check array when slide widths were fractional
  • As a result, canScroll returned incorrect boolean values for certain window/slide sizes
  • Added a 1px tolerance filter to prevent unexpected behavior from these sub-pixel differences

This fix ensures consistent canScroll behavior regardless of fractional slide widths by filtering out negligible 1px differences that should not affect scroll possibility determination.

@davidjerleke
Copy link
Owner

Hi @yasuhiro-yamamoto,

Thank you for your contribution. I'm working on v9 at the time of writing so it will take time before I have the chance to check this out. Thanks you for your patience.

One question: Regarding steps to reproduce the problem, can you reproduce the problem you describe simply by following the steps in this bug report?

@davidjerleke davidjerleke added the bug Something isn't working label Dec 29, 2024
@yasuhiro-yamamoto
Copy link
Contributor Author

@davidjerleke Thank you for your response.

Yes, I can reproduce the issue with the steps provided in the Issue. Here are the key points that I've confirmed also
reproduce the issue in playgrounds/embla-carousel-playground-react:

  • When the view displays an even number of slides, the issue occurs if the total number of slides is even (e.g., with flex-basis: 50%, the issue appears with 4 or 6 total slides)
  • When the view displays an odd number of slides, the issue occurs if the total number of slides is odd (e.g., with flex-basis: 33.3%, the issue appears with 5 or 7 total slides)

I noticed that my initially proposed code wasn't comprehensive enough and could still cause bugs in some cases, so I've added a commit with additional fixes.

I appreciate your time in reviewing this when possible.

@davidjerleke davidjerleke added the core This is related to the core package label Dec 30, 2024
@davidjerleke davidjerleke self-assigned this Jan 3, 2025
@davidjerleke davidjerleke changed the title #899 - Fix canScroll calculation for fractional slide widths [Bug]: Offset by 1 issue for canScrollNext when tab size has fractional width Jan 4, 2025
Co-authored-by: Yasuhiro Yamamoto <yasuhiroyamamoto@colers.jp>
Co-authored-by: David Jerleke <david.jerleke@gmail.com>
@davidjerleke
Copy link
Owner

davidjerleke commented Jan 6, 2025

@yasuhiro-yamamoto I've added tests and made some small adjustments. I think this is good to go. Thank you a lot for your contribution ⭐! This information helped me understand the problem and write appropriate tests.

@yasuhiro-yamamoto
Copy link
Contributor Author

Thank you for your feedback! I'm really glad I could help. This is such a great library, looking forward to contributing more!

@davidjerleke davidjerleke merged commit 1b01939 into davidjerleke:master Jan 6, 2025
@davidjerleke davidjerleke added the resolved This issue is resolved label Jan 6, 2025
@yasuhiro-yamamoto yasuhiro-yamamoto deleted the fix/issue-899 branch January 6, 2025 22:39
@davidjerleke
Copy link
Owner

@yasuhiro-yamamoto a bug fix for this was just released in 8.5.2. Thank you a lot for your contribution 🎉!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working core This is related to the core package resolved This issue is resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Offset by 1 issue for canScrollNext when tab size has fractional width
2 participants