-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Make system/violation messages text-selectable on desktop web #92142
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
Open
MelvinBot
wants to merge
5
commits into
main
Choose a base branch
from
claude-systemMessagesSelectable
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+25
−6
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
a811866
Make system/violation messages text-selectable on desktop web
MelvinBot cb816a1
Merge remote-tracking branch 'origin/main' into claude-systemMessages…
MelvinBot 3987980
Make inline system error messages text-selectable on desktop web
MelvinBot 11dd674
Merge remote-tracking branch 'origin/main' into claude-systemMessages…
MelvinBot ee290cd
Make task system messages text-selectable on desktop web
MelvinBot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MelvinBot, why are you using
shouldUseNarrowLayout?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It mirrors the existing selectability pattern already used elsewhere in the chat, so behavior stays consistent across message types:
TextCommentFragment.tsx:114(regular chat messages)DotIndicatorMessage.tsx:69(error messages)Both gate selectability on
!canUseTouchScreen() || !shouldUseNarrowLayout. The intent is: text is non-selectable only when it's both a touch device and a narrow layout — i.e. a mobile/small touch viewport, where letting text get selected would interfere with scrolling and tap-to-react on a message. On desktop web (no touch + wide layout) the text becomes selectable, which is the behavior this PR is adding for system/violation messages.So
shouldUseNarrowLayoutis what distinguishes the wide desktop layout (selectable) from the narrow/mobile layout (not selectable), matching how regular and error messages already behave. Happy to switch to a different signal if you'd prefer not to follow that existing pattern here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MelvinBot, what will be the behaviour on ipad? Also why aren't we only using
!canUseTouchScreen()?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iPad behavior depends on layout width (these
userSelect*styles are web-only — they have no effect in the native app, whereTextselectability is governed by theselectableprop instead):!canUseTouchScreen()isfalsebut!shouldUseNarrowLayoutistrue, sofalse || true→ selectable.false || false→ not selectable.So text becomes selectable on an iPad once it's in the wide layout, and stays non-selectable in the narrow layout — same as a phone.
Why not only
!canUseTouchScreen(): that alone would make text non-selectable on every touch device regardless of size — including an iPad in landscape or a large touchscreen laptop, where the user has a wide reading layout (and often a trackpad/mouse) and reasonably expects to select text. The|| !shouldUseNarrowLayoutadds those wide-layout touch devices back in.Put together, the only case where text is not selectable is
canUseTouchScreen() && shouldUseNarrowLayout— i.e. a touch device and a narrow layout (phones, iPad portrait/split). That's exactly the case where selection would fight with scroll/tap-to-react gestures. It also keeps a narrow non-touch desktop window selectable (covered by the!canUseTouchScreen()half), which using only!shouldUseNarrowLayoutwould not.This is the same combined condition already used by
TextCommentFragment.tsx:114andDotIndicatorMessage.tsx:69, which is why I matched it here rather than dropping to!canUseTouchScreen()alone.