[RNMobile] Image Block: Improve Text Entry for Long Alt Text

[SiobhyB]

Fixes: wordpress-mobile/gutenberg-mobile#922

gutenberg-mobile: wordpress-mobile/gutenberg-mobile#3236

Description

With this PR, a new AltTextSettings component has been introduced to replace the current TextControl that exists for entering an image's Alt text. This new component makes use of BottomSheet.SubSheet so that users are taken to a new subsheet when entering alt text.

The end goal is to provide a better experience for users who are entering a longer alt text for their images, as the current text entry has limited room/options for scrolling.

How has this been tested?

The general steps for testing, for both iOS and Android, are as follows:

1. Open Image Block settings.
2. Tap Alt Text and confirm that a new sub sheet opens with a text editor.
3. Enter text into editor and tap the back arrow.
4. Confirm that the image's alt text has been correctly updated by viewing the placeholder displayed alongside Alt Text in the settings panel. This can be further verified by saving changes and confirming the alt text has been correctly updated on the web.

As this is a UI change, dark mode and landscape orientation are also considerations when testing.

Screenshots

Light mode	Dark mode

Types of changes

Non-breaking change. No additional functionality is introduced beyond UI enhancement.

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.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @SiobhyB! 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.

[SiobhyB]

Marking as ready for review. :) As well as being keen to hear comments/suggestions on the overall approach I took to address this issue, I'm also curious about the following:

• I used React Native's TextInput component instead of one of Gutenberg's components, such as TextareaControl, as it automatically increases in size based on the input. Is that an okay decision to make or is it preferable to use Gutenberg's compontents when possible?
• I made a guess at the colours, based on the _colors.native.scss file and also updated the initial colour I chose for the settings to meet WordPress' accessibility standards. Is there a better approach for determining the colours that should be used based on the design?

Thanks in advance!

[guarani]

This is looking solid, functionally it's working great (and I see the alt attribute in the editor's HTML). I have some comments to share about the styles (comparing to the design here):

Bottom margin too small

When adding alt text, on iOS the margin in-between the text and the horizontal line below looks too tight (some letters almost touch the line). On Android the margin actually looks too big.

iOS	Android

Horizontal line and color

On both iOS and Android, the horizontal line color looks a bit off (especially in dark mode) when comparing to other places in the app (like Cover block bottom sheet):

Image alt text on iOS	Cover block on iOS

Text size and color

Looking at the example design here, the text side and color for the "Describe the.." string seems a bit different here. I'm not sure if @iamthomasbishop specified a particular text size that he'd like, but we could probably reduce it a little.

Dark mode

On iOS and Android the text stays black on the dark background in my tests:

[guarani]

* I used React Native's `TextInput` component instead of one of Gutenberg's components, such as [TextareaControl](https://developer.wordpress.org/block-editor/reference-guides/components/textarea-control/), as it automatically increases in size based on the input. Is that an okay decision to make or is it preferable to use Gutenberg's compontents when possible?

Just regarding this, it looks like the TextareaControl has a native implementation that uses TextInput: https://github.com/WordPress/gutenberg/blame/trunk/packages/components/src/textarea-control/index.native.js#L14
Did you run into issues with it not automatically increasing in size when text was multi-line?

[iamthomasbishop]

Nice work so far, @SiobhyB! 👏 Happy to see this in the works!

I think @guarani can lend a better hand with implementing/matching colors, but I'll try to clarify some things regarding sizing/spacing:

I'm not sure if @iamthomasbishop specified a particular text size that he'd like, but we could probably reduce it a little.

That's right, I believe I used the section footer component in Figma, which we're using various places (i.e. Columns block settings). I can't recall if we have the exact thing as a structured component yet, but it looks like this:

Let me clarify what I was imagining for padding/sizing of the text area. If we use the main cell component as a staring point, and expand the text label area downward, we would have something like this:

I'm not sure if the padding-top is 100% accurate to the production component, but let's use the same value. What's different here is that we are setting a min-height for the input — I'm thinking 3 or 4 lines-worth of text, so in the ballpark of ~80px (for the inner part of the block with padding this would == ~108px).

[SiobhyB]

@guarani, @iamthomasbishop, thanks so much for the help here. 🙇‍♀️ I've made updates to change the styles, based on your feedback, and have taken screenshots of the results below.

iOS

Light mode	Dark mode

Android

Light mode	Dark mode

Notes

I found that the FooterMessageControl component was used to add a footer to the Columns block settings. I didn't use it for the footer note here as the design was slightly different and I couldn't find a simple way of incorporating a hyperlink using it. So, I instead relied on Text components for this purpose. Let me know if that seems okay or if you think another approach is warranted. :) Looking forward to hearing any further feedback, too!

