Make system/violation messages text-selectable on desktop web#92142
Make system/violation messages text-selectable on desktop web#92142MelvinBot wants to merge 5 commits into
Conversation
Co-authored-by: Sahil <thesahindia@users.noreply.github.com>
|
@thesahindia I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
@MelvinBot, merge main. |
…Selectable Co-authored-by: Sahil <thesahindia@users.noreply.github.com>
|
🤖 Merged the latest |
|
I analyzed the two failing checks. Neither is caused by this PR, and there's nothing to fix on this branch.
|
|
I analyzed the failing checks. The
|
|
The failing Root cause (on
|
|
@MelvinBot, Inline error messages aren't selectable. |
Apply the same selectable pattern used by the other system-message renderers to InlineSystemMessage, which renders action.error text. It previously inherited user-select: none from the report action's PressableWithSecondaryInteraction. Co-authored-by: Sahil <thesahindia@users.noreply.github.com>
|
Good catch — fixed and pushed ( Root cause: Inline error messages render through Fix: Applied the identical conditional pattern already used by const selectableStyle = !canUseTouchScreen() || !shouldUseNarrowLayout ? styles.userSelectText : styles.userSelectNone;
// ...
<Text style={[styles.inlineSystemMessage, selectableStyle]}>{message}</Text>Touch/narrow-layout behavior is preserved. Note on the other inline-error path
Checks: Prettier, ESLint, and React Compiler compliance pass on the changed file. |
|
I analyzed the failing checks. Neither requires a code change — the PR diff is correct as-is.
|
|
I analyzed the two failing checks. Neither requires a code change — the PR diff is correct as-is.
|
|
Still testing. There are too many different instances. |
|
@MelvinBot, can you post test steps for different instances? |
|
This PR touches three different system-message renderers, so the "different instances" each cover a separate code path. Here are concrete steps to verify all three.
Instance 1 — Simple system messages —
|
| function InlineSystemMessage({message = ''}: InlineSystemMessageProps) { | ||
| const theme = useTheme(); | ||
| const styles = useThemeStyles(); | ||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); |
There was a problem hiding this comment.
@MelvinBot, why are you using shouldUseNarrowLayout?
There was a problem hiding this comment.
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 shouldUseNarrowLayout is 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.
@MelvinBot, what will be the behaviour on ipad? Also why aren't we only using !canUseTouchScreen()?
There was a problem hiding this comment.
iPad behavior depends on layout width (these userSelect* styles are web-only — they have no effect in the native app, where Text selectability is governed by the selectable prop instead):
- iPad web in landscape (window width > 800px → wide layout):
!canUseTouchScreen()isfalsebut!shouldUseNarrowLayoutistrue, sofalse || true→ selectable. - iPad web in portrait / split view (window width ≤ 800px → narrow layout):
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 || !shouldUseNarrowLayout adds 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 !shouldUseNarrowLayout would not.
This is the same combined condition already used by TextCommentFragment.tsx:114 and DotIndicatorMessage.tsx:69, which is why I matched it here rather than dropping to !canUseTouchScreen() alone.
|
The failing check is unrelated to this PR — it's a flaky test, no fix needed. Analysis: The only real failing check is Evidence:
Recommendation: Re-run the failed I did not push any changes since there's nothing to fix here. |
|
Still nothing to fix here — the failure is the same flaky
I couldn't trigger a CI re-run from here (it needs interactive approval in this automated context), and per policy I won't push a code change to paper over a flaky test. |
…Selectable Co-authored-by: Sahil <thesahindia@users.noreply.github.com>
|
The only real failing check ( As I noted in my previous comment, the failure was in What I did: Merged the latest Once the new run finishes, the test checks should pass. The two |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot fill this checklist for the author checklist section |
|
I filled in the PR Author Checklist, but I only checked the items I can honestly verify from the code/diff. I left the rest unchecked because they attest to human-only work that isn't present yet (empty Checked (verifiable from the diff — a pure UI/style change touching 3 files):
Left unchecked — needs you (the human co-author):
Once you've done the platform testing and pasted in the Tests/QA steps and screenshots, you can tick the remaining boxes yourself. |
|
@MelvinBot mark off all the items in the checklist, I have verified them. I don't have the access to edit. |
|
🤖 Done — I've checked off all items in the PR Author Checklist on your behalf, per your confirmation that you verified them. Note: I left the |
Note: Error messages inside pressable components won't be selectable. For example, the error text in the expense preview. @MelvinBot, add these steps to QA and Test steps section |
|
@MelvinBot, the system message "marked as complete" and "marked as incomplete" aren't selectable. These are related to tasks. |
Co-authored-by: Sahil <thesahindia@users.noreply.github.com>
|
Good catch — task system messages ( I applied the same selectable pattern there: Prettier, lint, typecheck, and the React Compiler compliance check all pass. |
|
@MelvinBot, can you find more such instances? |
|
Yes — I scanned the report-action / system-message renderers and found ~18 more spots that render text or HTML inside the pressable wrapper without opting into selectability ( Two highest-impact (very common paths) — confirmed:
Full list of remaining candidates (14 more)
Other
Plain
Lower confidence (UX call):
Confirmed out of scope (rendered outside the Want me to apply the selectable fix (same |
|
@MelvinBot mark PR as ready for review |
|
Marked as ready for review. cc |
melvin-bot
Bot
requested review from
trjExpensify
and removed request for
a team
|
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@trjExpensify, you can Ignore this one. I think @joekaufmanexpensify will review it. |
@joekaufmanexpensify, can we get QA to test the different instances before the merge? I have tested many instances but there are still some left that I don't know how to repro, and there might be more instances than what melvin has listed. |
Sounds good, I'll swap him on. |
trjExpensify
requested review from
joekaufmanexpensify
and removed request for
trjExpensify
|
Sounds good! I will work on this. |
|
@MelvinBot, can you share test steps for each instance? |
|
Here are concrete test steps for each of the 4 instances this PR actually changes (one per file). All verification is on desktop web / wide layout, where the fix applies.
Instance 1 — Simple system messages —
➡️ Run the core check on each message. Instance 2 — Modified-expense messages (RenderHTML) —
➡️ Run the core check on the HTML-rendered message. Instance 3 — Task system messages —
➡️ Run the core check on each. Instance 4 — Inline violation/system messages —
➡️ Run the core check on the inline message text. Finally: verify no errors appear in the JS console. Note on scope: this PR fixes these 4 renderers only. The larger list of additional candidates I posted earlier ( |
|
@MelvinBot provide test steps for the full list of instances that you shared in #92142 (comment) |
|
Here are test steps for the full list of instances from my earlier scan. These cover spots not addressed by this PR — they're for scoping/verification, not all in scope here. Universal verification (applies to every instance below): On macOS Chrome/Safari (wide layout), after triggering the message, double-click a word in it (or click-drag across it) and press
Confirmed / highest-impact
|
|
@MelvinBot can you help with the detailed steps instead of one line instructions? |
|
Here are detailed, step-by-step instructions for each instance. All testing is on macOS Chrome/Safari in a wide (non-narrow) browser window. Common verification step — do this after triggering each message:
Below, "the message" = the grey system message produced by each flow. Apply the common verification to each. Confirmed / highest-impact
Submitted / Approved
Held / Unheld expense
Member changes / room-description changes (
|
4 participants
Explanation of Change
Allow system/violation messages in chat (hold/unhold, approved, rejected, dismissed violation, modified expense, etc.) to be text-selectable and copyable on desktop web.
Every report action is wrapped in a
PressableWithSecondaryInteractionwithaccessibilityRole=BUTTON, which causesBaseGenericPressableto applystyles.userSelectNone(CSSuser-select: none). Child components must explicitly override this to be selectable — regular chat messages do so viaTextCommentFragment, but system messages did not.This PR applies the same pattern already used by
TextCommentFragmentandDotIndicatorMessageto the two system-message renderers:ReportActionItemBasicMessage.tsx— renders most simple system messages with<Text>/<TextWithEmojiFragment>. Now appliesstyles.userSelectTexton desktop web (and keepsstyles.userSelectNoneon touch/narrow layouts).ReportActionItemMessageWithExplain.tsx— renders modified-expense messages viaRenderHTML. The hard-codedisSelectable={false}is changed to the same conditional!canUseTouchScreen() || !shouldUseNarrowLayout.Touch-device behavior is preserved (no accidental selection during scrolling).
Fixed Issues
$ #84639
PROPOSAL: #84639 (comment)
Tests
Note: Error messages inside pressable components won't be selectable. For example, the error text in the expense preview.
Offline tests
N/A — this is a pure UI/style change with no network behavior.
QA Steps
Note: Error messages inside pressable components won't be selectable. For example, the error text in the expense preview.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari