Add clear button to text inputs

[derekblank]

Description

Addresses wordpress-mobile/gutenberg-mobile#3017 by adding clear button behavior to certain TextInput controls within BottomSheet. See #3017 for further discussion on how text input behavior may be standardized.

How has this been tested?

Test Case 1: Display the Clear Button and Clear Text Input

• Open the app with metro running.
• Add a Social Icons block.
• Tap in to a Social Icon to invoke the Link Settings modal.
• Tap "URL" or "Link label" to focus the Text Input.
• Add text to the Text Input via the keyboard.
• Expect: A clear button should appear if text value has been entered.
• If text has been entered and the clear button is displayed, tap the Clear Button.
• Expect: Any text input value should disappear, and placeholder text should be shown; Clear Button should not be visible.

Test Case 2: Do not display the Clear Button for Plain Text Editor components

• Open the app with metro running.
• Add a Cover block.
• Tap into the Cover caption Text Input to add a caption or text.
• Expect: A clear button should not appear if text value has been entered.

Screenshots

Android, dark mode:

android.mov

iOS, light mode:

gutenberg.mov

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @derekblank! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[fluiddot]

Thank @derekblank for tackling this issue 🙇 !

Here are some comments gathered after doing a first review on the draft PR. On the other hand, I noticed that unit tests are failing, so we might consider investigating the culprit of the errors, in case it uncovers any potential issues from the changes.

[derekblank]

Thanks for the feedback @fluiddot ! After considering your comments, and thinking through how we might resolve some of the edge cases like retaining the clipboardUrl in LinkPicker, I realized that a better approach would be to provide an onClear method as a prop to BottomSheet.Cell. This way, each use of displayClearButton in BottomSheet.Cell would be accompanied by its own onClear handler that the clear button itself could invoke, so that the TextInput value can be handled directly in the parent component of wherever BottomSheet.Cell is being used:

const clearInput = () => {
    setText( '' );
};

<BottomSheet.Cell
    displayClearButton={true}
    onClear={() => clearInput()}
    value={text}
    ...
/>

It is an extra prop and function, yes, but I think it is much cleaner as it eliminates the need for a localValue state within BottomSheet.Cell, and it allows the parent component to maintain control of the value and provide extra parameters like setValue( { value, clipboardUrl: '' } ) when needed. In addition, this change resolves several of your other comments on the code, but that feedback was helpful. Let me know your thoughts on this approach! 🙇

[fluiddot]

I noticed the clear icon's color is now using the values from iconStyleBase instead of having a specific one as we had with clearButtonIconStyle, it would be great if we could double-check that those colors are the good ones.

[derekblank]

I agree with you here that we may wish to have more fine-grained control over the clear button icon color. This code change here was actually an attempt to consolidate color variables a bit, and reduce some of the styling complexity as the colors for icon and iconDark were already very similar to the new clear button icon colors (which I had brought from LinkPicker directly). With regard to which colors are the "right" ones, there seems to be a bit of variance throughout the app with matching the true native platform UI (if that is even the desired goal, of course). I did note that there is a bit of a difference between the iOS native clear button color, and how we are implementing it. Here are some examples of BottomSheet.Cell usage (top) and Search Control (bottom). In both screenshots, the native iOS clear button (using clearButtonMode) is on the left, and the Javascript button is on the right:

Link Settings

Link Settings is an example of the BottomSheet.Cell clear button behavior introduced in this PR. (Native iOS clear button on the left, Javascript button on the right using the existing icon and iconDark colors.)

Search Control

Search Control implements a different clear button solution, using similar colors. (Native iOS clear button is on the left, Javascript on the right.)

These color differences are more noticeable on iOS because the icon has more surface area, but the result is also very similar on Android. While none of the colors in the examples seem "out of place" or unsympathetic to the design, I think there is room for some styling consolidation as part of the next iteration. I'd be interested to know your thoughts on if there is existing styling guidelines for matching the exact native platform colors when re-implementing platform-specific behavior in Javascript. I think it would be an interesting conversation to start, if not.

[fluiddot]

Great investigation @derekblank! I agree that none of the examples look incoherent in terms of styling, so I'd stick with your approach. However, I'd like to ping here @iamthomasbishop, in case he could provide further insights regarding this topic.

@iamthomasbishop I'd appreciate it if you could take a look at this and confirm the right colors for the clear input button, thank you very much for the help 🙇 !

[fluiddot]

