don't allow setSelection on invalid range #835
Conversation
| @@ -194,7 +194,9 @@ private void updateSelectionIfNeeded(ReactAztecText view, @Nullable ReadableMap | |||
| if ( selection != null ) { | |||
| int start = selection.getInt("start"); | |||
| int end = selection.getInt("end"); | |||
| view.setSelection(start, end); | |||
| if (view.getText().length() > end && start >= 0) { | |||
There was a problem hiding this comment.
I think we can add the equality to end in the condition since the caret can actually be placed right after the last char.
There was a problem hiding this comment.
Also, let's add a line comment about why we're doing this here, wdyt?
There was a problem hiding this comment.
both addressed in 7b8955a
…ing the added safeguard check
Generated by 🚫 dangerJS |
|
Just changed the base branch as we won't be merging it into |
|
FWIW, the ticket this PR was trying to address (#836) is closed now as it's not happening anymore so, we could concider closing this PR. Though, the fix here is one that we might still consider doing if we end up not fixing the root causes of how come the RN side tries to set the selection to a position that is beyond the text length. Context: #871 (comment) |
|
I think while the check looks harmless it could hide other problems so if the most urgent problem is not happening anymore I'd lean more to closing this one - also the references to this PR will still be there in case we need to grab n go with them in case we decide to not pursue the root cause further 👍 |
That's exactly what I'm struggling with at the moment 😬. This workaround works but masks all errors that stem from the RN side getting out-of-sync with Aztec. It will hide problems that we might miss or make hard to debug. Looping back, I've taken a slightly different route with WordPress/gutenberg#15021. Instead of letting Aztec accept the out-of-bounds selection range, I'm checking in the RN side when the html will get trimmed and avoid trying to set the caret in that case. |
Fixes the third case reported in this comment here: #208 (comment)
As expressed elsewhere, while the root cause of it may lay on how undo/redo and lists changes are working together, the extra check here should be harmless and fixes the issue for the time being.