Allow configuration of selection/caret color on Aztec view. #1776

SergioEstevao · 2020-01-14T15:11:07Z

Fixes #1773

This PR adds a prop to RCTAztecView to allow configuration of the selection/caret color

To test:

Change the code on one the block that uses the RichText component, and add a prop like this
selectionColor= { 'orange' }
Start the demo app
Add the block changed
Check that the caret color and selection are correct.

PR submission checklist:

I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

SergioEstevao · 2020-01-14T15:14:19Z

@chipsnyder Feel free to add the necessary changes for Android on this PR.

chipsnyder · 2020-01-14T15:38:50Z

Will do, I'll submit those changes today.

chipsnyder · 2020-01-14T20:11:33Z

Added the cursor color and highlight color based on React Native's implementation:
https://github.com/facebook/react-native/blob/233fdfc014bb4b919c7624c90e5dac614479076f/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L422-L457

Note this approach doesn't work on API 28+ because of Google's restrictions on reflection
facebook/react-native#22762

Once API 29 is supported this can be migrated to use https://developer.android.com/reference/android/widget/TextView#setTextCursorDrawable(android.graphics.drawable.Drawable)

Cursor	Highlight

lukewalczak · 2020-01-15T11:04:47Z

Hello @SergioEstevao @chipsnyder, thanks for a PR!

I've tested your feature on:

WordPress-iOS

caret color ✅	selection color ✅

WordPress-Android API 27

caret color ✅	selection color ✅

WordPress-Android API 29

caret color ❎	selection color ✅

I was also testing colors passed as:

rgb()✅
string ✅
hex ✅

@dratwas could you please check the code?

dratwas · 2020-01-15T14:42:11Z

LGTM! 👍

pinarol · 2020-01-16T16:27:55Z

...native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java

+            // Get the original cursor drawable resource.
+            Field cursorDrawableResField = TextView.class.getDeclaredField("mCursorDrawableRes");
+            cursorDrawableResField.setAccessible(true);
+            int drawableResId = cursorDrawableResField.getInt(this);
+
+            // The view has no cursor drawable.
+            if (drawableResId == 0) {
+                return;
+            }
+
+            Drawable drawable = ContextCompat.getDrawable(getContext(), drawableResId);
+            if (color != null) {
+                drawable.setColorFilter(color, PorterDuff.Mode.SRC_IN);
+            }
+            Drawable[] drawables = {drawable, drawable};
+
+            // Update the current cursor drawable with the new one.
+            Field editorField = TextView.class.getDeclaredField("mEditor");
+            editorField.setAccessible(true);
+            Object editor = editorField.get(this);
+            Field cursorDrawableField = editor.getClass().getDeclaredField("mCursorDrawable");
+            cursorDrawableField.setAccessible(true);
+            cursorDrawableField.set(editor, drawables);


I am not understanding the logic here. Why do we need to set accessible=true in multiple places? Is that necessary to be able to set cursor color? @chipsnyder

This method is a bit of a hack to get to the desired goal, but I can add some more info. In Android, the cursor color is managed by the theme and not on a control by control basis. You can set it in XML via android:textCursorDrawable. However there isn't an officially supported way to do change this programmatically in API 28 and below.

In API 27 and below, you can use reflection to dynamically lookup the drawable for the current TextView and replace it with a new drawable. Setting setAccessible to true allows us to make the changes we need through reflection. Without that set to true, the app will throw IllegalAccessException

Here's a breakdown of where we set it.

Variable How we use setAccessible

cursorDrawableResField Allows the lookup ofdrawableResId so we can clone it and change the color

editorField So we can get access to the object the holds the cursor drawable

cursorDrawableField So we can update that with the new drawable we created

I can go through and add more step by step inline documentation if you think that will help.

Also worth noting starting in API 28, Google restricts the use of reflection. So this won't work in API 28 and above. Once we update Aztec to target API 29 though we can set the drawable using setTextCursorDrawable

Thanks for the explanation. We need to make sure the screen reader doesn't say weird things after making these fields accessible. Could you be able to set selectionColor on a RichText and test accessibility/screen reader? Thanks!

API 29 supports setTextCursorDrawable which would be a cleaner way to handle this when an upgrade to that API level occurs.

Also, could you open a tech debt task in this repo and add it to our board to help us remember this in the future?

Could you be able to set selectionColor on a RichText and test accessibility/screen reader?

I tested this on my Pixel 3a, which runs API 29, and an API 24 emulator after installing https://play.google.com/store/apps/details?id=com.google.android.marvin.talkback&hl=en_US in the emulator. From my testing, it's behaving the same both before and after this change.

could you open a tech debt task

Just created that here: #1787

