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

[Mobile]: Add Image Caption Styling #14883

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Apr 9, 2019

Associated PR updating the gutenberg ref in gutenberg-mobile.

Description

Issue #574 notes that there are no controls for customizing the style or linking of an image caption. This PR adds that feature by converting the caption from a TextInput component to a RichText component. Using a RichText component enables the use of bold, italic, links, and strikethrough. In addition, this PR also addresses the following bugs:

  1. Captions that are longer than a single line will no longer be cut off (Define API method for handling legacy content data #590). Instead, they now wrap to another line.
  2. A number of issues where the the toolbar would not properly update when selecting/unselecting an image caption. See the Testing Scenarios below.

Center Alignment Regression

With this PR, the caption text is no longer aligned to the center because unlike TextInput, RichText does not respect the text-align: center style. I believe part of the solution to that may be adding a textAlign prop to the ReactAztecManager, but I need to do more investigation. My plan is to discuss my findings seperately from this PR because, even without the center alignment, I think that this PR substantially improves the user experience. Just let me know if you think we should hold off on this PR until the center alignment issue is resolved.

Before After
output_before output

How has this been tested?

Tested these changes on a Pixel 3 Android device in both the gutenberg-mobile demo app and in a WordPress-Android WasabiDebug build. I cannot build for iOS because my personal machine is linux, so I have NOT tested these changes on iOS. ⚠️

Testing Scenarios

  1. An unfocused image without a caption does not show the caption or any placeholder text.
  2. An unfocused image with a caption does display the caption.
  3. A focused image without a caption shows placeholder text.
  4. Tapping an unfocused caption focuses the caption with RichText styling buttons (bold, italic, link, strikethrough) in the toolbar with the appropriate active/inactive states and without the edit image (pencil) button in the toolbar.
  5. (1) Focus on an image caption, and (2) tapping on another block removes the caption styling buttons from the toolbar and replaces them with the now-focused block's buttons.
  6. (1) Focus on an image caption, and (2) tapping on the image for that caption removes the caption styling buttons from the toolbar and replaces them with the edit image toolbar button.
  7. (1) Open a caption, (2) edit the caption (so caption is focused and contains text), (3) tap on a different block, (4) tap on the image with the just-edited caption (but do not tap on the caption), and the caption will not be active (i.e., no cursor and none of the caption's styling buttons will are added to the toolbar).
  8. (1) Edit a caption so that it is non-empty, (2) tap on another block that uses RichText (paragraphs or headings are good candidates), (3) tap on the caption you edited, and there should only be the toolbar buttons for the caption (no duplicate buttons).
  9. (1) Edit a caption so that it is not empty, (2) tap on an "Unsupported" block, (3) tap on the caption you edited, and the toolbar buttons for the caption should be visible.
  10. Enter a caption that is longer than a single line should wrap the text to a new line instead of cutting off the text.
  11. (1) Create an image caption on the web with styles, (2) edit that post on mobile, and the caption should now be displayed with the appropriate styles instead of html markup.

-- Additional Test Scenarios based on PR review --

  1. (1) While editing a caption, (2) tap enter, and the cursor should be moved to a new line within the caption block.
  2. (1) Tap on an image without a caption, (2) tap on the empty caption, (3) enter a multiline caption (using the enter key, not just wrapping), (4) press the undo button, and the caption should return to an empty state.
  3. (1) Tap on an image caption, (2) add a link, (3) link UI bottom sheet is displayed and can be updated.

I realize there are quite a few testing scenarios here, but I did manage to break the functionality in most of these scenarios at some point while I was developing this feature (or that functionality was broken in the previous implementation), so I wanted to document them.

Checklist:

  • ⚠️ My code is tested: ⚠️
    • My Code is tested on iOS (not completed because my personal computer runs linux).
    • My code is tested on Android.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@mchowning mchowning changed the title Issue/image caption styling Mobile: Add Image Caption Styling Apr 9, 2019
@mchowning mchowning changed the title Mobile: Add Image Caption Styling [Mobile]: Add Image Caption Styling Apr 9, 2019
@etoledom etoledom self-requested a review April 9, 2019 17:49
@etoledom etoledom added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Apr 9, 2019
Adds ability to style image caption with bold, italic, links, and strikethrough (wordpress-mobile/gutenberg-mobile#574).
Adds ability for image captions to wrap to multiple lines instead of getting
cut off (wordpress-mobile/gutenberg-mobile#590).
The previous approach of setting the child component's `isSelected` prop
at the time the child was rendered based on
`this.props.isSelected && this.state.isCaptionSelected` did not work when:

  (a) the child was selected (so its `isSelected` prop was true);
  (b) the child component had no text; and
  (c) the parent's `isSelected` prop was changed to false
      (i.e., another block was selected).

Because the child component is not rendered when both the parent's `isSelected`
prop is false and the caption does not contain any text, the child
component's `onBlur` prop function (the `onCaptionBlur function) would not
be called and update the `isCaptionSelected` state to be false.

This bug shows when a user selects an empty caption, then selects a different
block (thereby hiding the caption since it is empty), and then re-selects the
image with the empty caption. In that scenario, upon re-selecting the image
with the empty caption, the empty caption would appear and immediately be
selected instead of just appearing. This bug would not appear if there
was any text in the caption, because then the caption would always
render and its `onBlur` prop function would be called.
componentWillReceiveProps( nextProps ) {
this.setState( ( state ) => ( {
isCaptionSelected: nextProps.isSelected && state.isCaptionSelected,
} ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guarding the isCaptionSelected state here with the image block's isSelected prop in order to avoid a noticeable flicker in the toolbar buttons when changing to another block with buttons. There seems to be a lag between the image block's isSelected prop being updated to false and its isCaptionSelected state being updated to false by the onCaptionBlur function (which is passed as the caption's onBlur prop). That lag created a split second where the toolbar buttons for both the caption and the newly-selected block were shown at the same time.

My original approach was to just set the caption's isSelected prop in the image block's render function as this.props.isSelected && this.state.isCaptionSelected. But this did not work when:

(1) the caption was selected (so the image's isCaptionSelected state was true);
(2) the caption had no text; and
(3) another block was selected, causing the image's isSelected prop to change to false.

When the new block is selected in (3), the caption's onBlur prop function (the onCaptionBlur function) would not be called and update the image block's isCaptionSelected state to be false becasuse an empty caption is not even rendered once the image block's isSelected prop is false:

{ ( ! RichText.isEmpty( caption ) > 0 || isSelected ) && (
<View style={ { padding: 12, flex: 1 } }>
<RichText

Because the isCaptionSelected state is never updated to false, if the user re-selects the image with the empty caption, the empty caption would appear and immediately be selected even though the user did not tap on the caption (the user couldn't have directly tapped on the caption because empty captions are not shown until the related image is selected). If the caption was not empty, then this bug does not appear because the caption would always render, allowing its onBlur prop function to update the isCaptionSelected state to false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!
Maybe for this one it would be worth to add a short comment in code?
So someone else (or future us) that sees it will understand the intention easily.

Copy link
Contributor Author

@mchowning mchowning Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I included this info in the commit body, but it makes sense to add a brief explanation here in the code.

I pushed an update addressing this.

@@ -108,6 +117,8 @@ class ImageEdit extends React.Component {
} else if ( attributes.id && ! isURL( attributes.url ) ) {
requestImageFailedRetryDialog( attributes.id );
}

this._caption.blur();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this so that when an image's caption is selected and the user taps on the image, the caption will lose focus and the toolbar will be updated to only contain the edit image button.

onFocusCaption() {
if ( this.props.onFocus ) {
this.props.onFocus();
}
Copy link
Contributor Author

@mchowning mchowning Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This this.props.onFocus() call avoids two issues:

  1. Going from another RichText component to an image caption would result in the toolbar having the buttons for both blocks (testing scenario 8); and
  2. None of the styling toolbar buttons would appear when switching from an unsupported block to a non-empty image caption (testing scenario 9).

Adding this same logic to the onBlurCaption to call this.props.onBlur did not appear to be strictly necessary because this.props.onBlur was always undefined in my testing, but I thought it was better to add it so that onBlurCaption and onFocusCaption would have consistent behavior.

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mchowning for this PR!
Code wise it looks shinny. And thanks for the explanations of your decisions! 🎉

Android:

  • All testing scenarios are working perfectly! Nicely done :)
  • I did find a small bug pressing the Enter key on the caption field:
    The gif is on the example app but I also was able to reproduce it on WPiAndroid.

Enter_on_caption_field

iOS:

  • All testing scenarios are working perfectly too!
  • Pressing Enter also has problems, but this one seems to be on the iOS Native side. There are few chances that fixing the Android crash will also fix the iOS crash.

Enter_caption_ios

  • There is one more issue on iOS, and this one is the one that is worrying me:
    • When I tap the links button to show the Links UI, it will try to show it but it will automatically dismiss it right away.

caption_links_ios

I did some debugging to figure out if it was an iOS Native issue, since it seems to affect only to iOS. This is what I found out:

The Link UI (BottomSheet) is added to the hierarchy by the FormatEdit component on RichText. And it's removed from the hierarchy if RichText is not selected. So far so good.

When the TextInput on the BottomSheet is focused (it is auto focus on iOS), the caption has to loose its focus, that triggers a blur call. Specifically this one:

https://github.com/mchowning/gutenberg-mobile/blob/bc76bc2f6d700a834a45b80d07f1b2ef107a4f00/react-native-aztec/src/AztecView.js#L96

That blur is propagated all the way until the Image block, and that triggers our new onBlurCaption(), setting isCaptionSelected to false, removing FormatEdit from the hierarchy and effectively dismissing the BottomSheet.

Why this doesn't happen on Android?

On Android I noticed that the blur event from AztecView is not triggered when the TextInput on the BottomSheet is focused, so the ImageBlock still thinks that the caption is focused.

After figuring this out, it feels to me that the bug is on Android side, and this implementation works thanks to that bug 😱

I'm not sure if it's expected|required that AztecView._blur is not called on Android when Aztec looses focus by focusing another TextInput. Maybe @daniloercoli can give us some lights?

What I do know is that on iOS this will always happen, so we will need to find a way to sort this out anyway. It doesn't seem to be simple though 🤔

I wasn't expecting this complication for this task, but that's the beauty of RN and its "multiplatform" 😁

@etoledom
Copy link
Contributor

etoledom commented Apr 10, 2019

Center Alignment Regression

About this, IMHO the benefits of having the caption handling formatted text outweighs loosing the center alignment. It might look like a bug at first sight though.

What do you think @koke, @hypest ?

@koke
Copy link
Contributor

koke commented Apr 10, 2019

I think it's OK to deal with alignment on a separate PR, but I'd prefer if we get that one fixed before the next release

@daniloercoli
Copy link
Contributor

After figuring this out, it feels to me that the bug is on Android side, and this implementation works thanks to that bug 😱

I'm not sure if it's expected|required that AztecView._blur is not called on Android when Aztec looses focus by focusing another TextInput. Maybe @daniloercoli can give us some lights?

I've checked the Android native side, and we're not doing anything special there. There is just a listener that sends an event to the JS side when a blur happens on the native side.

I've also checked on gb-mobile with the demo content, and the blur and focus methods are both correctly called when you move the caret to another RichText powered field. Instead it's not called when you set the focus on the TextInput field in the Links BottomSheet.
Maybe it's a platform difference thing.

@etoledom
Copy link
Contributor

etoledom commented Apr 10, 2019

Maybe it's a platform difference thing.

I agree! I did some more tests, and this difference also happens on the RN TextInput component.
Thank you @daniloercoli for checking. 👍

@mchowning
Copy link
Contributor Author

I did find a small bug pressing the Enter key on the caption field

Good catch @etoledom! I had a bit of time to look into this tonight, and it appears that it is caused by the AztecParser losing the newline character (or <br> tag depending on how you look at it) when it updates the view after the Enter key press. The view then tries to set the cursor at the index after the no-longer-present newline character, resulting in an out-of-bounds-by-one error.

In particular, in image/editor.native.js I am currently giving the caption a <p> tag (via tagName={ "p" }, but AztecParser's tidy function removes <br> tags within a paragraph tag. For example, <p>a<br></p> becomes <p>a</p>. Here are two tests I wrote demonstrating how the <br> tag gets removed during the parsing round trip with <p> tags, but not div tags.

In light of that, it's not surprising that switching the caption's tagName from "p" to "div" is one way to avoid this Enter key bug. I'm a bit hesitant about that though because the <p> seems more semantically descriptive than a div tag for an image caption. I also still need to do some follow-up to see:

  1. Why we're removing the <br> inside <p> tags in the first place. The reason may point to a better solution to this Enter key bug.
  2. What are the effects of switching the tagName from "p" to "div". I've only really had time to confirm that change avoids this Enter key bug. I haven't had a chance to see what other effects that change might have.

Obviously I have more investigation to do, but I wanted to put this out there in case anyone has any thoughts on what I've found so far.

@etoledom
Copy link
Contributor

etoledom commented Apr 11, 2019

@mchowning - Thanks for clearing up where the problem is!
Related to this: wordpress-mobile/gutenberg-mobile#426

It would be good to clear up what is the behavior we want in place before continuing fixing this.

It also looks like on web they use the <br> tag for new lines on captions:

<!-- wp:image {"id":798} -->
<figure class="wp-block-image">
    <img src="https://etoledomatomicsite01.blog/wp-content/uploads/2019/02/image_571855593746995-768x1024.jpg" alt="" class="wp-image-798"/>
    <figcaption>Hello<br>world<br>multiline</figcaption>
</figure>
<!-- /wp:image -->

@mchowning
Copy link
Contributor Author

mchowning commented Apr 14, 2019

Maybe it's a platform difference thing.

I agree! I did some more tests, and this difference also happens on the RN TextInput component.

I'm late to the party 😄, but it definitely looks like a platform difference. Even a new React Native app using either react-native-modal (like we do) or React Native's built-in Modal shows the same behavior difference (iOS calls onBlur on focused TextInput components that are behind the modal, Android does not).

It may be worth keeping an eye on how the work to port react-native-modal to be a 100% js component proceeds since that might avoid most platform-specific differences.

This change brings mobile's handling of image captinos in line
with the web. It also addresses a crash that was occurring when
the enter key was tapped to enter a new line in an image caption.
@mchowning
Copy link
Contributor Author

mchowning commented Apr 15, 2019

Added a commit addressing the crash when tapping enter to add a new line by updating the tagName for the caption's RichText component from p to figcaption. As @etoledom noted, figcaption is used by the web and, as with div, our parser does not remove br tags that are inside figcaption tags.

⚠️ Again, I have only tested this on Android because I am working on a linux machine.

One thing to note is that I am not setting the multiline prop on the caption's RichText component. I was a little surprised to see that setting multiline to be true breaks the caption's consistency with the web. Without the multiline flag we get br tags like the web does, but with that flag we get a paragraph tags for each line. As best I can tell, setting multline={ 'br' } has no actual effect and just falls back to the default behavior (it only matters if multiline is true, same as if multiline were false or undefined). Although RichText does pass the multliine prop to the native side, I'm pretty sure it's not used on Android, and I don't see it being referenced on iOS. I still don't feel like I have a complete grasp on how multiline is used here, so let me know if you think we should be using it here.

I have noticed that there are some crashes when pressing enter with lines that have extra spaces:

  1. (1) Begin editing empty caption, (2) add a space, (3) add any letter, (4) press Enter, and 💥
  2. (1) Begin editing empty caption, (2) add any letter, (3) add a space, (4) press Enter, and 💥
  3. (1) Begin editing empty caption, (2) add any letter, (3) add a period, (4) add two spaces, (5) add any letter, (6) press Enter, and 💥

I do not think these issues are directly tied to my changes because a list block has similar issues. I also saw similar crashes when an undo action was executed that removed multiple lines in a caption or a list, but recent gutenberg updates appear to have resolved those.

At a high level, these crashes appear to be occurring because we are sending a text prop to the native code that includes both (a) text that ends up being trimmed in some way when we parse the html and (b) start and end selection indices that do not account for that trimming. A fix along the lines of @mzorz 's PR adding a guard against invalid selection indices on the native side would avoid these crashes, although the underlying issue leading to the index-out-of-bounds error would remain.

@hypest
Copy link
Contributor

hypest commented Apr 16, 2019

FWIW, tested out @mchowning 's image_caption_styling/remove_onBlurCaption branch at commit mchowning/gutenberg-mobile@ed80fd1 (Gutenberg branch and commit) and the links UI bottomsheet does stay on when trying to add a link inside the image caption area 🎉.

On iOS the display of the link UI modal was causing the `onBlurCaption`
function to be called, which would update the image caption's `isSelected`
prop to false. That would, in turn, immediately remove the
just-displayed modal. Restructuring the logic to not like this to not
use the image caption's `onBlur` function avoids that issue.
@mchowning
Copy link
Contributor Author

mchowning commented Apr 16, 2019

There is one more issue on iOS, and this one is the one that is worrying me:
When I tap the links button to show the Links UI, it will try to show it but it will automatically dismiss it right away.

Added 4c4801a to address this issue (thanks for testing this out @hypest). As already discussed, this occurred because showing that modal causes a call to the previously active text field's onBlur function (only on iOS). This change avoids that issue by adjusting the logic so that we no longer use the caption's onBlur function to update it's isSelected state.

Issues that must be addressed before merge:

  1. Tests are failing. I need to look into this because it's not obvious why. Took a few days off for vacation, merged the latest from master, and now the tests are passing again. One or two more vacations and this PR will be ready to merge. 😆
  2. Tapping enter key on iOS. My fix for allowing new lines to be entered has only been verified on Android.

Other issues

  1. UPDATE: This has been resolved. Tapping the enter key when there is space at the beginning or end of a caption causes a crash. As I mentioned earlier, I believe this is a more general issue that can also be seen with lists. With that said, this crash is easier to produce on captions than lists because lists require multiple consecutive spaces to create the crash, whereas here we only need a single space at the beginning or end of a line to create a crash when enter is tapped. That might mean we need to get that issue fixed before we merge this (so this actually belongs in the "must be addressed before merge list")?
  2. Loss of center alignment for captions. As already discussed, this needs to be addressed, but that can be done outside this PR.

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mchowning - Great job working on the issues! 🎉

The LinksUI problem on iOS is completely solved ✅
The Red Screen on enter is solved (Android) ✅
The app crash on enter is solved (iOS) ✅

Thanks also for creating the tickets for the issues found beyond this PR 🙏


I found one particular case where the multiline caption behaves unexpectedly on Android:
These are the steps:

  • Insert an Image on an Image block.
  • Write a one line caption.
  • Press Enter.
  • Write another line.
  • Press Enter.
  • Press <- (Arrow back button) to exit the editor saving a Draft.
  • Open the recently saved Draft.
  • You will see a "Block Validation" error:

Screen Shot 2019-04-17 at 10 22 34 AM

What I see when switching to HTML mode is this:

<!-- wp:image {"id":790} -->
<figure class="wp-block-image">
    <img src="https://etoledomatomicsite01.blog/wp-content/uploads/2019/02/img_1041.jpg" alt="" class="wp-image-790"/>
    <figcaption>
        Thanks for<br>I will be<br>Thanks</figcaption><br>
    </figcaption>
</figure>
<!-- /wp:image -->

If you notice, there is an extra </figcaption> by the end of the text content.

On iOS:
On iOS there is also a small issue with multiline, I think that the cause is the same even though it presents itself differently:

<!-- wp:image -->
<figure class="wp-block-image">
    <img src="https://cldup.com/cXyG__fTLN.jpg" alt=""/>
    <figcaption>
        <p>First line</p>Second line<br><br>
    </figcaption>
</figure>
<!-- /wp:image -->

As you see, the first line gets wrapped in p tags. (And it's always just the first line)

Let's focus on the Android side. I'll be helping you all I can with the iOS side. 😃👍

@mchowning
Copy link
Contributor Author

mchowning commented Apr 22, 2019

The crash I was seeing in this PR if a new line was added to a caption after a line with "extra" spaces has been resolved by @hypest 's PR. We now avoid updating the rich-text caret position when the content will be trimmed on the native side. 🎉

Explicitly setting the tagName to figcaption caused an
invalid block to be saved if a caption ended with an empty line.
@mchowning
Copy link
Contributor Author

@etoledom : Updated to no longer explicitly set figcaption as the tagName for the caption. In my testing, this resolves the invalid block format issue on Android and 🤞 will also resolve it on iOS. Would you mind checking iOS for me?

No longer setting the tagName prop for the caption did require a change in RichText to guard against an undefined tagName. Without that change, the RichText component's render method would create some lovely <undefined>my caption</undefined> html. Let me know if you see anything about that change is concerning.

@etoledom
Copy link
Contributor

Hey @mchowning ! Great job fixing the figcaption issue 🎉

Tested on Android and it works great, adding <br> for each new line.

No longer setting the tagName prop for the caption did require a change in RichText.

Looks good to me. I didn't see regressions and works as expected.

and 🤞 will also resolve it on iOS.

The <p> issue on iOS is still around, it behaves a bit different than before. At this point I'm inclined to think that the solution for iOS will need changes on the native side. I'll investigate this further, and ask for your assistance if there are changes required for Android (hopefully they won't).

Interestingly I have made it work properly on iOS by setting RichText with multiline={ false } and tagName="p", but that's a bit counter intuitive and it breaks Android ¯\_(ツ)_/¯. IMHO your proposal looks more correct 👍


To move forward, I have created a feature branch rnmobile/feature-caption on this repo. Let's target that branch and merge. I also have created feature/caption on gutenberg-mobile.

Then we can start working on the alignment on a new PR. Does it sound good @mchowning ?

@mchowning mchowning changed the base branch from master to rnmobile/feature-caption April 26, 2019 23:59
@mchowning
Copy link
Contributor Author

mchowning commented Apr 27, 2019

Thanks @etoledom !

To move forward, I have created a feature branch rnmobile/feature-caption on this repo. Let's target that branch and merge.

I think having a feature branch for this work is a really good idea. I have updated the base branch to rnmobile/feature-caption, and I believe we are ready to merge.

I also have created feature/caption on gutenberg-mobile.

I have updated the target branch for that PR as well.

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @mchowning !

As we discussed, let's merge and continue in a new PR 👍

@etoledom etoledom merged commit d9390f3 into WordPress:rnmobile/feature-caption Apr 27, 2019
@mchowning mchowning deleted the issue/image_caption_styling branch April 27, 2019 10:10
etoledom added a commit that referenced this pull request May 16, 2019
* [Mobile]: Add Image Caption Styling (#14883)

* Add ability to style image caption

Adds ability to style image caption with bold, italic, links, and strikethrough (wordpress-mobile/gutenberg-mobile#574).
Adds ability for image captions to wrap to multiple lines instead of getting
cut off (wordpress-mobile/gutenberg-mobile#590).

* Address style errors from linter

* Hide image toolbar controls if caption selected

* Update state in componentDidMount

The previous approach of setting the child component's `isSelected` prop
at the time the child was rendered based on
`this.props.isSelected && this.state.isCaptionSelected` did not work when:

  (a) the child was selected (so its `isSelected` prop was true);
  (b) the child component had no text; and
  (c) the parent's `isSelected` prop was changed to false
      (i.e., another block was selected).

Because the child component is not rendered when both the parent's `isSelected`
prop is false and the caption does not contain any text, the child
component's `onBlur` prop function (the `onCaptionBlur function) would not
be called and update the `isCaptionSelected` state to be false.

This bug shows when a user selects an empty caption, then selects a different
block (thereby hiding the caption since it is empty), and then re-selects the
image with the empty caption. In that scenario, upon re-selecting the image
with the empty caption, the empty caption would appear and immediately be
selected instead of just appearing. This bug would not appear if there
was any text in the caption, because then the caption would always
render and its `onBlur` prop function would be called.

* Add comment explaining isSelected guard

* Update image caption tagname from p to figcaption

This change brings mobile's handling of image captinos in line
with the web. It also addresses a crash that was occurring when
the enter key was tapped to enter a new line in an image caption.

* Remove `onBlurCaption` function

On iOS the display of the link UI modal was causing the `onBlurCaption`
function to be called, which would update the image caption's `isSelected`
prop to false. That would, in turn, immediately remove the
just-displayed modal. Restructuring the logic to not like this to not
use the image caption's `onBlur` function avoids that issue.

* Remove tagName from caption RichText component

Explicitly setting the tagName to figcaption caused an
invalid block to be saved if a caption ended with an empty line.

* iOS: Remove p tags from image caption paragraphs (#15366)

* Add center alignment to image caption on mobile (#15257)

* Adding missing style to video/style.native.scss
This style was fetched from the Image block, and it was removed after replacing the TextInput with RichText on captions
@mchowning mchowning mentioned this pull request Nov 29, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants