Skip to content

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Aug 22, 2024

Closes #5068

I was able to debug with my local build

We were getting into a state where the offsetWidth was within 1px of the scrollWidth, resulting in trying to make the font ever smaller since there was no guard against it getting too small/negative

The reason we were within 1px is that Safari seems to floor clientWidth when other browsers do round, so I've added a + 1 to account for that. I tried using the bounding client rect and rounding myself, but for some reason, it sometime suffer from being a pixel off as well. So it may be a deeper rounding issue than I can account for.

the + 1 discrepancy doesn't appear to harm anything and it was taking wayyyy too much of my time to try to fix it.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

window.addEventListener('load', () => listen());
}

let raf = null;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is just some defensive code

@snowystinger snowystinger mentioned this pull request Aug 22, 2024
@rspbot
Copy link

rspbot commented Aug 22, 2024

@theMosaad
Copy link
Contributor

Nice one 🙌 Can confirm it fixed the issue for me

Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

Fixes the issue when zooming. I'll have to re-test this after release to see if it fixes the crashing I experience in the production build without zooming. That could be a separate issue.

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

verified that I could now reproduce the previous issue on prod docs in Safari and that the changes here have fixed the crash

@rspbot
Copy link

rspbot commented Aug 22, 2024

@rspbot
Copy link

rspbot commented Aug 22, 2024

@snowystinger snowystinger merged commit c8e5ed4 into main Aug 22, 2024
29 checks passed
@snowystinger snowystinger deleted the fix-safari-crash-on-docs branch August 22, 2024 21:58
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.

Issues with Safari
6 participants