pinarol · 2020-01-16T16:49:14Z

As far as I see, the output is different on both platforms. On Android, the selection color looks like exactly same with cursor color. But on iOS, selection color seems more opaque(because that's simply how tintColor works so it is supported by system). I am curious if this can go wrong if we want to set selection color to the same color with font color. Could we also test that case?

We need this for Button block, and in Button block both background color and the font color is configurable. So we'll probably need to set the selectionColor=fontColor.

chipsnyder · 2020-01-16T22:30:16Z

I am curious if this can go wrong if we want to set selection color to the same color with font color.

That's a good point. I didn't think about that. Testing this out if I set the selection color to be black, you end up with something like this in Android:

We can set it so that the highlight color is 50% of either the selection color (which will control the cursor) or the font color. So you end up with something more like this if the font color and selection color match.

We could really go either way with setting the highlight to match either 50% alpha of either the font or selection color. I think having it match the selection color would be the most consistent with how iOS works.

I went ahead and made that change. Just let me know if you would like it to be based on the font color, and I'll make the adjustment.

…r color

pinarol · 2020-01-20T10:13:43Z

...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java

@@ -362,6 +364,17 @@ public void setColor(ReactAztecText view, @Nullable Integer color) {
        view.setTextColor(newColor);
    }

+    @ReactProp(name = "selectionColor", customType = "Color")
+    public void setSelectionColor(ReactAztecText view, @Nullable Integer color) {
+        int newColor = Color.BLACK;


I'd expect this method to change nothing if the passed color is null. Currently it is proceeding with Black.

When I first did this I was pretty much just following the pattern in setting color. I was basically thinking they set it they should see some change. I agree though having it make no changes seems to be the best path.

pinarol · 2020-01-20T10:20:48Z

We could really go either way with setting the highlight to match either 50% alpha of either the font or selection color. I think having it match the selection color would be the most consistent with how iOS works.

I'd like to go with lowered opacity either, however, do you also think it looks more like a gray rather than a lowered opacity black? 🤔Is that possible we are missing something?

chipsnyder · 2020-01-20T14:50:58Z

do you also think it looks more like a gray rather than a lowered opacity black?

When I change the background color to a light blue, then it's easier to see the transparency. Comparing iOS and Android, they look pretty similar, although Android might be a touch more opaque. I looked up the iOS applied tint and based on the interface inspector, they use 20% alpha. Here's the comparison. The 20% seems light to me. So I played around and set it to 33%, which I feel is a decent balance between the two. Let me know your thoughts, and I can play around with it more.

Platform	Screenshot
iOS
Android 50%
Android 33%
Android 20%

cc @iamthomasbishop

… colors if the new color in null

iamthomasbishop · 2020-01-20T16:44:02Z

@chipsnyder I haven't had a chance to dig through the whole comment history here, but looking at your examples in the previous comment, it looks as if iOS puts the selection highlight over the top of the text, whereas Android is behind the text — is that right or just my imagination?

Is there a "default" highlight alpha on Android? 33% looks fine, but I'm wondering what Android uses by default, because if it's close to 20% maybe we should align both platforms closer to that ballpark.

chipsnyder · 2020-01-20T17:13:02Z

@iamthomasbishop

highlight over the top of the text, whereas Android is behind the text — is that right or just my imagination?

I think that's right, the default behavior of Android when setting the color is to use the color as provided. So given black, you'll get black with an alpha of 100% like in the first image here #1776 (comment).

When I was debugging, the default color was a lighter shade of the cursor but rendered with full alpha.

iamthomasbishop · 2020-01-20T17:18:54Z

I think that's right, the default behavior of Android when setting the color is to use the color as provided. So given black, you'll get black with an alpha of 100% like in the first image here #1776 (comment).

Ah, ok. So is the iOS screenshot here using 20% or 33%? The iOS example looks a bit stronger, but maybe that's just because it's over the text vs. under. Both look nice, tbh, so if I had to choose between 20% and 33% I'd choose the one that provides a stronger contrast.

chipsnyder · 2020-01-20T17:29:49Z

The iOS example is 20%, which is outside of our control to tweak. With Android, we can go with any of those options, or I can create some more options if you'd like to fine-tune it.

pinarol · 2020-01-20T17:48:30Z

Thanks for listing the options @chipsnyder! Lowering the alpha seems helping a lot. One more thing that can help us decide would be again using the same selection color with font color. As I mentioned, both background color and the font color is dynamically configurable for Button block. So we'll probably need to set the selectionColor=fontColor there.

chipsnyder · 2020-01-20T18:59:00Z

same selection color with font color

Sure thing. I have a few instances above that use a black font and selection colors. I'll consolidate them here and add some white font/selection color examples as well. Or were you thinking more directly of not having the selectionColor be its own property and base it on the font only?

Platform	Black Font/Selection	White Font/Selection
iOS *
Android 100%
Android 50%
Android 33%
Android 20%

* System set to 20% of Tint color

On a side note, a friend of mine shared with me a few ideas on how to update the bubble colors using the same technique. I'll update this PR with those changes as well.

pinarol · 2020-01-20T19:52:27Z

@chipsnyder Thank you very much! Maybe it is because i am an iOS user and that’s what i am used to see but %20 looks best imho.

Or were you thinking more directly of not having the selectionColor be its own property and base it on the font only?

No, i think we do need the selectionColor prop. I was just trying to say in case of the Button block we’ll probably need to make it same color with font. It is not a general rule.

On a side note, a friend of mine shared with me a few ideas on how to update the bubble colors using the same technique. I'll update this PR with those changes as well.

Great! Hope it won’t be much trouble.

chipsnyder · 2020-01-20T19:58:50Z

i am an iOS user and that’s what i am used to see but %20 looks best imho.

I'm good with the plan of setting it to be 20% if you think that looks better. I use iOS as my daily driver too but I think what @iamthomasbishop pointed out, the highlight is behind the text on Android, is what made the difference to me. Just pushed that change with the pointer color as well.

pinarol · 2020-01-20T21:25:43Z

I’d prefer deferring the last decision to @iamthomasbishop on this.
And bubbles look better now, thanks!

etoledom · 2020-01-20T22:45:12Z

Moved milestone to v1.22.
Please feel free to re-target if you consider this PR must be shipped together with v1.21.

chipsnyder · 2020-01-22T13:52:59Z

@iamthomasbishop

I’d prefer deferring the last decision to @iamthomasbishop on this.

Do you have any preference for which transparency setting to use for Android?
Here's a break down of each option: #1776 (comment)

iamthomasbishop · 2020-01-22T17:53:27Z

Do you have any preference for which transparency setting to use for Android?

20% looks good to me, we can roll with that!

chipsnyder · 2020-01-22T19:38:35Z

20% looks good to me, we can roll with that!

This PR is good to go then from the Android side. I'll work with @SergioEstevao to update the Gutenberg reference. The failing test here seems to be something that is failing routinely in develop

…ion_to_change_caret_color

SergioEstevao · 2020-01-27T11:51:51Z

@lukewalczak can you give the final approval on this PR?

lukewalczak · 2020-01-27T12:06:42Z

I'm happy to use that feature within Button block 🎉

Allow configuration of selection color on Aztec view.

a381773

SergioEstevao added the [Status] Needs Android Dev label Jan 14, 2020

SergioEstevao added this to the 1.21 milestone Jan 14, 2020

SergioEstevao requested a review from chipsnyder January 14, 2020 15:11

SergioEstevao assigned chipsnyder Jan 14, 2020

SergioEstevao requested review from lukewalczak and removed request for chipsnyder January 14, 2020 15:11

SergioEstevao mentioned this pull request Jan 14, 2020

Add a prop to RichText native to allow configuration of selection/caret color WordPress/gutenberg#19635

Merged

6 tasks

Add selection Highlight color to Android

0a02c76

chipsnyder removed the [Status] Needs Android Dev label Jan 14, 2020

Format cursor color based on selectionColor setting

a4bbb1d

pinarol reviewed Jan 16, 2020

View reviewed changes

Update the highlight color to be a semi-transparent form of the curso…

5bd2eaf

…r color

pinarol reviewed Jan 20, 2020

View reviewed changes

Adjusted the alpha of the highlight color and fellback to the default…

d9f9d9c

… colors if the new color in null

Update cursor to coloring to include the selector icons

86e20b2

Update the highlight transparency to be 20 percent alpha

3235dc1

etoledom modified the milestones: 1.21, 1.22 Jan 20, 2020

chipsnyder added the [Status] Needs Design Review Needs design review or sign-off before shipping label Jan 22, 2020

chipsnyder removed the [Status] Needs Design Review Needs design review or sign-off before shipping label Jan 22, 2020

SergioEstevao added 2 commits January 24, 2020 14:05

Update GB reference.

90f8130

Merge branch 'develop' into issue/1773_add_option_to_change_caret_color

13ece3f

chipsnyder force-pushed the issue/1773_add_option_to_change_caret_color branch from 74acf9b to 13ece3f Compare January 24, 2020 20:25

chipsnyder and others added 2 commits January 24, 2020 15:29

Merge remote-tracking branch 'origin/develop' into issue/1773_add_opt…

e5d8aeb

…ion_to_change_caret_color

Merge branch 'develop' into issue/1773_add_option_to_change_caret_color

3296ed4

lukewalczak approved these changes Jan 27, 2020

View reviewed changes

SergioEstevao merged commit f284931 into develop Jan 27, 2020

SergioEstevao deleted the issue/1773_add_option_to_change_caret_color branch January 27, 2020 13:34

This was referenced Feb 3, 2020

Release v1.22.0 #1849

Merged

Gutenberg: update to v1.22.0 wordpress-mobile/WordPress-iOS#13353

Merged

Gutenberg: update to v1.22.0 wordpress-mobile/WordPress-Android#11224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration of selection/caret color on Aztec view. #1776

Allow configuration of selection/caret color on Aztec view. #1776

SergioEstevao commented Jan 14, 2020 •

edited

Loading

SergioEstevao commented Jan 14, 2020

chipsnyder commented Jan 14, 2020

chipsnyder commented Jan 14, 2020 •

edited

Loading

lukewalczak commented Jan 15, 2020

dratwas commented Jan 15, 2020

pinarol Jan 16, 2020

chipsnyder Jan 16, 2020

pinarol Jan 20, 2020

pinarol Jan 20, 2020

chipsnyder Jan 20, 2020

pinarol commented Jan 16, 2020 •

edited

Loading

chipsnyder commented Jan 16, 2020

pinarol Jan 20, 2020

chipsnyder Jan 20, 2020

pinarol commented Jan 20, 2020 •

edited

Loading

chipsnyder commented Jan 20, 2020 •

edited

Loading

iamthomasbishop commented Jan 20, 2020

chipsnyder commented Jan 20, 2020

iamthomasbishop commented Jan 20, 2020

chipsnyder commented Jan 20, 2020

pinarol commented Jan 20, 2020 •

edited

Loading

chipsnyder commented Jan 20, 2020

pinarol commented Jan 20, 2020

chipsnyder commented Jan 20, 2020

pinarol commented Jan 20, 2020

etoledom commented Jan 20, 2020

chipsnyder commented Jan 22, 2020

iamthomasbishop commented Jan 22, 2020

chipsnyder commented Jan 22, 2020

SergioEstevao commented Jan 27, 2020

lukewalczak commented Jan 27, 2020

Variable	How we use `setAccessible`
cursorDrawableResField	Allows the lookup of`drawableResId` so we can clone it and change the color
editorField	So we can get access to the object the holds the cursor drawable
cursorDrawableField	So we can update that with the new drawable we created

Allow configuration of selection/caret color on Aztec view. #1776

Allow configuration of selection/caret color on Aztec view. #1776

Conversation

SergioEstevao commented Jan 14, 2020 • edited Loading

SergioEstevao commented Jan 14, 2020

chipsnyder commented Jan 14, 2020

chipsnyder commented Jan 14, 2020 • edited Loading

lukewalczak commented Jan 15, 2020

dratwas commented Jan 15, 2020

pinarol Jan 16, 2020

Choose a reason for hiding this comment

chipsnyder Jan 16, 2020

Choose a reason for hiding this comment

pinarol Jan 20, 2020

Choose a reason for hiding this comment

pinarol Jan 20, 2020

Choose a reason for hiding this comment

chipsnyder Jan 20, 2020

Choose a reason for hiding this comment

pinarol commented Jan 16, 2020 • edited Loading

chipsnyder commented Jan 16, 2020

pinarol Jan 20, 2020

Choose a reason for hiding this comment

chipsnyder Jan 20, 2020

Choose a reason for hiding this comment

pinarol commented Jan 20, 2020 • edited Loading

chipsnyder commented Jan 20, 2020 • edited Loading

iamthomasbishop commented Jan 20, 2020

chipsnyder commented Jan 20, 2020

iamthomasbishop commented Jan 20, 2020

chipsnyder commented Jan 20, 2020

pinarol commented Jan 20, 2020 • edited Loading

chipsnyder commented Jan 20, 2020

pinarol commented Jan 20, 2020

chipsnyder commented Jan 20, 2020

pinarol commented Jan 20, 2020

etoledom commented Jan 20, 2020

chipsnyder commented Jan 22, 2020

iamthomasbishop commented Jan 22, 2020

chipsnyder commented Jan 22, 2020

SergioEstevao commented Jan 27, 2020

lukewalczak commented Jan 27, 2020

SergioEstevao commented Jan 14, 2020 •

edited

Loading

chipsnyder commented Jan 14, 2020 •

edited

Loading

pinarol commented Jan 16, 2020 •

edited

Loading

pinarol commented Jan 20, 2020 •

edited

Loading

chipsnyder commented Jan 20, 2020 •

edited

Loading

pinarol commented Jan 20, 2020 •

edited

Loading