Skip to content
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

Only select message data on triple click #6500

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

nrayburn-tech
Copy link
Contributor

Fixes #6428

First time contributor checklist:

Contributor checklist:

  • My contribution is not related to translations.
  • My commits are in nice logical chunks with good commit messages
  • My changes are rebased on the latest main branch
  • A yarn ready run passes successfully (more about tests here)
  • My changes are ready to be shipped to users

Description

I'm not sure if the code should exist somewhere else, if so I can move it.
I'm not sure if triple-clicking is a macOS specific functionality or not. If it is, a check can be added to only enable this change for macOS devices.

Can be tested by opening a conversation and triple-clicking a message. All text up to the next line break in the message should be selected. It can then be copied and pasted with only the message text, the timestamp should not be copied or pasted.

I didn't find any existing tests to go off of for the Message component, so I didn't write any new tests.

Tested on macOS 13.4.1 (22F82).

triple-click.mov

@scottnonnenberg-signal
Copy link
Contributor

scottnonnenberg-signal commented Jun 30, 2023

@nrayburn-tech Thanks for digging in on this. I played with your selection-changing code a bit, and it does seem to have some problems when the contents of the message is not just text. Links for sure cause weird behavior. Given this message:

Hey, did you see this? https://github.com/signalapp/Signal-Desktop/pull/6500

Your default triple-click would select the entire thing, but your logic will force the selection to just 'Hey, did you see this?'

You'll probably also want to test things like emoji and new formatting (bold, italic, strikethrough, monospace, spoiler). Those all put interesting things into the composer that might change behavior.

@nrayburn-tech
Copy link
Contributor Author

@scottnonnenberg-signal Updated the PR with changes that should be safer. If the triple click creates a selection that includes the metadata, then the selection is modified to be just before the metadata element so that it is excluded.

I was able to test your example with a link, but I couldn't figure out how to do custom formatting (bold, italic, strikethrough, etc.) to test it. I think those scenarios should still work with this approach though.

When running yarn ready I get a warning that I don't really understand, any help resolving it would be appreciated.

[lint-deps    ] Questionable lines:
[lint-deps    ] [
[lint-deps    ]   {
[lint-deps    ]     "rule": "React-createRef",
[lint-deps    ]     "path": "ts/components/conversation/Message.tsx",
[lint-deps    ]     "line": "  private metadataRef: React.RefObject<HTMLDivElement> = React.createRef();",
[lint-deps    ]     "reasonCategory": "falseMatch|testCode|exampleCode|otherUtilityCode|regexMatchedSafeCode|notExercisedByOurApp|ruleNeeded|usageTrusted",
[lint-deps    ]     "updated": "2023-06-30T22:12:49.259Z",
[lint-deps    ]     "reasonDetail": "<optional>"
[lint-deps    ]   }
[lint-deps    ] ]

@scottnonnenberg-signal
Copy link
Contributor

That error is coming from yarn lint-deps - you'll need to update ts/util/lint/exceptions.json with the new ref usage you've added. You can copy that 'questionable lines' output into the JSON directly, then modify. It's a system that prevents us from mindlessly adding things which are a little risky.

@scottnonnenberg-signal
Copy link
Contributor

Thanks for all your efforts on this! We've merged it internally, and it will be in the next beta family we release - probably 6.26.x.

@scottnonnenberg-signal
Copy link
Contributor

(if you keep this PR open, it will automatically be marked as merged when we push our internal merge commits to a public branch)

@indutny-signal indutny-signal merged commit b292568 into signalapp:main Jul 13, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

triple-clicking on message body to copy text includes timestamp
3 participants