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

Fix android rich text errors #949

Merged
merged 25 commits into from
May 15, 2019
Merged

Fix android rich text errors #949

merged 25 commits into from
May 15, 2019

Conversation

Tug
Copy link
Contributor

@Tug Tug commented May 2, 2019

Gutenberg PR: WordPress/gutenberg#15392

This fixes RichText errors on merge and split.
Most of the work is in the GB PR.

Important fix in this PR:

  • We remove calls to clearSelectedBlock on focus that would reset the block selection and thus cause error in the MERGE_BLOCKS effect that need it initialized

Other fixes:

  • Refactor mergeBlocks in BlockHolder to be closer to the web version
  • Mock focusing RN elements for tests

Testing Instructions (on both Android and iOS)

  • Try splitting a header block
  • Try splitting a paragraph block
  • Try merging a paragraph block with a header block
  • Try merging 2 paragraph blocks
  • Try editing text into a paragraph or a header block
  • Try converting a selection into bold and/or italic
  • Try writing in bold
  • Try writing in italic
  • Try writing in both bold and italic
  • Try removing a format without selecting any text by having the cursor on formatted text and pressing on the selected format button
  • Try adding a link
  • Try converting a word to a link
  • Try copying a link and pasting it into a selection
  • Try copy pasting text

Tug added 4 commits May 2, 2019 13:12
commit 72f934e
Merge: c935c81 d46cebd
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu May 2 10:32:48 2019 +0300

    Merge branch 'develop' into fix/list-handling-in-android

commit c935c81
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu May 2 09:48:36 2019 +0300

    Don't use a lambda, to avoid bringing in Java8 features

commit d46cebd
Merge: 42d8443 09f1ca7
Author: etoledom <etoledom@icloud.com>
Date:   Wed May 1 09:37:06 2019 +0200

    Merge pull request #890 from wordpress-mobile/issue/autoscroll-on-list-block

    [iOS] Activate autoscroll on ListBlock

commit 04610cc
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Wed May 1 00:56:33 2019 +0300

    Need to include the kotlin plugin dep location

commit 52832ce
Merge: 897d272 42d8443
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Wed May 1 00:26:57 2019 +0300

    Merge branch 'develop' into fix/list-handling-in-android

commit 897d272
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Wed May 1 00:11:36 2019 +0300

    Set Aztec t delete the Enter for paragraph block

commit ca2a93f
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Tue Apr 30 20:24:28 2019 +0300

    Update GB from its master

commit 0417343
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Tue Apr 30 19:58:53 2019 +0300

    Utilize isEnterPressedUnderway

commit 991f8a8
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Tue Apr 30 18:10:09 2019 +0300

    Use Aztec helper isEnterPressedUnderway

commit 09f1ca7
Merge: 61b52cb 42d8443
Author: etoledom <etoledom@icloud.com>
Date:   Tue Apr 30 13:26:04 2019 +0200

    Merge branch 'develop' into issue/autoscroll-on-list-block

commit 61b52cb
Author: etoledom <etoledom@icloud.com>
Date:   Tue Apr 30 13:25:10 2019 +0200

    Update gutenberg ref to master

commit 57c8de0
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Fri Apr 26 15:13:02 2019 +0300

    Use a git tag to try please JitPack

commit ab4f3e3
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Fri Apr 26 15:07:33 2019 +0300

    Use shortened hash version to try please JitPack

commit ce92f47
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Fri Apr 26 14:43:16 2019 +0300

    Update Aztec ref

commit 7897f98
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 16:07:12 2019 +0300

    Update Aztec ref

commit 76678c9
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 15:13:06 2019 +0300

    Minimize diff

commit cd97945
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 13:03:12 2019 +0300

    Update GB hash

commit c3f094a
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 12:44:16 2019 +0300

    Update GB hash

commit 7391049
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 12:02:19 2019 +0300

    Update GB hash

commit 70112bf
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 04:11:43 2019 +0300

    Update GB hash