[guarani]

I made a guess at the colours, based on the _colors.native.scss file and also updated the initial colour I chose for the settings to meet WordPress' accessibility standards. Is there a better approach for determining the colours that should be used based on the design?

I think your approach here is what I've used. Just want to mention that depending on what kind of changes are included in a PR, an installable build can be made for WPiOS and WPAndroid. If this is something that you'd like @iamthomasbishop, I'm available to help show you @SiobhyB how to make those.

Regarding the question of whether we should be using TextareaControl instead of a plain TextInput, I have no strong feelings either way. The code looks good and I've tested both platforms so this is about done. What's left from my perspective is to add a note to the change log here: https://github.com/WordPress/gutenberg/blob/trunk/packages/react-native-editor/CHANGELOG.md

[iamthomasbishop]

@SiobhyB Looking pretty close, for sure! I dug in a bit more and have a few questions/bits of feedback, which I'll try to find/drop inline.

[SiobhyB]

If we renamed it to something like BottomSheetTextControll and we could pass the different content that is specific to
current AltTextSettings into it would make it easier down the road the implement a similar feature on different blocks in the code base.

@enejb, this makes a lot of sense, thank you for the note! I've gone ahead to convert this to a more generic BottomSheetTextControl control now.

On a related note it would be great to add a quick Readme to the new AltTextSettings component this helps other developers know how to use it :)

I've also gone ahead to create a README.md now. :)

@enejb, @guarani, when you have a chance, I'd be really grateful for your eyes on these changes. Thank you again for all the help and input you've already provided. 🙇‍♀️

[enejb]

Thanks for changes @SiobhyB and adding the documentation! ❤️

I wonder the current FooterNote part of BottomSheetTextControl is a bit too inflexible.

Instead of passing in 3 different variables. We could pass in a single FooterNote component. That gets passed down into BottomSheetTextControl?

This way in case we want to add 2 links for example or it would work out nicer.

Also if we do take this approach a bit further we might also want to create a very simple FooterNote and FooterNoteLink components.

(Maybe this is taking things too far) But in the end we would end up with something like.

<BottomSheetTextControl
...
footerNote={
<FooterNote>
<>
    { __( 'Describe the purpose of the image. Leave empty if the image is purely decorative. ' ) }
    <FooterNoteLink url="'https://www.w3.org/WAI/tutorials/images/decision-tree/'">
      { __( 'What is alt text?' )}
    </FooterNoteLink>
    </>
</FooterNote>
}
/>

This way if we add footer notes to other bottom sheet control components they would all be the same styles.
I think the down side is that we are introducing more components that we might not need.

What do you think?

[SiobhyB]

Thank you again @enejb, this makes total sense and is a really valuable lesson for me to be mindful about "future-proofing" changes. :) I've split out two smaller components to make this more re-usable: SmallFooterMessageControl and SmallFooterMessageControlLink. The reason for the naming here is that we already have a FooterMessageControl control component which uses different styling (with larger text) to what we're using here.

Let me know if this looks okay to you!

[enejb]

Thanks for working on this. I have some more suggestions here :)

1. Instead of creating a new component SmallFooterMessageControl we can pass in a prop size="small" this aligns to the patterns that the new g2components are taking. See https://github.com/ItsJonQ/g2/tree/main/packages/components/src/Button#size.
   This would help with increasing the consistency across the different FooterMessage components. Right now we are introducing a new component that has a different size, colour, and text alignment.

cc @iamthomasbishop Can we align the two footer message components to always be the same? The previous one is currently only being used in the columns component as well as the columns picker component.

See (currently it looks like this)

If we applied the same new footer style (new colour, fontsize and alignment)

If we want to stick with the 2 different variations for now let's add the new one by passing in the correct attributes/props to the previous FooterMessageControl component instead of creating a new one.

