Skip to content

Tableview crash bug #5380

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

Merged
merged 11 commits into from
Nov 11, 2023
Merged

Tableview crash bug #5380

merged 11 commits into from
Nov 11, 2023

Conversation

yihuiliao
Copy link
Member

@yihuiliao yihuiliao commented Nov 7, 2023

Might be related to #5357

Some observations:

✅ 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:

  1. Pull down branch
  2. Run storybook locally
  3. Go to TableView > Async Reload on Sort
  4. Set window to 1082.73 width (or as close to it as possible) 90% zoom
  5. Try sorting the Title column by clicking on it
  6. It should crash but you might need to click on Title a couple times to get it to work. See if you notice any difference with Strictmode on or off.

Note: It is a bit finicky but both Rob and I have been able to reproduce it at this width and zoom pretty consistently.

🧢 Your Project:

@rspbot
Copy link

rspbot commented Nov 9, 2023

@LFDanLu
Copy link
Member

LFDanLu commented Nov 10, 2023

Note about the wrapping the following updateSize in RAFs: it resolves the crashes in both TableView and CardView but infinite resize loops can still occur. They don't crash the components but they are visible as constant flickering at specific breakpoints unique to each setup. Tests and lint run clean

@rspbot
Copy link

rspbot commented Nov 10, 2023

@rspbot
Copy link

rspbot commented Nov 10, 2023

@rspbot
Copy link

rspbot commented Nov 10, 2023

@adobe adobe deleted a comment from rspbot Nov 10, 2023
@devongovett
Copy link
Member

I'd rather not add a new property to LayoutInfo. Instead, I think we can make the header have an inner div with the content size so it matches how ScrollView works.

@rspbot
Copy link

rspbot commented Nov 10, 2023

@yihuiliao yihuiliao marked this pull request as ready for review November 10, 2023 22:38
snowystinger
snowystinger previously approved these changes Nov 10, 2023
@@ -655,7 +656,14 @@ function TableVirtualizer(props) {
transition: state.isAnimating ? `none ${state.virtualizer.transitionDuration}ms` : undefined
}}
ref={headerRef}>
{state.visibleViews[0]}
<div
Copy link
Member

Choose a reason for hiding this comment

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

clever 👍🏻

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.

@snowystinger
Copy link
Member

Oo, those all crash instantly, i don't think we want to introduce that

LFDanLu
LFDanLu previously approved these changes Nov 11, 2023
@devongovett devongovett dismissed stale reviews from LFDanLu and snowystinger via c96a5f3 November 11, 2023 02:08
@rspbot
Copy link

rspbot commented Nov 11, 2023

@rspbot
Copy link

rspbot commented Nov 11, 2023

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@dannify dannify merged commit dad13d5 into main Nov 11, 2023
@dannify dannify deleted the tableview-crash-bug branch November 11, 2023 02:46
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.

6 participants