commit 91c222e
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 03:21:56 2019 +0300

    Update GB hash to use iOS regression fixes

commit 1786fab
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 03:20:19 2019 +0300

    Use simplified access to EnterPressedUnderway

commit 166b206
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 03:15:37 2019 +0300

    Remove temporary code

commit d17e5d2
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 02:05:01 2019 +0300

    Use Aztec ref with the Enter processing fix

commit bbda142
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 02:00:41 2019 +0300

    Use the GB hash with the rich-text fix

commit 8889b8d
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Thu Apr 25 01:58:58 2019 +0300

    Don't emit text changed event when Enter detected

    The Enter event (ReactAztecEnterEvent) will have the text that the RN
    side needs to process through the format-lib and hopefully match Aztec's
    processing.

commit 38cdc59
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Tue Apr 23 11:54:54 2019 +0300

    Use passed selection when Enter already altered text

commit 2372231
Author: Stefanos Togkoulidis <stefanostogoulidis@gmail.com>
Date:   Tue Apr 23 10:11:51 2019 +0300

    Signal if text has already been changed

commit 1a4f344
Author: etoledom <etoledom@icloud.com>
Date:   Fri Apr 19 18:14:16 2019 +0200

    Update gutenberg ref

commit abc09d1
Merge: c6ca773 0be0fa2
Author: etoledom <etoledom@icloud.com>
Date:   Fri Apr 19 17:49:27 2019 +0200

    Merge branch 'develop' into issue/autoscroll-on-list-block

commit c6ca773
Author: etoledom <etoledom@icloud.com>
Date:   Fri Apr 19 17:49:17 2019 +0200

    Update gutenberg ref

commit a521284
Author: etoledom <etoledom@icloud.com>
Date:   Thu Apr 18 17:01:53 2019 +0200

    Update gutenberg ref
@Tug Tug requested a review from etoledom May 2, 2019 12:05
@Tug Tug self-assigned this May 2, 2019
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@hypest
Copy link
Contributor

hypest commented May 9, 2019

Tried out 95f7d9a and there's an event loop happening (apparently). It happens in various cases when typing but managed to capture one when typing and deleting some text in a paragraph:

Gutenberg-loop

I think the "key" detail in order to replicate is to have some typing/deleting happening fast enough and then the issue arises.

@etoledom
Copy link
Contributor

etoledom commented May 9, 2019

EDIT: Fixed by WordPress/gutenberg@cc12da7 and WordPress/gutenberg@1421cdd

Testing on iOS I found a strange behavior with the list block:

  • The list block doesn't get selected.
  • Pressing "Enter" in the list block, duplicates the content of the block.

list_block

I was able to reproduce the same behavior on Android.

@daniloercoli
Copy link
Contributor

Tests run by using Google Board on my Nexus 5X running Android 7.1

Test 1
I was able to replicate a loop easily, just split a para block and start writing into it.

demo-buy

Test 2
I was able to replicate a "deleting loop" by tapping on a para block, start deleting content by tapping backspace and keep it pressed, then release the backspace key.

demo-buy2

@hypest
Copy link
Contributor

hypest commented May 9, 2019

Steps to repro a merge problem:

  1. Start with this initial html (empty content will do too):
<!-- wp:paragraph -->
<p>split, add a char in new para, backspace it and backspace again to merge</p>
<!-- /wp:paragraph -->
  1. Put caret inside the split word and hit Enter to split the block
  2. Type one char (e.g. g) at the start of the new block
  3. Backspace to delete it
  4. Backspace again to merge the blocks
  5. Notice the g being there in the middle of the merged word/block

@hypest
Copy link
Contributor

hypest commented May 9, 2019

We've been working on some fixes and @etoledom and I have some commits to share. Will merge to this branch now.

@daniloercoli
Copy link
Contributor

Undo doesn't work as expected.

Steps to repro:

  • Start the demo app
  • Put the caret in a para block, and tap Enter to split the block in two
  • Tap Undo ---> Boom!
  • The "original" block is back with the full content, but the block created after the splitting is not removed
    output-scaled

