Skip to content

Only set userSelect:auto on anchor tags (native) #324

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 1 commit into from
Jun 27, 2025
Merged

Conversation

necolas
Copy link
Contributor

@necolas necolas commented Jun 26, 2025

Fix #314

Copy link

github-actions bot commented Jun 26, 2025

workflow: benchmarks/size

Comparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.

Results Base Patch Ratio
react-strict-dom/dist/dom/index.js
· compressed 2,513 2,513 1.00
· minified 8,695 8,695 1.00
react-strict-dom/dist/dom/runtime.js
· compressed 855 855 1.00
· minified 2,435 2,435 1.00
react-strict-dom/dist/native/index.js
· compressed 16,226 16,259 1.00 +
· minified 60,854 60,755 1.00 -

Copy link

github-actions bot commented Jun 26, 2025

workflow: benchmarks/perf (native)

Comparison of performance test results, measured in operations per second. Larger is better.

Results Base Patch Ratio
css.create
· small 1,129,366 1,136,227 1.01 +
· small with units 438,689 436,853 1.00 -
· small with variables 671,873 673,174 1.00 +
· several small 323,657 321,264 0.99 -
· large 209,140 206,674 0.99 -
· large with polyfills 149,180 146,523 0.98 -
· complex 102,377 101,433 0.99 -
· unsupported 220,474 222,422 1.01 +
css.createTheme
· simple theme 228,220 227,624 1.00 -
· polyfill theme 213,098 211,756 0.99 -
css.props
· small 245,757 246,198 1.00 +
· small with units 191,828 192,099 1.00 +
· small with variables 106,690 107,315 1.01 +
· small with variables of units 75,678 75,666 1.00 -
· large 104,852 104,345 1.00 -
· large with polyfills 37,359 37,573 1.01 +
· complex 23,406 23,689 1.01 +
· unsupported 147,408 146,998 1.00 -
· simple merge 163,554 164,534 1.01 +
· wide merge 17,722 17,905 1.01 +
· deep merge 17,418 17,578 1.01 +
· themed merge 31,386 31,312 1.00 -

Copy link

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

I've done some practical checks, and:

  • on iOS: neither onPress nor userSelect style triggers keyboard navigation focus. Wrapping Text in Pressable (or simply <View accessible>) allows for focusing the text. However, when nesting that inside Text you get whacky layout.

CleanShot 2025-06-27 at 15 08 01@2x

  • on Android: having onPress prop alone is sufficient for Text component to receive keyboard navigation focus (being wrapped in Pressable is also fine)

Therefore, I would recommend on not applying userSelect: auto to a elements, as:

  • it's not needed on Android, having onPress prop is enough
  • it does not work on iOS anyway
  • AFAIU the purpose of userSelect style is to allow for user to copy given text, not to make it focusable by keyboard, that would be tabIndex/focusable props (but these work only on Android anyway)

You can check the experiements repo here: https://github.com/mdjastrzebski/repro-rsd-keyboard-navigation

@necolas necolas force-pushed the fix/userSelect-auto branch from 8efcd35 to 560a273 Compare June 27, 2025 18:07
@necolas
Copy link
Contributor Author

necolas commented Jun 27, 2025

Updated to remove it entirely

@necolas necolas requested a review from mdjastrzebski June 27, 2025 18:08
Copy link

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

✅ Looks Good! Thank you for fixing it.

@necolas necolas merged commit 0be4843 into main Jun 27, 2025
7 checks passed
@necolas necolas deleted the fix/userSelect-auto branch June 27, 2025 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants