[HOLD for payment 2023-03-31] [$1000] Clicking ctrl+A in the chat also selects the tooltip

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open staging on the desktop app or in a browser
2. Find an old chat thread with a history of messages
3. Highlight one of the messages in the chat
4. Click ctrl+A / cmd+A on mac
5. It highlights all text in the chat & the tooltip

Expected Result:

Only the text should be highlighted and not the tooltip

Actual Result:

The tooltips are selected

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.83-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Expensify/Expensify Issue URL:
Issue reported by: @tienifr
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678684053281919

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019f7286a4382e4682
• Upwork Job ID: 1635664567150116864
• Last Price Increase: 2023-03-14

[MelvinBot]

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[Christinadobrzyn]

I have no idea what these steps mean so I reached out in the Slack chat https://expensify.slack.com/archives/C049HHMV9SM/p1678684053281919

@kavimuru have you been able to reproduce this? Can you update the steps to reflect how you reproduced it?

Press Ctrl + A to select all the text within the drawer
Observe that all the text in the Report screen and all the tooltips are selected

[Christinadobrzyn]

Got more information on testing in the Slack convo and updated the OP. Going to add External label.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~019f7286a4382e4682

[MelvinBot]

Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

[MelvinBot]

Triggered auto assignment to @tylerkaraszewski (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The tooltips on the page is currently selectable by Ctrl A.

What is the root cause of that problem?

We're not preventing user from selecting tooltip's text content. I don't see any case where a tooltip text should be selectable. The tooltip text is only shown when we hover on the element, once we try to click to select any text, the tooltip will disappear.

What changes do you think we should make in order to solve the problem?

We should apply user-select: none style to the tooltip text style, this can be done by adding userSelect: 'none', here

App/src/styles/getTooltipStyles.js

Line 41 in e95b436

tooltipTextStyle: {

In order for the proposed solution to be effective, we'll need to remove the Safari hack mentioned in issue #3613 as well. I have tested this and can confirm that removing the hack will not reintroduce the bug that the hack was originally implemented to fix, using the latest code in the main branch.

[hoangzinh]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The tooltips of emotion icons on the chat page is currently selectable by Ctrl-A.

What is the root cause of that problem?

It's default behavior of the browser, when you press Ctrl-A, it will select all things on current page except elements having attribute user-select: none

Unlucky, the attribute user-select of tooltip is unset atm:

App/src/styles/getTooltipStyles.js

Lines 41 to 45 in e95b436

	tooltipTextStyle: {
	color: themeColors.textReversed,
	fontFamily: fontFamily.GTA,
	fontSize: variables.fontSizeSmall,
	},

What changes do you think we should make in order to solve the problem?

I think about an additional param textSelectable: PropTypes.bool (default is true) to getTooltipStyles util. So we can

tooltipTextStyle: {
    color: themeColors.textReversed,
    fontFamily: fontFamily.GTA,
    fontSize: variables.fontSizeSmall,
+   userSelect: textSelectable ? 'unset' : 'none',
}

It will give us more control each place we want to disable user-select on the tooltip text. I have checked several design systems, they allow selecting tooltip text by default

https://ant.design/components/tooltip
https://mui.com/material-ui/react-tooltip/

What alternative solutions did you explore? (Optional)

Another option is we write a HOC with style user-select: none, and we wrap current tooltip with this HOC

const withUserSelectDisable = (Component) => (props) =>
  (
    <div style={{userSelect: 'none'}}>
      <Component {...props} />
    </div>
  );
....
withUserSelectDisable(Tooltip)

[getusha]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When you use the keyboard shortcut CTRL+A to select the entire chat text, the visible tooltip text is also included in the selection.

What changes do you think we should make in order to solve the problem?

we can apply the existing styles.userSelectNone so that We can ensure that the existing style userSelectNone works on both Webkit and Chrome-based browsers by applying it

App/src/styles/styles.js

Lines 2851 to 2854 in eedd92a

	userSelectNone: {
	userSelect: 'none',
	WebkitUserSelect: 'none',
	},

What alternative solutions did you explore? (Optional)

N/A

[tienifr]

To clarify my proposal here will also include the variations of the user-select to support all browsers (like webkit), if not already clearly implied in the solution description.

[thesahindia]

if not already clearly implied in the solution description.

@tienifr, I don't think there was any implication at all.

@tienifr's proposal was the first but it only mentioned using user-select: none which will not work for Webkit based browsers. They added a new comment after @getusha's proposal. @getusha proposed using styles.userSelectNone which will work for both (Webkit and Chrome-based browsers) so I prefer going with them.

C+ reviewed 🎀👀🎀

@tylerkaraszewski, please take the final decision.

[tylerkaraszewski]

Let's use @getusha's proposal.

[thesahindia]

@Christinadobrzyn, let's hire and assign @getusha.

[getusha]

@tylerkaraszewski @thesahindia should i wait to be assigned or raise the PR now?

[MelvinBot]

📣 @getusha You have been assigned to this job by @Christinadobrzyn!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[Christinadobrzyn]

Hired @getusha for the fix, @thesahindia as C+, @tienifr as reporter

Upwork job - https://www.upwork.com/jobs/~019f7286a4382e4682

[getusha]

Thanks @Christinadobrzyn Accepted!

@thesahindia @tylerkaraszewski the PR is ready for review!

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.88-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Clicking ctrl+A in the chat also selects the tooltip #16075

If no regressions arise, payment will be issued on 2023-03-31. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@thesahindia / @tylerkaraszewski] The PR that introduced the bug has been identified. Link to the PR:
• [@thesahindia / @tylerkaraszewski] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@thesahindia / @tylerkaraszewski] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@Christinadobrzyn] Determine if we should create a regression test for this bug.
• [@thesahindia] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@Christinadobrzyn] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Christinadobrzyn]

Time for payment! It looks like the PR was created on March 17th and merged on March 20th so that would be a bonus.

So payment is:

@tienifr reporter $250
@getusha for the fix $1500
@thesahindia as C+ $1500

Sound correct?

[Christinadobrzyn]

• Paid in Upwork and job closed
• I think this should be a regression test, @thesahindia @getusha do you agree? Do you have a suggestion?

[MelvinBot]

@tylerkaraszewski, @Christinadobrzyn, @getusha, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick!

[getusha]

Regression Test Proposal

1. Hover over any Hoverable element that has a tooltip
2. Select the whole text using CMD + A or Ctrl + A
3. Verify that the text inside the Tooltip is not selected

Do we agree?

[thesahindia]

I think we can skip the first three items in the checklist. We shouldn't call this a regression, it was an edge case that was overlooked during the implementation of tooltip.

[Christinadobrzyn]

Thanks for the insights! I'll close this without creating a regression test based on @thesahindia's comment. Thanks!