@etoledom
Copy link
Contributor

etoledom commented May 9, 2019

EDIT: Fixed by WordPress/gutenberg@86894ea

Found an issue on split (iOS and Android):

Steps to reproduce:

  • Select a paragraph block in the middle of the content.
  • Split the block.
  • Merge back the new block.
  • (Android) Split again.
    • (iOS) Move the cursor and then Split again.
  • You will see that all the content stays in the first block.

phantom_split

@daniloercoli
Copy link
Contributor

I noticed a minor(?) problem when trying to remove a block by tapping Backspace, and erasing all the content from it.
To reproduce the problem you can follow the steps below for simplicity, but I guess there are easier ways to reproduce it:

  • Start the demo app
  • On a para block tap enter and create a new block
  • Paste something in the block
  • Place the caret at the end of the block
  • Backspace to delete all the content
  • You will see that once the caret reached the beginning of the block, and the placeholder "Start Writing" is on the screen, you need to tap the Backspace 2 times to get the block deleted.

output-scaled

This seems like a minor issue, but better to not underestimate it, since it may uncover other issues we're missing from testing.

@hypest
Copy link
Contributor

hypest commented May 13, 2019

Will now merge the branches with the fixes I was working on over the last few days, so we can coordinate on a single branch/PR.

@hypest
Copy link
Contributor

hypest commented May 14, 2019

There's an undo bug where an extra character is appearing, which is a web-side issue. Tracked in WordPress/gutenberg#15619.

@hypest
Copy link
Contributor

hypest commented May 14, 2019

The undo issue reported by @daniloercoli in #949 (comment) is happening on the web too (I tried https://frontenberg.tomjn.com/ as well as local WP on GB hash eea04cf and happens there too) so, not a native mobile related issue.

@koke
Copy link
Member

koke commented May 14, 2019

Not sure if it's a regression, but the moment you press backspace, the current format gets deselected:

format-backspace

@koke
Copy link
Member

koke commented May 14, 2019

I found some odd things splitting lists and trying to undo the changes:

odd-lists

  • When you press enter at the beginning of the first item on a list, the content moves to a new item, and the first one remains empty. This is fine.
  • When you press backspace, the first empty item gets removed, but the list formatting is lost on the remaining item
  • Trying to undo the changes leaves just an empty list

@@ -84,6 +84,8 @@
public ReactAztecText(ThemedReactContext reactContext) {
super(reactContext);

setGutenbergMode(true);
Copy link
Contributor

@marecar3 marecar3 May 14, 2019

Choose a reason for hiding this comment

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

Hey @Tug @hypest I have tried to test WPAndroid against this PR and it's crashing with this error: java.lang.NoSuchMethodError: No virtual method setGutenbergMode(Z)V in class Lorg/wordpress/mobile/ReactNativeAztec/ReactAztecText

...... I have realized that I forgot to update aztecVersion on WPAndroid, so everything is ok :)

@etoledom
Copy link
Contributor

etoledom commented May 14, 2019

I found some odd things splitting lists and trying to undo the changes:

This seems to be fixed by: #973

Not sure if it's a regression, but the moment you press backspace, the current format gets deselected:

Just tested this and it's working good now! 🎉


EDIT: Fixed by WordPress/gutenberg@50527d3

With the last updates, I found a small issue on iOS: Sometimes the formats won't go away by pressing the format buttons.

It's easiest to reproduce in the lists block, but it also happens in paragraph. I couldn't reproduce it in Android, just iOS:

formats

I can take a look at this. Done.

@etoledom
Copy link
Contributor

The format library doesn't seem to recognize bold with b tags, that's true for the web version too.
This means that our <p><b>Hello</b> World!</p> paragraph block in the initial example content is "buggy", but this issue is not generated in our end.

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.

LGTM!

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.

Looks good!! 🎉

@SergioEstevao SergioEstevao self-requested a review May 15, 2019 11:46
Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

:shipit: !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants