Mobile - RichText - Set a default value for selection values #40581
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geriux
added
[Status] In Progress
|
Size Change: 0 B Total Size: 1.23 MB ℹ️ View Unchanged
|
geriux
force-pushed
the
rnmobile/fix/text-selection-update
branch
from
c02b0c5
to
2e95aae
Compare
fluiddot
reviewed
Apr 25, 2022
There was a problem hiding this comment.
When the font size or the line-height is changed, it forces a selection update on Aztec to show the new selected values, by setting this.needsSelectionUpdate to true.
In this case where this selection update happens it expects to have a selectionStart and selectionEnd value. On Android if these values are null or undefined it crashes.
Since this is a new case where a block can set the font size/line-height value of its Paragraph inner blocks without being focused it'll crash.
I understand that this case comes from the following condition when the component is updated, right?
I'm wondering if we should also prevent the selection update by checking that selectionStart and selectionEnd are defined, similar to previous conditions within shouldComponentUpdate like:
fluiddot
reviewed
Apr 25, 2022
Comment on lines
1072
to
1073
| start: this.props.selectionStart, | ||
| end: this.props.selectionEnd, | ||
| start: this.props.selectionStart ?? 0, | ||
| end: this.props.selectionEnd ?? 0, |
There was a problem hiding this comment.
I noticed that this.props.selectionStart and this.props.selectionEnd is also used in the following lines after this block:
If we want to ensure that both props are always defined, we might consider having them as constants within the render function, wdyt?
There was a problem hiding this comment.
That's a good idea! I'll update it
The thing is we do want to trigger the selection update so the new font size/line-height is updated accordingly 😅 |
Ok, I see, I thought that this might be covered by just calling |
No problem! it's a bit confusing and that itself needs its own refactor 😄 |
fluiddot
approved these changes
Apr 25, 2022
There was a problem hiding this comment.
LGTM 🎊 ! Awesome work @geriux, thanks for addressing this issue 🙇.
I followed the testing instructions and I confirm that the crash can't be reproduced.
Tested on Samsung Galaxy S20 FE 5G (Android 11) and Simulator - iPhone 13 (iOS 15.4).
…nEnd for undefined values
geriux
force-pushed
the
rnmobile/fix/text-selection-update
branch
from
2e95aae
to
7fef2af
Compare
geriux
removed
the
Mobile App - Automation
geriux
added a commit
that referenced
this pull request
Apr 26, 2022
…nEnd for undefined values (#40581)
4 tasks
geriux
added a commit
that referenced
this pull request
Apr 26, 2022
* Release script: Update react-native-editor version to 1.74.0 * Release script: Update with changes from 'npm run core preios' * Update CHANGELOG * Release script: Update react-native-editor version to 1.74.1 * Release script: Update with changes from 'npm run core preios' * Mobile - RichText - Set a default value for selectionStart / selectionEnd for undefined values (#40581) * Mobile - Update changelog Co-authored-by: Siobhan <siobhan@automattic.com>
SiobhyB
pushed a commit
that referenced
this pull request
Apr 26, 2022
…nEnd for undefined values (#40581)
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #40545
Gutenberg Mobile PR-> Mobile - RichText - Set a default value for selection values wordpress-mobile/gutenberg-mobile#4790What?
This PR fixes an issue on Android where changing the font size/line-height of the Quote block V2 would make the app crash.
Why?
When the font size or the line-height is changed, it forces a selection update on Aztec to show the new selected values, by setting
this.needsSelectionUpdatetotrue.In this case where this selection update happens it expects to have a
selectionStartandselectionEndvalue. On Android if these values arenullorundefinedit crashes.Since this is a new case where a block can set the font size/line-height value of its Paragraph inner blocks without being focused it'll crash.
How?
This PR adds a default value of
0for cases where theselectionStartandselectionEndareundefinedand a selection update was triggered.Testing Instructions
Precondition for enabling v2:
Steps:
Up (^)arrow in the floating toolbar to go up one level and select the parent block if the inner block is focused.Screenshots or screencast