Update TextSelectionOverlay

[justinmc]

TextSelectionOverlay wasn't getting updated when parameters that affect it changed in EditableText. This PR recreates the class when those parameters change.

Fixes #142077

[justinmc]

I changed this name just so that I could uniquely refer to it in a test (there is another _TextSelectionHandlePainter in material/text_selection.dart).

[Hixie]

extremely trivial nit and feel free to ignore: if the ||s are on the right hand side, rather than the left, things line up slightly more clearly

[justinmc]

I like it 👍

[Hixie]

is the dispose necessary? it's omitted on line 4290.

[justinmc]

I played around with this and yes it's necessary, things from the old overlay stick around on the screen if it's not there. It looks like 4290 should have made that assignment only when it was null, which I'll fix.

[Hixie]

what does this look like from an animation perspective? will it cause weird effects if someone is trying to, say, animate the magnifierConfiguration?

[justinmc]

I tried it and it does as I feared. I've created an issue to track this: #142763

Screen.Recording.2024-02-01.at.4.21.51.PM.mov

[Hixie]

[Hixie]

So the problem with this approach (I just discovered, when trying it out with blankcanvas) is that if any of the settings do change, the menu disappears. In blankcanvas, the text field is rebuilt in an animation when the mouse enters or exits the text field. This means the context menu builder is recreated every frame. This means I can't bring up the menu -- if I right click the field, the menu is visible for one frame, then is immediately disposed.

There are workarounds I could apply in blankcanvas, e.g. caching the context menu builder, but fundamentally I don't think there's any reason the context menu builder itself shouldn't be animated, which right now just wouldn't work.

Instead I think we need to have some approach that updates the selection overlay with the new values.

[Hixie]

(The code does attempt to avoid this issue but it doesn't seem to be working for some reason... I wonder if it's something to do with the context menu fade in / fade out animations or something...)

[Hixie]

Continuing my stream of consciousness: shouldShowToolbar and shouldShowHandles are both false when it happens in my case...

[Hixie]

in SelectionOverlay.toolbarIsVisible, everything is false (selectionControls is TextSelectionHandleControls, _contextMenuController.isShown, _spellCheckToolbarController.isShown, and _toolbar != null are all false)

[Hixie]

Flipping the test in toolbarIsVisible (is! instead of is) seems to make it behave as intended:

  bool get toolbarIsVisible {
    return selectionControls is! TextSelectionHandleControls
        ? _contextMenuController.isShown || _spellCheckToolbarController.isShown
        : _toolbar != null || _spellCheckToolbarController.isShown;
  }

...however "as intended" is no good in this case, because what it does in the blankcanvas case is every time you move the mouse near the text field, the context menu disappears and fades back in!

[justinmc]

Ah bummer. Well it should be doable with some refactoring of widgets/text_selection.dart, it just doesn't seem to have been built with these kinds of updates in mind.

I think we can track this in #142969.

I think the logic flipping you did is not right. TextSelectionHandleControls is used to indicate that the _contextMenuController approach should be used. Sorry, it's my weird 2 step deprecation striking again.

[Hixie]

Yeah I think the bug is that selectionControls can be null and this test doesn't check for that.