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

[RNMobile] Prevent always dismissing keyboard when switching away from empty image caption #16759

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Jul 25, 2019

Related gutenberg-mobile PR

Description

This is a bugfix addresses an issue on Android where switching from an empty caption to another text block would cause the keyboard to be dismissed. The issue appears to occur as a result of the caption not being rendered when it is empty, so it was possible to fix the issue by always "rendering" the caption, but setting it's style's display property to "none" when it should not be visible.

Before After
before_keyboard-dismiss mp4 after_keyboard-dismiss mp4

This issue did not occur on iOS and it appears it may be a platform difference (as opposed to an issue with our RichText component) because I also observed this same issue when I replaced our RichText caption with a TextInput component.

NOTE: This PR only fixes this issue for image captions. The issue remains for video captions. I am exploring a few different ways of resolving the issue for video captions, and I would like to present that fix in a separate PR.

Test steps

  1. Open demo app
  2. Select image block without a caption
  3. Tap on empty caption ("Write caption...")
  4. Observe soft keyboard is displayed
  5. With the empty image caption, select any other block where you can edit text (paragraph, heading, etc.)
  6. Observe that the soft keyboard remains open

I also checked and verified that this change did not seem to mess up TalkBack or VoiceOver support.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@mchowning mchowning changed the title [RNMobile] Prevent always dismissing keyboard when switching away from empty caption [RNMobile] Prevent always dismissing keyboard when switching away from empty image caption Jul 25, 2019
@mchowning mchowning self-assigned this Jul 25, 2019
@mchowning mchowning requested review from daniloercoli and Tug July 25, 2019 20:41
@mchowning mchowning 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 Jul 25, 2019
@@ -270,6 +270,12 @@ class ImageEdit extends React.Component {
);
}

// We still want to render the caption so that the soft keyboard is not forced to close on Android
Copy link
Contributor

@hypest hypest Jul 26, 2019

Choose a reason for hiding this comment

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

Nit: even though the comment makes sense in the context of this PR (the change from not rendering at all to rendering with display: none) it's not so clear when read on its own in the final file version.

What do you think about this instead:

We'll render the caption so that the soft keyboard is not forced to close on Android but still hide it by setting its display style to none. See wordpress-mobile/gutenberg-mobile#1221

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Tried this and works fine!

To be honest though, I think I would consider this a workaround instead of a final fix. I mean, it's still not clear for example why the paragraph that receives the caret is not causing the virtual keyboard to open. To me that feels like there's something more going wrong here.

That said, let's merge this one since it does make the writing flow better but, can you please open a ticket to track our concern?

In the meantime, I only left a comment about adjusting a comment so, feel free to merge after revising it.

@mchowning
Copy link
Contributor Author

mchowning commented Jul 26, 2019

To be honest though, I think I would consider this a workaround instead of a final fix.

So true. 😞 When I initially drafted this PR I called it a hack, but "workaround" is such a nicer way to say it.

let's merge this one since it does make the writing flow better but, can you please open a ticket to track our concern?

That was my thinking in opening this. After finding this "fix", I spent more than a day trying to find the real cause, and I eventually felt like I should just get the fix in since it was small and would be easy to revert later if needed. I'll actually take a second look at this today though to see if taking a day off from the issue has given me the distance to see the issue with new eyes.

Thanks @hypest !

@mchowning mchowning merged commit 57b2c2c into master Jul 30, 2019
@mchowning mchowning deleted the rnmobile/prevent_keyboard_from_always_dismissing_when_closing_caption branch July 30, 2019 14:16
@mchowning
Copy link
Contributor Author

let's merge this one since it does make the writing flow better but, can you please open a ticket to track our concern?

Issue opened: wordpress-mobile/gutenberg-mobile#1252

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.

2 participants