Thanks for the feedback @fluiddot ! After considering your comments, and thinking through how we might resolve some of the edge cases like retaining the clipboardUrl in LinkPicker, I realized that a better approach would be to provide an onClear method as a prop to BottomSheet.Cell. This way, each use of displayClearButton in BottomSheet.Cell would be accompanied by its own onClear handler that the clear button itself could invoke, so that the TextInput value can be handled directly in the parent component of wherever BottomSheet.Cell is being used:

const clearInput = () => {
    setText( '' );
};

<BottomSheet.Cell
    displayClearButton={true}
    onClear={() => clearInput()}
    value={text}
    ...
/>

It is an extra prop and function, yes, but I think it is much cleaner as it eliminates the need for a localValue state within BottomSheet.Cell, and it allows the parent component to maintain control of the value and provide extra parameters like setValue( { value, clipboardUrl: '' } ) when needed. In addition, this change resolves several of your other comments on the code, but that feedback was helpful. Let me know your thoughts on this approach! 🙇

Good job on addressing the issue 🎊 . I like the approach of delegating the clear input functionality to the invoker, this way we make the component agnostic to the data management.

[fluiddot]

@derekblank I've tested the changes in both iOS and Android and identified the following issues, none of them should block the PR but I think it would be great to try addressing them:

Android - Visual glitch

I noticed that on Android the content of empty text inputs is aligned to left and when introducing a value they change the alignment to right. This is a visual glitch that is present in the current version so it's not produced by these changes, however, in combination with displaying the clear button, it makes it more prominent leading to a minor visual glitch (see attached video captures).

Click here to display video captures

Current version	Including clear button
no-clear-button.mp4	with-clear-button.mp4

Content size reduced by clear button

In some cases where in the same row we present the text input along with an icon and text label, like for example when editing the URL of an embed block, the available size for the content is reduced when the clear button is displayed resulting in a small area (see attached screenshot).

Click here to display screenshot

This is more a design issue than implementation but I'm wondering if we could figure out a solution eventually until further design feedback is provided, wdyt?

cc @iamthomasbishop

Enable clear button in Link rel input

While I was testing the Image block, I noticed that the Link Rel input could be benefited from the clear input button functionality (see attached screenshot) as it's a similar input to the Link label used on other blocks. I'm wondering if we could enable it there too, wdyt?

Click here to display screenshot

---

Apart from the mentioned issues, the PR looks great to me 🎊 . As the last step and since the new functionality is part of the Cell component which is a core piece of the editor, I'm wondering if we could add some unit tests to cover the changes introduced in the PR.

[derekblank]

@fluiddot Thanks for the thorough review! These are some good points. Per the last point:

Enable clear button in Link rel input

While I was testing the Image block, I noticed that the Link Rel input could be benefited from the clear input button functionality (see attached screenshot) as it's a similar input to the Link label used on other blocks. I'm wondering if we could enable it there too, wdyt?

This is a good callout. I've went ahead and updated this component to add the clear button behavior to Link Rel on LinkSettings.

Regarding the other two points (the Android visual glitch and the content size design issue), I have done some further investigations here on these two specific issues that I will post my thoughts on these here shortly. Also, I agree with you that the next steps would be to add some basic unit testing for clear button behavior on the BottomSheet Cell component.

[fluiddot]

This is a good callout. I've went ahead and updated this component to add the clear button behavior to Link Rel on LinkSettings.

Awesome! Thanks for applying the suggestion 🙇 .

Regarding the other two points (the Android visual glitch and the content size design issue), I have done some further investigations here on these two specific issues that I will post my thoughts on these here shortly

Great, I look forward to your investigations 🔍 .

---

@derekblank I created a test PR in the Gutenberg Mobile repository (reference) in order to verify that E2E tests and CI checks are passing. It would be great if you could take a look in case any of them fail and require further fixing, thanks!

[derekblank]

@fluiddot Just following up with updates to the other two of your comments:

1. Android - Visual glitch

I noticed that on Android the content of empty text inputs is aligned to left and when introducing a value they change the alignment to right. This is a visual glitch that is present in the current version so it's not produced by these changes, however, in combination with displaying the clear button, it makes it more prominent leading to a minor visual glitch (see attached video captures).