<FooterMessageControl size="small" label={ footerNote } textAlign="left" />

This helps developers choose the right component in the future since there will be only one :)

2. I think we could omit the "control" suffix from SmallFooterMessageLinkControl name. Since the component doesn't control anything and it makes the component name more verbose. The same goes for the "Small" prefix. This way the new link component would be FooterMessageLink

3. I noticed that the new footer has cut off the bottom parts of some of the letters. See
   Notice y and g letters in the screenshot above.
   I am not sure what changes are now causing this.

4. I think it would help if the link text was underlined to make it more appear more clickable. cc @iamthomasbishop
   See

<PanelBody style={ styles.textFooternote }>
<SmallFooterMessageControl
	value={ footerNote }
></SmallFooterMessageControl>
</PanelBody>

could be rewritten to be

{ footerNote && <PanelBody style={ styles.textFooternote }>
  <SmallFooterMessageControl
	  value={ footerNote }
  ></SmallFooterMessageControl>
</PanelBody> }

This way the FooterMessageControl would only show up if there is a footer note argument.

6. It would be great to also add a readme file for the new FooterMessageLink component. :)

[SiobhyB]

Thanks again for your help! I've implemented the following from your list:

2. Done, I've changed SmallFooterMessageLinkControl to FooterMessageLink. ✅
5. Done, thank you for the tip! :D ✅

And am either working on or have some follow up questions on the following:

1. I converted SmallFooterMessageControl to FooterMessageControl and passed the text-align prompt, as you suggested. Adding size="small" doesn't work to change the font size, however. I'll wait to hear from @iamthomasbishop on his thoughts around aligning the styles before digging further into this.
3. I'm still working to replicate this.
4. I'll await @iamthomasbishop's feedback on the design change you suggested, also.
6. I placed the FooterMessageLink component under the bottom-sheet folder as I see that's where a lot of other smaller components for mobile live. There aren't any other READMEs for these smaller components, so am not sure where to place a new one exactly. Should I perhaps create a separate GitHub issue regarding these components with missing READMEs?

[SiobhyB]

Following a chat with @iamthomasbishop, I've gone ahead to complete the following:

• I've changed the style of FooterMessageControl so that its text is aligned to the left and the size is now slightly smaller, at 12px. The styles of the footer message in the Columns block and the Image block's settings now match. :) This should also resolve the issue with padding.
• We're going to keep the style of the link as-is for now, with the option of opening a new PR if we'd like to change this at a later stage.
• I've added a README for the FooterMessageLink component.

Android:

Light mode	Dark mode

iOS:

Light mode	Dark mode

New issues created following the discussions here:

• Low Contrast Between Text Colour and Background in Footer: Mobile: Low Contrast Between Text Colour and Background in Footer Message Cell  #29866.
• Missing READMEs for Some Mobile Components: Mobile: Missing READMEs for Some Mobile Components #30355.

@enejb, could I bother you for another review, please? Thank you in advance. 🙇‍♀️

[enejb]

Nicely done @SiobhyB!

I tested this on the android and ios simulators and it worked well for me. (lite and dark modes)
Thank you for all the different code changes. I think this is ready to ship !

[iamthomasbishop]

@SiobhyB This is looking good, just one tiny thing I noticed in your last round of screenshots. It looks like the text label might be getting clipped by the container boundary:

[SiobhyB]

@iamthomasbishop, thanks for your review! I spent a bit of time looking into this and it appears to be Android-specific, with the bottom parts of some text being visible at the top when multiple lines are added. I couldn't find a quick fix, as testing will be required across different size screens in both iOS and Android, but have created a new GitHub issue for this: #30457

Do you think it'd be okay to merge this PR and then I'll work on that issue separately when I'm back from AFK?

[iamthomasbishop]

Do you think it'd be okay to merge this PR and then I'll work on that issue separately when I'm back from AFK?

@SiobhyB That's fine, thank you!

[hypest]

Just to be sure, since there are bridge interface changes, we have to merge the WPAndroid side at the same time, right? If so, we'll need to get the WPAndroid PR CI green and approved before merging 👍

Oops, I mixed up two different efforts, there are no bridge changes in this one so, will go ahead and merge (and update+merge the gutenberg-mobile PR too). Thanks for the work here @SiobhyB!