Limit start value on ordered lists #47362
Conversation
|
Size Change: +5.17 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
👋 Hey @jhnstn, I'm experiencing erratic input when testing this PR. Do you have the same issue? I checked out Simulator.Screen.Recording.-.iPhone.13.-.2023-03-09.at.19.37.38.mp4 |
There was a problem hiding this comment.
Hey @jhnstn! 👋 It's been a while since reviewing JS so bear with me. 😄 I added some comments that are optional IMO.
I'm only currently blocked by the issue mentioned here, which may be on my end.
| } | ||
| const int32bit = Math.pow( 2, 31 ); | ||
| return Math.min( | ||
| Math.max( parseInt( value, 10 ), -1 * ( int32bit - 1 ) ), |
There was a problem hiding this comment.
Could the const int be reused here instead of parsing the value again?
| import { InspectorControls } from '@wordpress/block-editor'; | ||
| import { TextControl, PanelBody, ToggleControl } from '@wordpress/components'; | ||
|
|
||
| function limitIntegerValue( value ) { |
There was a problem hiding this comment.
Since we're not taking in a min / max and it's specific to signed 32-bit integers, should we make the function name more specific, or add a comment describing what it does?
There was a problem hiding this comment.
I had a different name for the function in an earlier PR a bit more specific and it was suggested that it be named limitValue. I thought that it was too generic so opted for limitIntegerValue to imply that the max min limits were integer limits. But I see your point about the 32 bit integer assumption which is not explicit in the function name. Perhaps a comment is enough?
There was a problem hiding this comment.
IMO a comment is fine if we're keeping it local to these files.
hmm, I don't remember running into this when I was testing but I'll check again. |
|
Thanks for reviewing @twstokes ! While looking into the typing jitter issue you found I noticed that the original issue might be resolved without these changes ? I wonder if it has to do with the upgrade to Android 12 ? I've upgraded my OS from Android 12 to 13 but it's the same device that I used to test the original issue ( I have pixel 6 that supports 64 bit ). Now the behavior on trunk looks fine, no more crashing and overflows reset to 1 ( same as how Gutenberg Web handles overflows. ) I'm not sure we need these changes but could be a problem with folks using 32 bit devices. I did revert the change to the rich text component that I suspect was causing the issue you were seeing. I believe that was added as a preventative measure in the event the rich text component encountered a cursor start value greater than a 32-bit integer. That seems highly unlikely as I think about it now. Keeping the change in the ordered list settings would prevent issues with 32-bit devices and I don't suspect it would be a problem for folks with 64-bit devices. I doubt preventing someone from starting an ordered list at |
I failed to bring this up and noticed that as well in my testing. Same scenario - the physical device is the same but it's since been upgraded from 12 to 13.
Yeah, the number we support is quite unrealistic IMO. Do you propose we keep things as is? I'd at least like to test on an Android 12 device and see if we can still cause a crash. If so, it feels like a responsible thing to do here is to sanitize input to prevent crashing. What are your thoughts? |
I agree on testing for crashes on older versions of Android. I don't have a physical device but I try on an emulator. I think if it's still crashing on older OSs then I think we should ship the change to the ordered list. I don't think we need the change to the rich text component ( which has already been reverted in be5a839 ) |
|
Ok, I just tested on a emulated pixel 3 running Android 11, no crashes. I think it's safe to abandon these changes. |
Very nice! 🥳 |
What?
Limits the start value for ordered lists to fit in a 32 bit integer:
2147483647.This also limits the start value passed to the react native
<RichText>component.Why?
This fixes #39727
The mobile editor on Android is crashing when the start value is larger than an 32 bit integer. iOS can handle the larger values but the UI becomes unstable when the start value is over a 64 bit integer.
How?
I've added two
Math.min(start, MAX_INT)calls where theolstart value is handled in theollist settings and in the props list that is passed to the native<RichText>componentTesting Instructions
2147483647214748364721474836472147483647Screenshots or screencast
Testing Instructions for Keyboard
Screenshots or screencast