[RNMobile] Try fixing split/merge issue on native mobile by dropping …

…selection update event when late (#29683) * Bump native event count and drop selection update event if late * Limit the event count bump and event dropping to iOS * Sync native mobile event counter to the JS side one This way, when the JS bumps the counter, the native side picks it up and increments from there. * More usage of event counter bumping instead of undefining * Introducing shouldDropEventFromAztec and using it in onChange too So to drop the Aztec onChange events too, if late. * Don't bump the local event counter on componentDidUpdate If the update was due to content change from the JS side of things, the counter would have already been bumped in shouldComponentUpdate (hopefully), so bumping it again in DidUpdate introduces a counter bump that gets out-of-sync with Aztec since (the bump inside DidUpdate doesn't cause a render and thus no new counter sent to Aztec). * Use window.console, to satisfy linter * Fix typo in comments Co-authored-by: Joel Dean <[email protected]> * Sync changelog instructions for native mobile * Changelog entry for the split/merge regression fix Co-authored-by: Joel Dean <[email protected]>
WordPress · Mar 18, 2021 · 028709c · 028709c
1 parent b319dda
commit 028709c
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 9 deletions.
diff --git a/packages/react-native-aztec/CHANGELOG.md b/packages/react-native-aztec/CHANGELOG.md
@@ -1,3 +1,13 @@
 <!-- Learn how to maintain this file at https://github.com/WordPress/gutenberg/tree/HEAD/packages#maintaining-changelogs. -->
 
-## Unreleased
+<!--
+For each user feature we should also add a importance categorization label  to indicate the relevance of the change for end users of GB Mobile. The format is the following:
+[***] → Major new features, significant updates to core flows, or impactful fixes (e.g. a crash that impacts a lot of users) — things our users should be aware of.
+
+[**] → Changes our users will probably notice, but doesn’t impact core flows. Most fixes.
+
+[*] → Minor enhancements and fixes that address annoyances — things our users can miss.
+-->
+
+## Unreleased
+* [*] Block split/merge fix for a (never shipped) regression (Android only) [#29683]
diff --git a/...-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java b/...-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java
@@ -217,7 +217,8 @@ public void setText(ReactAztecText view, ReadableMap inputMap) {
             // force a 2nd setText from JS side to Native, just set a high eventCount
             int eventCount = inputMap.getInt("eventCount");
 
-            if (view.mNativeEventCount < eventCount) {
+            if (view.getEventCounter() < eventCount) {
+                view.setEventCounterSyncFromJS(eventCount);
                 setTextfromJS(view, inputMap.getString("text"), inputMap.getMap("selection"));
             }
         }

diff --git a/...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java b/...ive-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecText.java
@@ -66,7 +66,8 @@ public class ReactAztecText extends AztecText {
 
     // FIXME: Used in `incrementAndGetEventCounter` but never read. I guess we can get rid of it, but before this
     // check when it's used in EditText in RN. (maybe tests?)
-    int mNativeEventCount = 0;
+    private int mNativeEventCount = 0;           //  \ Using two distinct counters to avoid race conditions,
+    private int mEventCountSyncFromJS = 0; //  / each side is responsible for bumping the respective counter.
 
     String lastSentFormattingOptionsEventString = "";
     boolean shouldHandleOnEnter = false;
@@ -377,7 +378,22 @@ private void setIntrinsicContentSize() {
 
     //// Text changed events
 
+    public int getEventCounter() {
+        return mNativeEventCount;
+    }
+
+    public void setEventCounterSyncFromJS(int syncToValue) {
+        mEventCountSyncFromJS = syncToValue;
+    }
+
     public int incrementAndGetEventCounter() {
+        if (mNativeEventCount < mEventCountSyncFromJS) {
+            // need to sync up to the counter the JS side is expecting. Avoiding setting
+            //   mNativeEventCount directly from the JS side to avoid race conditions, and instead
+            //   syncing just-in-time when we need the new increment
+            mNativeEventCount = mEventCountSyncFromJS;
+        }
+
         return ++mNativeEventCount;
     }
 

diff --git a/packages/rich-text/src/component/index.native.js b/packages/rich-text/src/component/index.native.js
@@ -117,6 +117,12 @@ export class RichText extends Component {
 		this.convertFontSizeFromString = this.convertFontSizeFromString.bind(
 			this
 		);
+		this.manipulateEventCounterToForceNativeToRefresh = this.manipulateEventCounterToForceNativeToRefresh.bind(
+			this
+		);
+		this.shouldDropEventFromAztec = this.shouldDropEventFromAztec.bind(
+			this
+		);
 		this.state = {
 			activeFormats: [],
 			selectedFormat: null,
@@ -221,7 +227,7 @@ export class RichText extends Component {
 
 	insertString( record, string ) {
 		if ( record && string ) {
-			this.lastEventCount = undefined;
+			this.manipulateEventCounterToForceNativeToRefresh(); // force a refresh on the native side
 			const toInsert = insert( record, string );
 			this.onFormatChange( toInsert );
 		}
@@ -285,6 +291,10 @@ export class RichText extends Component {
 	 * Handles any case where the content of the AztecRN instance has changed
 	 */
 	onChangeFromAztec( event ) {
+		if ( this.shouldDropEventFromAztec( event, 'onChange' ) ) {
+			return;
+		}
+
 		const contentWithoutRootTag = this.removeRootTagsProduceByAztec(
 			unescapeSpaces( event.nativeEvent.text )
 		);
@@ -365,6 +375,10 @@ export class RichText extends Component {
 	}
 
 	handleDelete( event ) {
+		if ( this.shouldDropEventFromAztec( event, 'handleDelete' ) ) {
+			return;
+		}
+
 		const { keyCode } = event;
 
 		if ( keyCode !== DELETE && keyCode !== BACKSPACE ) {
@@ -592,7 +606,22 @@ export class RichText extends Component {
 		this.props.onSelectionChange( start, end );
 	}
 
+	shouldDropEventFromAztec( event, logText ) {
+		const shouldDrop =
+			! this.isIOS && event.nativeEvent.eventCount <= this.lastEventCount;
+		if ( shouldDrop ) {
+			window.console.log(
+				`Dropping ${ logText } from Aztec as its event counter is older than latest sent to the native side. Got ${ event.nativeEvent.eventCount } but lastEventCount is ${ this.lastEventCount }.`
+			);
+		}
+		return shouldDrop;
+	}
+
 	onSelectionChangeFromAztec( start, end, text, event ) {
+		if ( this.shouldDropEventFromAztec( event, 'onSelectionChange' ) ) {
+			return;
+		}
+
 		// `end` can be less than `start` on iOS
 		// Let's fix that here so `rich-text/slice` can work properly
 		const realStart = Math.min( start, end );
@@ -667,13 +696,28 @@ export class RichText extends Component {
 		return value;
 	}
 
+	manipulateEventCounterToForceNativeToRefresh() {
+		if ( this.isIOS ) {
+			this.lastEventCount = undefined;
+			return;
+		}
+
+		if ( typeof this.lastEventCount !== 'undefined' ) {
+			this.lastEventCount += 100; // bump by a hundred, hopefully native hasn't bombarded the JS side in the meantime.
+		} else {
+			window.console.warn(
+				"Tried to bump the RichText native event counter but was 'undefined'. Aborting bump."
+			);
+		}
+	}
+
 	shouldComponentUpdate( nextProps ) {
 		if (
 			nextProps.tagName !== this.props.tagName ||
 			nextProps.reversed !== this.props.reversed ||
 			nextProps.start !== this.props.start
 		) {
-			this.lastEventCount = undefined;
+			this.manipulateEventCounterToForceNativeToRefresh(); // force a refresh on the native side
 			this.value = undefined;
 			return true;
 		}
@@ -703,7 +747,7 @@ export class RichText extends Component {
 				this.needsSelectionUpdate = true;
 			}
 
-			this.lastEventCount = undefined; // force a refresh on the native side
+			this.manipulateEventCounterToForceNativeToRefresh(); // force a refresh on the native side
 		}
 
 		if ( ! this.comesFromAztec ) {
@@ -715,7 +759,7 @@ export class RichText extends Component {
 				nextProps.__unstableIsSelected
 			) {
 				this.needsSelectionUpdate = true;
-				this.lastEventCount = undefined; // force a refresh on the native side
+				this.manipulateEventCounterToForceNativeToRefresh(); // force a refresh on the native side
 			}
 		}
 
@@ -748,7 +792,6 @@ export class RichText extends Component {
 	componentDidUpdate( prevProps ) {
 		if ( this.props.value !== this.value ) {
 			this.value = this.props.value;
-			this.lastEventCount = undefined;
 		}
 		const { __unstableIsSelected: isSelected } = this.props;
 
@@ -772,7 +815,7 @@ export class RichText extends Component {
 		let value = this.valueToFormat( record );
 
 		if ( value === undefined ) {
-			this.lastEventCount = undefined; // force a refresh on the native side
+			this.manipulateEventCounterToForceNativeToRefresh(); // force a refresh on the native side
 			value = '';
 		}
 		// On android if content is empty we need to send no content or else the placeholder will not show.