This is a good callout, but it is also interesting because when focusing the TextInput on my testing device, the cursor appears to the right of the placeholder text by default, unlike your example where the cursor appears on the left of the TextInput before any text is actually added. This may be an OS-level difference between Android devices or Android versions -- I'm not entirely sure at this point without investigating further with different test devices. (I am using a Google Pixel 6 on Android 12, for reference.) I know that it is possible with React Native's TextInput to modify the start and end value of the text selection's position using the selection property, which may help in this case. I am curious which device and OS you are using to reproduce this, however. Regardless, I experienced the same glitchy behavior you mentioned, even with the cursor position already set to the "right" of the TextInput placeholder...so while it is certainly an issue, I'm not entirely certain that it is caused by the right-vs-left position of the cursor. Investigating further, I did a 60fps screen recording of an interaction with the TextInput, and broke out every frame into a separate image file using ffmpeg. Here's an example of the output:

What I interpreted was that between Frame 8 and Frame 11, there was a three frame gap where the actual text in the text input appeared, but the clear button did not appear until three frames later. (The gap from Frame 1 to Frame 8 was just me moving my finger from the input to the keyboard.) Without a bit more exploration, my best guess as to why this is the case is because the presence of the clear button in BottomSheet.Cell is checking for the existence of the TextInput value, which must be passed up to the parent component and then back down again to BottomSheet.Cell.:

{ value !== '' && displayClearButton && clearButton() }

This behavior only shows the clear button when the TextInput actually has some value, but it also leads to the "glitchy" behavior that is more noticeable on right-aligned TextInputs. Looking into the behavior further of what else we might be able to hook into to display the clear button, I tried changing the check for any TextInput value to local state when the input is focused, like this:

{this.state.isEditingValue && displayClearButton && clearButton() }

This now displays the clear input button whenever the text input is focused, regardless of when the TextInput actually has value. This reduces the glitchy behavior (especially on right-aligned TextInputs). Here's an example in action:

Click to display persistent Clear Button via `this.state.isEditingValue`

showClearButton.mov

So, while the clear button displays immediately on TextInput focus without any TextInput value, and it reduces the glitchy behavior, this differs a bit from the previous behavior where the clear button only appears once value has been entered. I did not push up this change, as I wanted to discuss and get some more input. I think we could make a case for moving to this behavior, as there is a native precedent for this on iOS on the clearButtonMode while-editing property. Another alternative would be to continue investing ways to speed up the value !== '' check, or some other creative ways to make the behavior appear less glitchy in the UI (since it mostly affects right aligned inputs). What do you think?

2. Content size reduced by clear button

In some cases where in the same row we present the text input along with an icon and text label, like for example when editing the URL of an embed block, the available size for the content is reduced when the clear button is displayed resulting in a small area (see attached screenshot).

I agree that it is not ideal that the clear button takes up even more space here. One possible implementation solution would be to reduce the left margin of the clear button container for 16 to 12, to match the text block on the left side. That's not much gain, of course, but it is a small amount. Another option that would be more of a design consideration would be to explore moving these text blocks out of the input row to free up more horizontal space -- perhaps above, below, or even explore placeholder text options as well? Just some initial ideas.

[fluiddot]

@derekblank Thanks for investigating both issues, you did a superb investigation 🙇 .

1. Android - Visual glitch

This is a good callout, but it is also interesting because when focusing the TextInput on my testing device, the cursor appears to the right of the placeholder text by default, unlike your example where the cursor appears on the left of the TextInput before any text is actually added. This may be an OS-level difference between Android devices or Android versions -- I'm not entirely sure at this point without investigating further with different test devices. (I am using a Google Pixel 6 on Android 12, for reference.) I know that it is possible with React Native's TextInput to modify the start and end value of the text selection's position using the selection property, which may help in this case. I am curious which device and OS you are using to reproduce this

Interesting, I didn't realize that this might be different depending on the device/OS version, in my case I'm using the device Samsung Galaxy S20 FE 5G (Android 11).

Not sure if the selection prop would help since I only managed to reproduce this when the input is empty. As you mentioned, this might be an issue on the Android side that doesn't preserve alignment for the placeholder element 🤔 .

Regardless, I experienced the same glitchy behavior you mentioned, even with the cursor position already set to the "right" of the TextInput placeholder...so while it is certainly an issue, I'm not entirely certain that it is caused by the right-vs-left position of the cursor. Investigating further, I did a 60fps screen recording of an interaction with the TextInput, and broke out every frame into a separate image file using ffmpeg. Here's an example of the output:

What I interpreted was that between Frame 8 and Frame 11, there was a three frame gap where the actual text in the text input appeared, but the clear button did not appear until three frames later. (The gap from Frame 1 to Frame 8 was just me moving my finger from the input to the keyboard.) Without a bit more exploration, my best guess as to why this is the case is because the presence of the clear button in BottomSheet.Cell is checking for the existence of the TextInput value, which must be passed up to the parent component and then back down again to BottomSheet.Cell.:

