Skip to content

Conversation

@Kartik-Murthy
Copy link
Contributor

@Kartik-Murthy Kartik-Murthy commented Apr 2, 2025

Description
This PR resolves pointer cancellation issues in the Select component to ensure compliance with WCAG 2.5.2 accessibility requirements.

Changes

  • Added proper pointer event handling to address accessibility concerns
  • Included test cases to verify WCAG 2.5.2 compliance

Fixes #45301

Aditional

@Kartik-Murthy Kartik-Murthy force-pushed the fix/select-focus-drag-pointer branch from cbb9faa to 74a4a03 Compare April 2, 2025 11:39
@mui-bot
Copy link

mui-bot commented Apr 2, 2025

Netlify deploy preview

https://deploy-preview-45789--material-ui.netlify.app/

Bundle size report

Bundle Parsed Size Gzip Size
@mui/material 🔺+518B(+0.10%) 🔺+181B(+0.12%)
@mui/lab 0B(0.00%) 0B(0.00%)
@mui/system 0B(0.00%) 0B(0.00%)
@mui/utils 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against aff2e30

@zannager zannager added the scope: select Changes related to the select. label Apr 2, 2025
@zannager zannager requested a review from siriwatknp April 2, 2025 13:58
@Kartik-Murthy Kartik-Murthy force-pushed the fix/select-focus-drag-pointer branch 7 times, most recently from 5d7bc9c to eb21c2d Compare April 8, 2025 06:35
@Kartik-Murthy Kartik-Murthy force-pushed the fix/select-focus-drag-pointer branch from eb21c2d to aeab281 Compare April 10, 2025 12:19
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@Kartik-Murthy Thanks for the PR! Let’s keep it focused on one thing. Issues #44505 and #45374 shouldn’t be addressed yet — they’ll be handled once the Material UI Select is built on top of Base UI. For now, let’s just implement #45301. Could you remove the other changes and add a test case for #45301?

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Select] Fix focus, drag, and pointer interaction issues [Select] Fix focus, drag, and pointer interaction issues Apr 15, 2025
@Kartik-Murthy
Copy link
Contributor Author

Thank you @ZeeshanTamboli for the guidance! I've revised the PR to focus solely on fixing issue #45301 . I've removed the changes related to the other issues as requested and added test cases to verify the behavior. Let me know if you'd like to see any additional changes.

@Kartik-Murthy Kartik-Murthy force-pushed the fix/select-focus-drag-pointer branch from 863f35e to 7a58f33 Compare April 15, 2025 17:28
@ZeeshanTamboli ZeeshanTamboli changed the title [Select] Fix focus, drag, and pointer interaction issues [Select] Implement pointer cancellation Apr 16, 2025
@Kartik-Murthy Kartik-Murthy force-pushed the fix/select-focus-drag-pointer branch from 7a58f33 to 5a7a06a Compare April 16, 2025 08:33
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Before reviewing this in detail, I’m wondering whether we should even add this feature (or maybe it's an accessibility bug?), given that it already works in Base UI Select — and Material UI will eventually be built on top of Base UI.

@aarongarciah @DiegoAndai What do you think? How far off is the Material UI release based on Base UI? How should we handle support requests and community PRs like this in the meantime? There have been a few similar feature additions — see #45789 (review).

If we decide to go ahead with adding this in 7.x despite the upcoming Base UI integration in a new major version, I’ll proceed with a full review.

@Kartik-Murthy Kartik-Murthy force-pushed the fix/select-focus-drag-pointer branch from 369791d to 74f06a8 Compare April 17, 2025 03:10
- Fixed onFocus being triggered excessively (mui#44505)
- Enabled drag-and-release to select items (mui#45374)
- Addressed pointer cancellation (WCAG 2.5.2) failure (mui#45301)
- Restore original import order as per project conventions
@Kartik-Murthy Kartik-Murthy force-pushed the fix/select-focus-drag-pointer branch from 74f06a8 to d91d5ba Compare April 18, 2025 01:56
@DiegoAndai
Copy link
Member

DiegoAndai commented Jun 12, 2025

Hey @Kartik-Murthy, thanks for working on this, sorry for the delay.

@ZeeshanTamboli, we can move forward with this review. I don't have an estimate for Material UI being rebuilt on top of Base UI.

About the solution, I wonder if there's a way of handling the mouse up from the items, instead of having to set a global event listener. Did you explore that idea?

@DiegoAndai
Copy link
Member

Code looks good @ZeeshanTamboli!

Multiple select demos are broken though: https://deploy-preview-45789--material-ui.netlify.app/material-ui/react-select/#multiple-select

Base UI recently implemented multiple, maybe there are some clues there? mui/base-ui#2173

Scary that these weren't caught by tests, lets add some after finding the root cause of why this PR broke the demos.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jul 10, 2025

Code looks good @ZeeshanTamboli!

Multiple select demos are broken though: https://deploy-preview-45789--material-ui.netlify.app/material-ui/react-select/#multiple-select

Base UI recently implemented multiple, maybe there are some clues there? mui/base-ui#2173

@DiegoAndai Fixed now. It was occuring because both the mouseup and click events were triggering when we click on a menu item. It should trigger either mouseup or click. When clicking the item, it should trigger only click and cancel mouseup and when doing mouseup it should trigger only onMouseUp. I copied the pointer down logic from Base UI for this: https://github.com/mui/base-ui/blob/master/packages/react/src/select/item/SelectItem.tsx#L197C9-L197C26

Scary that these weren't caught by tests, lets add some after finding the root cause of why this PR broke the demos.

I think the bug wasn't caught because the tests didn't simulate both mouseup and click. However, surprisingly, there were no tests that directly checked item selection.

@DiegoAndai
Copy link
Member

DiegoAndai commented Jul 21, 2025

Thanks @ZeeshanTamboli!

think the bug wasn't caught because the tests didn't simulate both mouseup and click.

Could we use userEvent to add a test that would cover this?


cc @siriwatknp for awareness. We're close to merging this. May I ask you to review it? I want to be extra cautious with the Select as it's a priority component.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jul 24, 2025

Could we use userEvent to add a test that would cover this?

@DiegoAndai Added. The test now uses userEvent.click, which covers the bug since it fires the full event sequence: mouseovermousemovemousedownmouseupclick like a real user would in a browser.

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for working on it @ZeeshanTamboli

I'll leave the final approve for @siriwatknp

@ZeeshanTamboli
Copy link
Member

@siriwatknp Can you review?

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Great fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility a11y scope: select Changes related to the select.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Select] Pointer Cancellation (WCAG 2.5.2) failure

6 participants