Skip to content

Conversation

levrik
Copy link

@levrik levrik commented Feb 27, 2025

Closes #8662

Upstreaming a fix for the product I'm working on. Please let me know if you require an issue to be created and I'll create one.

I'm not particulary happy with the fix but making e.shiftKey flag available in onChange of useCheckbox, which goes down quite deep through other hooks, probably does not make a lot of sense. Simply tracking shift key state in a local ref seemed like the cleanest solution to me.

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

Use a table with slot="selection" checkboxes and try to do a range selection by holding Shift key.

🧢 Your Project:

@levrik levrik force-pushed the fix-collection-checkbox-range-selection branch from be59ade to b451fa3 Compare February 28, 2025 10:56
}
};

document.addEventListener('keydown', trackKeyDown);
Copy link
Member

Choose a reason for hiding this comment

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

these should be capturing listeners, don't want to miss it should something call stopPropagation

Copy link
Author

Choose a reason for hiding this comment

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

makes sense, added

document.removeEventListener('keydown', trackKeyDown);
document.removeEventListener('keyup', trackKeyUp);
};
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
});
}, []);

We don't want to re-run this every time we render, only on mount/unmount

Copy link
Author

@levrik levrik Mar 3, 2025

Choose a reason for hiding this comment

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

oh my... no idea how I missed that. added.

};
});

// Checkbox should always toggle selection, regardless of selectionBehavior.
Copy link
Member

Choose a reason for hiding this comment

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

Will need to discuss with the team if the proposed change conflicts with this intent

Copy link
Author

@levrik levrik Mar 3, 2025

Choose a reason for hiding this comment

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

Just to add some background here: I have a table with href on each row so people use the checkboxes to do the initial selection. While the table then switches into another mode that allows selection also on the rest of each row, people tend to keep clicking on the checkboxes. Not having Shift+Click for range selection available there while it is on the rest of the table is kinda confusing and led to complaints from customers and even the CEO 😅

@snowystinger
Copy link
Member

FYI, haven't had a chance to bring it up yet. However, I did notice another issue we'll need to consider.

This attaches keydown window listeners, however, if the window isn't focused when the shift key is pressed. Maybe the user was responding to Slack in another window, but then shift clicked back. The range won't be expanded. We have to know at the time that the click occurs regardless. We'll either need to expose our press events on checkbox or we'll need to expose the shift key in the onChange event.

Thanks for your patience.

@levrik
Copy link
Author

levrik commented Mar 10, 2025

@snowystinger Yeah. These were compromises that I personally am fine with for my usecase but I understand that as a library this maybe is not good enough. Having access to the shiftKey flag of the originating event would be the much better implementation of course but I wasn't sure if exposing this in onChange makes any sense, especially given that it's only relevant when onChange was triggered by a mouse click.

@snowystinger
Copy link
Member

So sorry and thank you for your patience, looks like we never quite got final agreement on this. I checked back on the conversation, and it was left here:

leaning toward exposing the e.shiftKey on the toggle's onChange unless we have other specific use cases for user provided press handlers on the checkbox. However that being said, we accept press events for hooks like useMenuItem, aka elements that already have pre-defined press behaviors so it might not be so bad adding press handler support to checkbox

yeah, i was thinking we expose press events on other components, so we could that here too. it’d be harder to implement the “is shift pressed” going that route, as you’d need to have extra event listeners and remember to clear the “state” out after the press end/cancel/whatever

Will try to get more movement on it again

@levrik
Copy link
Author

levrik commented Aug 4, 2025

@snowystinger Maybe the change from the last release about supporting native DOM events can help implementing a slightly different behavior for mouse clicks here?

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.

[RAC] Checkbox not recognizing shift-click inside collections

2 participants