{ value !== '' && displayClearButton && clearButton() }

This behavior only shows the clear button when the TextInput actually has some value, but it also leads to the "glitchy" behavior that is more noticeable on right-aligned TextInputs. Looking into the behavior further of what else we might be able to hook into to display the clear button, I tried changing the check for any TextInput value to local state when the input is focused, like this:

{this.state.isEditingValue && displayClearButton && clearButton() }

This now displays the clear input button whenever the text input is focused, regardless of when the TextInput actually has value. This reduces the glitchy behavior (especially on right-aligned TextInputs).

Great findings, I agree that this looks related to the fact that it requires multiple renders to determine whether to display the clear button, leading to the visual glitch. As you commented, this is prominent only in the right-aligned text inputs, so I'd focus on fixing that case. The approach about keeping the clear button even when it's empty would be a good workaround, however, I'm thinking of only applying it for the right-aligned case, wdyt?

2. Content size reduced by clear button

I agree that it is not ideal that the clear button takes up even more space here. One possible implementation solution would be to reduce the left margin of the clear button container for 16 to 12, to match the text block on the left side. That's not much gain, of course, but it is a small amount. Another option that would be more of a design consideration would be to explore moving these text blocks out of the input row to free up more horizontal space -- perhaps above, below, or even explore placeholder text options as well? Just some initial ideas.

I like the idea of reducing it even though it's a small amount, keep in mind that if we make it too small we might re-introduce the issue about overlapping the button with text selection (reference). Nevertheless, I'd like to know the feedback from @iamthomasbishop here, in case we could make any design tweak in order to improve tex inputs.

[fluiddot]

@derekblank I've just performed a review on the last changes, awesome work on adding the test cases for the Cell component 🥇 .

I added some comments regarding the addition of test IDs, let me know if you could take a look at them, thanks 🙇 !

[fluiddot]

@derekblank I noticed that the PR checks are not being triggered due to file conflicts, it would be great if you could solve them in order to make them pass, thanks 🙇 !

[iamthomasbishop]

@derekblank Thank you for tackling this, it's looking pretty solid. In the long-term, I would prefer to move to a more scalable solution for BottomSheet Text Inputs (more on that below) but I think we will be in good shape if we can solve the "jumpiness" that happens when focusing and adding text on right-aligned inputs.

I put together a quick Figma prototype to see if applying a quick/subtle transition would help and it does seem to feel a little more natural. I think I would prefer this solution over always showing the clear icon/button because that diverges from what is an established pattern. Here is an example using a quick (50ms) easeOut curve to change the x-position of the text label upon text-entry:

RPReplay_Final1641577455.mov

For the future

Note: This is probably beyond scope for this project, but it is related so I wanted to write it down so that it's documented.

As I mentioned above, I would prefer that we move to a more scalable model in general for text-entry — esp within BottomSheets. For context, we do already have something roughly equivalent to a textarea that we're using for alt-text settings on Image blocks but I think we want something more compact in most cases that resides within a single-line cell. Here are a couple of options I have in mind:

Option A (preferred)

The text-entry task is performed on a sub-sheet, which is accessed by tapping on the cell label/disclosure. This provides more space for the Text Input itself, but also allows us to provide additional information/contextual elements to users if necessary.

Option B

We maintain the same inline text-entry, but left-align the value/right side of the cell. This would be somewhat consistent with the "Left Detail" variation (UITableViewCellStyleValue2) from the Human Interface Guidelines, but would likely cause some awkward visual alignment issues. I would definitely prefer Option A over this.

[derekblank]

@iamthomasbishop Thanks for putting together the animation focusing on the jumpiness. I think this is a good place to start! My initial concern was that three frame gap between when the text input value appears, and when the logic of displaying the clear button can complete, so if we can't improve this, I think we can use the animation to help mask it.

Regarding the future implementation, I very much agree with your Option A plan -- this seems like a good approach to free up more horizontal space. As a side note, we'll be hitting pause on this work for the time being, but I would love to resume discussing this implementation later on.

[ttahmouch]

Not Crazy Important: The done callback usages will likely need to be removed when you update your branch because of recent changes related to jest-circus.
https://github.com/WordPress/gutenberg/pull/37715/files#r780551266

[derekblank]

@ttahmouch Thanks for the heads up! 👍