Fix SelectableRegion crash when the selection starts in a scrollable child but does not select anything initially#184164
Conversation
…he scrollable but nothing is selected initially
…use they are possible, previously we were checking _selectionStartsInScrollable and it could only be true or false and false represented both set and uninitialized states, but after becoming nullable null represents uninitialized and false represents set, we should handle this new state the same as if it were false and fail gracefully
|
|
||
| @override | ||
| SelectionResult handleSelectAll(SelectAllSelectionEvent event) { | ||
| assert(!_selectionStartsInScrollable); |
There was a problem hiding this comment.
It's a little unclear what this assert was checking before.
It could be verifying that we only call handleSelectAll when we have not initialized the selection yet inside the scrollable. This is because _selectionStartsInScrollable could be a signal to whether the selection has ever reached this container. This also doesn't make sense to me since it seems reasonable for select all to reach a selectable who was not reached yet.
It could also be verifying that we only call handleSelectAll when the selection starts outside of the scrollable. I think we don't want to do this since it seems reasonable for a user to select some text in a scrollable (_selectionStartsInScrollable will be true), and then proceed with ctrl + a to send a select all event to the scrollable. The reason this crash has not been reported is that SelectableRegionState.selectAll prevents this by calling clearSelection before selecting everything. This is also the reason it does not crash with the new assert _selectionStartsInScrollable == null. Another option here could just to remove the assert.
Fixes #115787
Before this change sometimes when starting a selection gesture such as a long press drag or a double tap drag directly on the empty padding of a
Scrollablethe selection may not be set initially because there was nothing to select in the empty padding. This caused a crash when receiving subsequent edge update events because we believe the selection starts in the scrollable but we do not have any selection edges set.To fix this I made
_selectionStartsInScrollablenullable, this way a subsequent selection edge will not try to set_selectionStartsInScrollableagain if it has already been set removing the need for the assert.handleSelectionEdgeUpdatepreviously assumed that if the cached edges were not set then we should set_selectionStartsInScrollable, the assert was used to verify that_selectionStartsInScrollablewas false i.e. had not been set/changed from its initial value. By making_selectionStartsInScrollablenullable we can ensure a subsequent edge update event does not re-set the value if it has already been set previously.Pre-launch Checklist
///).