[Select, Menu] Handle pseudo-element bounds in mouseup detection #1250
Conversation
- Add getPseudoElementPadding utility to measure ::before and ::after dimensions - Update mouseup detection logic to include pseudo-element padding in clickable area - Fix unexpected Select closing when clicking within extended pseudo-element bounds Fixes mui#1232
Netlify deploy preview |
- Add getPseudoElementPadding utility to compute ::before/::after dimensions - Refactor mouseup handler to check extended clickable area - Update click detection logic to include pseudo-element padding - Improve trigger bounds calculation for more accurate click handling Fixes mui#1232 - Prevents menu from closing unexpectedly when clicking within the pseudo-element extended area of the trigger.
|
@atomiks I believe these changes might work better if we move the entire handleMouseUp and getPseudoElementPadding logic to a common place, as the behavior seems consistent. What do you think? |
onehanddev
requested review from
atomiks,
colmtuite and
michaldudak
as code owners
|
CircleCI is failing JSDOM test, I guess rerunning it should the fix the issue. |
onehanddev
changed the title
zannager
added
component: select
| function getPseudoElementPadding(triggerRefElement: HTMLElement) { | ||
| const beforeStyles = window.getComputedStyle(triggerRefElement, '::before'); | ||
| const afterStyles = window.getComputedStyle(triggerRefElement, '::after'); | ||
|
|
||
| const hasPseudoElements = | ||
| beforeStyles.content !== 'none' || afterStyles.content !== 'none'; | ||
|
|
||
| const padding = hasPseudoElements | ||
| ? Math.max( | ||
| parseInt(beforeStyles.width || '0', 10), | ||
| parseInt(afterStyles.width || '0', 10), | ||
| parseInt(beforeStyles.height || '0', 10), | ||
| parseInt(afterStyles.height || '0', 10), | ||
| ) / 2 | ||
| : 0; | ||
|
|
||
| return padding; | ||
| } | ||
|
|
There was a problem hiding this comment.
Thanks for your contribution!
This could be refactored into a utility function (and technically, parts of the rest of the function too) to reuse between Menu and Select.
- Given the padding can be asymmetric, this wouldn't actually measure it correctly I think, e.g. if
lefthas0pxwhilerighthas20px. Maybe instead of considering it as padding, you just check the max between the trigger rect width/height along with the pseudo elements' width/height parseFloat(beforeStyles.width) || 0is a simpler and more accurate function to use thanparseInt
There was a problem hiding this comment.
Thanks for the detailed review @atomiks
I have made these changes except I was a bit skeptical about moving the final checks to a utility function (see: technically, parts of the rest of the function too) as it was abstracting the code a bit much, Please take a look at the updated code and let me know if you think it still needs to be more reusable.
There was a problem hiding this comment.
That's ok, and I see that determining asymmetry on a given axis might not be feasible, so it's ok to assume it's symmetric.
However I still see the bug with the CSS modules selector on the docs, if I click on the bottom-left corner or right at the left edge, it still closes. Maybe floating point errors or something else?
There was a problem hiding this comment.
Yes its indeed a minor floating point error, I assume it should be fixed with Math.round, let me try that and push the fix.
There was a problem hiding this comment.
Can you please check the latest commit @atomiks Thanks !
There was a problem hiding this comment.
Yeah I cleared cache, and incognito is the same
There was a problem hiding this comment.
Just saw your earlier comment, Let me just add +1 -1 to see if the issue fixes for you.
There was a problem hiding this comment.
Just made above change. Feel free to recheck the preview URL.
There was a problem hiding this comment.
Debugged locally - we'll need 2 pixels, not 1. In fact I remember needing to use 2px for other things related to mouse events, and it works in all cases.
… code for the same.
michaldudak
changed the title
Below changes were made for Select and Menu components for their use[Menu/Select]trigger hook -
Fixes #1232