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

feat: selectionHandleColor prop on Android #41092

Closed
wants to merge 5 commits into from

Conversation

jakex7
Copy link
Contributor

@jakex7 jakex7 commented Oct 19, 2023

Summary:

This PR addresses the problem raised in the #41004 issue.

The current logic is that selectionColor on iOS sets the color of the selection, handles, and cursor. On Android it looks similar, while it doesn't change the color of the handles if the API level is higher than 27. In addition, on Android there was an option to set the color of the cursor by cursorColor prop, but it didn't work if the selectionCursor was set.

Changelog:

[GENERAL] [ADDED] - Make same behavior of the selectionColor prop on Android as iOS
[ANDROID] [ADDED] - Introduced selectionHandleColor as a separate prop
[ANDROID] [CHANGED] - Allowing cursorColor and selectionHandleColor to override selectionColor on Android

Test Plan:

Manual tests in rn-tester:

selectionColor same as iOS, sets selection, handles and cursor color

There is a way to set only "rectangle" color by setting other props as null

image

selectionHandleColor

image

cursorColor

image

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 19, 2023
@jakex7 jakex7 force-pushed the feature/selectionHandleColor branch from 1521e8b to eb5647b Compare October 19, 2023 20:20
@jakex7 jakex7 force-pushed the feature/selectionHandleColor branch from eb5647b to c411416 Compare October 19, 2023 20:21
@analysis-bot
Copy link

analysis-bot commented Oct 19, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,649,219 -4,445
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,026,811 -6,835
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: c267a06
Branch: main

@javache
Copy link
Member

javache commented Nov 3, 2023

Thanks for contributing!

Having props that overlap require careful handling. If in a state update you change just selectionColor, the previous value set for selectionHandleColor will be overwritten. There's also no guarantee to which order setters are called.

Can you validate that when you change these properties in different order (eg add useState hook to control each individually) that this still works correctly?

@jakex7
Copy link
Contributor Author

jakex7 commented Nov 6, 2023

Hi @javache,
I've tested this in rn-tester and can confirm that changing in different order works correctly. selectionHandleColor and cursorColor will overwrite selectionColor regardless of the setting order. Below is a short video of this test.

During the testing, I also found incorrect behavior when changing from color to undefined so I uploaded a fix.

selectionHandleColor-order-test.mov

… they're not both in otherProps and colorProps.
Copy link

Warnings
⚠️

packages/rn-tester/js/examples/TextInput/TextInputExample.android.js#L13 - packages/rn-tester/js/examples/TextInput/TextInputExample.android.js line 13 – Requires should be sorted alphabetically (lint/sort-imports)

Generated by 🚫 dangerJS against fb1435b

@facebook-github-bot
Copy link
Contributor

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 23, 2023
@facebook-github-bot
Copy link
Contributor

@javache merged this pull request in 1e68e48.

Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
Summary:
This PR addresses the problem raised in the facebook#41004 issue.

The current logic is that `selectionColor` on iOS sets the color of the selection, handles, and cursor. On Android it looks similar, while it doesn't change the color of the handles if the API level is higher than 27. In addition, on Android there was an option to set the color of the cursor by `cursorColor` prop, but it didn't work if the `selectionCursor` was set.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[GENERAL] [ADDED] - Make same behavior of the `selectionColor` prop on Android as iOS
[ANDROID] [ADDED] - Introduced `selectionHandleColor` as a separate prop
[ANDROID] [CHANGED] - Allowing `cursorColor` and `selectionHandleColor` to override `selectionColor` on Android

Pull Request resolved: facebook#41092

Test Plan:
Manual tests in rn-tester:

### `selectionColor` same as iOS, sets selection, handles and cursor color

_There is a way to set only "rectangle" color by setting other props as null_

![image](https://github.com/facebook/react-native/assets/39670088/9cba34c2-c9fc-4d84-a9cb-3b28a754671d)

### `selectionHandleColor`

![image](https://github.com/facebook/react-native/assets/39670088/8a7e488e-0e35-4646-9efe-4783420b41fa)

### `cursorColor`

![image](https://github.com/facebook/react-native/assets/39670088/06798b8a-851f-44c7-979e-a4e74681b29a)

Reviewed By: NickGerleman

Differential Revision: D51253298

Pulled By: javache

fbshipit-source-id: 290284aa38c6ba0aa6998b937258788ce6376431
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants