Don't send empty paragraph tags to view on Android #22561
Conversation
|
Size Change: 0 B Total Size: 1.17 MB ℹ️ View Unchanged
|
cameronvoell
force-pushed
the
rnmobile/fix-missing-placeholder-quote-blocks
branch
from
353607a
to
af6afdd
Compare
cameronvoell
added
the
Mobile App - i.e. Android or iOS
cameronvoell
force-pushed
the
rnmobile/fix-missing-placeholder-quote-blocks
branch
from
af6afdd
to
c7cb54a
Compare
| } | ||
| return value; | ||
| } | ||
|
|
There was a problem hiding this comment.
Removing empty paragraph tags to fix missing placeholders on Quote/Pullquote blocks seems to work fine except for calls to the imported create function from gutenberg/packages/rich-text/src/create.js. That function seems to only work at extracting text in order to create the record object if we re-add paragraph tags for multiline text.
| @@ -679,8 +698,8 @@ export class RichText extends Component { | |||
| value = ''; | |||
| } | |||
| // On android if content is empty we need to send no content or else the placeholder will not show. | |||
| if ( ! this.isIOS && value === '' ) { | |||
| return value; | |||
| if ( ! this.isIOS && ( value === '' || value === '<p></p>' ) ) { | |||
There was a problem hiding this comment.
In here instead of doing value === <p></p> you should write value === '<'+tagname+'></'+tagname'>'
There was a problem hiding this comment.
I understand the thought here, but there are a few issues. First off to catch this issue for Quote/Pullquote and List Block we would not want to use tagName, but this.multilineTag which seems to be the tag that is staying in value that is preventing us from seeing the placeholder. Even if we do switch this to target the general case of a remaining this.multilineTag, removing this for the List Block seems to create issues that are quite unique from the Quote and Pullquote block (basically the ListBlock no longer acts like a list block), different enough to justify a separate fix for List Block in my opinion.
cameronvoell
force-pushed
the
rnmobile/fix-missing-placeholder-quote-blocks
branch
from
717df2d
to
eace29c
Compare
@SergioEstevao Thanks for the comments and feedback. I addressed your second point by making the fix only touch Android. As far as fixing List Block placeholders go, it seems to me that the strategy of this fix for Quote/Pullquote would require some pretty unique changes to simultaneously fix List Block (see comment here #22561 (comment)). My opinion now is that given how convuluted RichText has become, to get a quick win for fixing Quote/Pullquote placeholders for the upcoming release, my thinking was that the strategy of being as non-intrusive as possible felt like the only way of fixing without refactoring more and more of RichText. I would love to make this fix feel even safer and more targeted for this release, and am open to advice on that front, otherwise if we think it is not worth fixing this single case, without a larger refactor to address some of the underlying causes I understand that perspective as well. @hypest I'd be interested to hear your opinion on this as well. |
|
Had a look on Let's get to the bottom of this before finalizing the fix, to be sure the fix is safe. |
|
Noticed that if text without any newline is added to a pullquote or quote block and then deleted, the placeholder is restored. But adding text with a newline then deleting all the text (including the newline) does not restore the placeholder. If you deselect the block and reselect it, the placeholder reappears, so this seems like a pretty minor issue that doesn't need to be fixed right away. A quick bit of debugging makes me think this is because of something happening inside of Aztec because the html looks fine.
|
|
Ran through the writing flows manual tests, making sure to test Quote and Pullquote whenever relevant
Had some problems with this test:
It looks like this behavior is not perfect on Android on develop and Beta release (cursor seems to move to incorrect position, end -1 ). However on the branch with this fix, the Quote and Pullquote seems to have additional problems on undo and redo. I had one case where I was unable to press redo enough times to recover all the text I deleted by pressing undo. I'm going to see if I can fix this. |
cameronvoell
force-pushed
the
rnmobile/fix-missing-placeholder-quote-blocks
branch
from
e0f3acf
to
47494b5
Compare
|
Tried this from inside WPAndroid and got a red screen: To reproduce:
|
cameronvoell
force-pushed
the
rnmobile/fix-missing-placeholder-quote-blocks
branch
from
0d04f77
to
47494b5
Compare
|
@ceyhun Did not get to test this extensively yet, but f664971 seems to fix the red screen you were seeing. I think I can spend some time making the code itself more clear, but at least this seems like a relatively simple viable strategy since it seems to fix all issues I was seeing previously. Can test WPAndroid using the APK here: wordpress-mobile/WordPress-Android#12663 (comment) |
|
I feel like there's a chance this PR might also address wordpress-mobile/gutenberg-mobile#1524. May be worth checking that issue as well when reviewing this. |
| ! this.isIOS && | ||
| ( value === '' || value === EMPTY_MULTILINE_PARAGRAPH ) | ||
| ) { | ||
| return ''; |
There was a problem hiding this comment.
This ensures that empty paragraph tags do not get passed to the RCTAztecView (thus hiding the placeholder)
| @@ -283,14 +286,29 @@ export class RichText extends Component { | |||
| const contentWithoutRootTag = this.removeRootTagsProduceByAztec( | |||
| unescapeSpaces( event.nativeEvent.text ) | |||
| ); | |||
| let formattedContent = contentWithoutRootTag; | |||
| if ( ! this.isIOS ) { | |||
| formattedContent = this.restoreParagraphTags( | |||
There was a problem hiding this comment.
Text with removed paragraph tags from https://github.com/WordPress/gutenberg/pull/22561/files#r478829512 can not pass from RCTAztecView to the RichText value without coming through this call in the onTextUpdate function.
| @@ -264,7 +267,7 @@ export class RichText extends Component { | |||
| /* | |||
| * Handles any case where the content of the AztecRN instance has changed | |||
| */ | |||
| onChange( event ) { | |||
| onChangeFromAztec( event ) { | |||
There was a problem hiding this comment.
Updated method name to help distinguish from this.props.onChange which is used many times elsewhere in this class.
@mchowning Unfortunately, that issue seems to be different from the quote/pullquote issue, and the fix here does not address the backspace sometimes needed to show the placeholder. My current thoughts are that that issue should be tackled in a separate PR. |
| @@ -59,6 +59,8 @@ const gutenbergFormatNamesToAztec = { | |||
| 'core/strikethrough': 'strikethrough', | |||
| }; | |||
|
|
|||
| const EMPTY_MULTILINE_PARAGRAPH = '<p></p>'; | |||
There was a problem hiding this comment.
Not a big deal, but what is the significance of "MULTILINE" in this name? Is there a non-multiline paragraph we're distinguishing?
There was a problem hiding this comment.
Good point, updated to better name here: 8f9bd43
👍 I agree, that doesn't need to be fixed here. I just thought there was a chance that might get fixed "for free" with your changes. |
|
@hypest Let us know if you can remove the requested change restriction here from Sergio #22561 (review). His concern was about simultaneously fixing List block placeholder on Android which as I explained here: #22561 (comment) it feels like the List block issue should be addressed separately. |
hypest
dismissed
SergioEstevao’s stale review
Dismissing since Sérgio is on sabbatical at the moment and the blocking bit (including the List block handling as well) can be addressed seperately.
Fixes wordpress-mobile/gutenberg-mobile#1970
Gutenberg-Mobile PR: wordpress-mobile/gutenberg-mobile#2307
Description
This change is for fixing missing placeholder text on Quote and Pullquote blocks on Android. The updates here detect in
RichTexton Android when we are sending empty paragraph tags toRCTAztecViewand send an empty string instead.We then add a check to onTextUpdate to ensure that empty paragraph text does not get passed back to the RichText Value without having its paragraph tags added back.
How has this been tested?
*Note that this fix does not resolve the extra backspace issue seen in other blocks (wordpress-mobile/gutenberg-mobile#1524). The simple case of typing a quote and then pressing backspace seems to reshow the placeholder correctly, but if you press enter to make a multiline quote, and then backspace til empty, the placeholder will not show similar to how we need an extra backspace in other blocks.
Screenshots
Checklist: