Does anyone know if renderEditable.getTransformTo(null) == Matrix4.zero() is an accurate way of telling if a render object is in the current viewport? From the documentation of applyPaintTransform which is called during getTransformTo, this sounds like it is accurate

Some RenderObjects will provide a zeroed out matrix in this method, indicating that the child should not paint anything or respond to hit tests currently. A parent may supply a non-zero matrix even though it does not paint its child currently, for example if the parent is a [RenderOffstage] with offstage set to true. In both of these cases, the parent must return false from [paintsChild].

and from defaultApplyPaintTransform which is called from applyPaintTransform.

  /// Applies the transform that would be applied when painting the given child
  /// to the given matrix.
  ///
  /// Render children whose [TextParentData.offset] is null zeros out the
  /// `transform` to indicate they're invisible thus should not be painted.

I'm not sure I understand this line, the zero matrix isn't special as a perspective transform matrix?

I had this question #138313 (comment) related to that. I think it may be in accurate after thinking some more about it, but what do you think?

can BrowserContextMenu.enabled change?

Oh good catch, I can make this a getter.

I'd recommend using an exhaustive switch instead, in case more supported platforms are added in the future.

You don't need the boolean since the extra bit can be represented by setting _valueWhenShowToolbarOnScreenScheduled to null?

Also this is a bit hard to read.

That makes sense. Thanks for the suggestion, should look cleaner now.

nit: ?.toolbarIsVisible ?? false

do you need to enter the clause if toolbarVisibleAtScrollStart is false?

Good point we do not. I can add a check.

Nit:

Is this the Viewport as in the scrollable RenderViewport rect?

If this is the scrollable RenderViewport, I think you could get the rect from RenderAbstractViewport.of(context)

My intention here was to get a rect that closest represents the visible portion of the flutter view. I want to know if the RenderEditable is visible on the screen. Would RenderAbstractViewport.of(context) still work if we have a TextField within a horizontal scrollable nested within a vertical scrollable?

In that case you may have to bubble up through multiple viewports to find out. Maybe calling getOffsetToReveal for each viewport will work?

Check this out from showInViewport, this is the case where we determine, this is visible we don't need to make it visible. I don't think you need to do the things in the if clause here, it's jsut an example of how we determined it was visible:

Thanks for the suggestion! Something like this works for me, but I realized RenderAbstractViewport doesn't take into account things like status bar or keyboard. In this case maybe the current implementation of calculating the approximate visible viewport rect might be more accurate, what do you think?

  bool? _renderEditableIsVisible(ScrollNotification notification) {
    RenderAbstractViewport? closestViewport = RenderAbstractViewport.maybeOf(renderEditable);
    if (closestViewport == null) {
      return null;
    }
    while (closestViewport != null) {
      final RevealedOffset leadingEdgeOffset = closestViewport.getOffsetToReveal(renderEditable, 0.0);
      final RevealedOffset trailingEdgeOffset = closestViewport.getOffsetToReveal(renderEditable, 1.0);
      final double currentOffset = -notification.metrics.pixels;
      final RevealedOffset? targetOffset = RevealedOffset.clampOffset(
        leadingEdgeOffset: leadingEdgeOffset,
        trailingEdgeOffset: trailingEdgeOffset,
        currentOffset: currentOffset,
      );
      if (targetOffset != null) {
        // `descendant` is between leading and trailing edge and hence already
        //  fully shown on screen. No action necessary.
        return false;
      }
      closestViewport = RenderAbstractViewport.maybeOf(closestViewport.parent);
    }
    return true;
  }

SGTM! 🎉

looks like toolbarIsVisible also accounts for the spellcheck popup.

Are you using _selectionOverlay?.toolbarIsVisible to tell whether showToolbar was called?

Sorry missed this, I am using it to check if the text selection toolbar is visible. toolbarIsVisible does account for spellcheck popup, do you recommend I add a new API that would tell us if only the text selection toolbar is visible? Another route we could go is save some _showToolbarCalled member.

I think I can just use !_selectionOverlay.spellCheckToolbarIsVisible along with toolbarIsVisible to target the text selection toolbar.

Looks like _internalScrolling doesn't have to be an instance variable? Making it a parameter would make it slightly easier to understand imo, as the scope of the variable is smaller.

Also why do we need to distinguish internal / parent scrolling?

I decided to distinguish between internal/parent scrolling because when using only ScrollNotificationObserver, EditableText receives internal scrolling updates too late and the context menu is hidden on TextSelectionGestureDetector.onTapDown which prevents the scroll logic from making the toolbar re-appear. onTapDown is dispatched even though the TextSelectionGestureDetector has not won the arena yet. This is due to the default behavior of TapAndDragGestureRecognizer, where it dispatches onTapDown after kPressTimeout = 100ms if no other recognizer has won the arena. When placing the NotificationListener as a direct parent of the internal Scrollable we are able to catch the ScrollNotification in time to beat the kPressTimeout.

NotificationListener doesn't cover parent Scrollables so I ended up using both. The _internalScrolling flag is mainly to prevent duplicate notifications, since ScrollNotificationObserver will still report internal scrolling notifications.

I was able to get rid of the _internalScrolling and the differentiation by just using ScrollNotificationObserver, however doing this makes EditableText unable to receive any scroll notifications unless it has a parent ScrollNotificationObserver (Scaffold has one but if someone doesn't use it they won't be able to take advantage of this feature). If we go with this route we should document these details somewhere. A pro of this solution is that we are able to conditionally listen for internal scrolling events (only listen when the toolbar is shown), versus NotificationListener where we do not have this control.

Using the previous combination of NotificationListener and ScrollNotificationObserver allows EditableText to still listen to its own internal scrollable notifications without a parent ScrollNotificationObserver. To listen to parent scroll events a user would need to place a ScrollNotificationObserver somewhere higher in the tree. I think if we go with this solution we need to document this detail somewhere. I can also remove the use of the local member _internalScrolling with this solution by checking if the notification context is the same as the internal scrollable context.

Maybe another option would be to place ScrollNotificationObserver even higher in the tree in the framework like MaterialApp or WidgetsApp? This would cover a larger range of users.

Curious to hear thoughts on this @LongCatIsLooong @Piinks

Discussed some of this offline, but leaning towards using the combination of NotificationListener and ScrollNotificationObserver so EditableText can get this functionality for it's internal scrollable without having to place a ScrollNotificationObserver as a parent.

Every editable field in the app now needs to add itself as a listener. Can the logic be moved to the context menu or something (I think there was a singleton class since there can be at most one menu visible), or only add itself as the listener when the menu becomes visible?

Also my understanding is this notifies listeners when any descendent scrolls, even when the scrolling scrollable and the active text field are in different subtrees. That probably shouldn't cause the toolbar to disappear as it is not scrolling at all.

Good point about every editable field having to listen, i'll try out your suggestions and see what works best. WRT to ScrollNotificationObserver notifying on every descendant scroll, I tried checking if the EditableTexts context contains the notification.context in this commit e172707. Is there a better way to do this, then looking up the tree?

The documentation states that:

Notifications bubble up through the tree, which means a given NotificationListener will receive notifications for all descendant Scrollable widgets. To focus on notifications from the nearest Scrollable descendant, check that the depth property of the notification is zero.

So I think that means only Scrollables send notifications so you can do that more efficiently by going up the Scrollable tree instead (but you may want to avoid calling Scrollable.of since it establishes a dependency).

Yeah, in this case, you would likely not want to check notification depth and limit it to 0. The depth increases for each nested scrollable.

return _scrollableKey.currentContext == scrollableState?.context;

nit: view.padding.horizontal

Do you have other plans to update the implementation here? Using the view dimensions is a bit strange, for example when the viewport is not fullscreen.

(Or you could move this to a different PR and always show the edit menu for now)

I don't follow, do you mean when the textfield is inside a viewport that is not the fullscreen?

My thought with the logic here is:

selectionIsVisible tells us if the selection is visible within the RenderEditable.
renderEditableBounds is the Rect for the RenderEditable, if it hasNaN it is likely not laid out.
viewportRect attempts to calculate the device entire viewport with paddings for system overlays.
renderEditableInView checks whether the viewportRect and renderEditableBounds overlap, if they do then the RenderEditable should be visible within the device viewport.

The final check is selectionIsVisible && renderEditableInView which checks if the selection is visible in the RenderEditable and if the RenderEditable is visible within the device viewport.

Ah ok thanks for the explanation! But why do you need to adjust for system overlays? If the menu is too close to system overlays shouldn't it move to the other side of the text field to keep itself visible?

I think it makes more sense to hide the overlay if the RenderEditable itself is not visible. This way the menu still adjusts to avoid the overlays if the text field is visible and hides it when its completely obscured to prevent the menu from overlapping with the system overlays. This seems consistent with the Android behavior.

Taking system overlays into account:
https://github.com/flutter/flutter/assets/948037/f45c9172-0815-4362-bcfd-01b4af02b2d6

Without taking system overlays into account:
https://github.com/flutter/flutter/assets/948037/4fe00dab-eccb-49fa-8627-ef5a2a65f8a3

Jetpack compose behavior:
https://github.com/flutter/flutter/assets/948037/76ca8bce-dc27-4e78-a8d3-4289b636c413

I see the behavior makes sense to me. But the code seems to only check for system UI obstruction, that seems oddly specific. If the text field is obstructed by the outer scroll view would the menu also disappear?

Isn't showOnScreen/showInViewport/or ensureVisible called as part of focusing on a text field and showing the context menu? If so, it should account for the case where a nested scroll view is scrolled out of view by the outer scroll view.

Yeah my implementation of trying to calculate the viewport rect didn't take into account for this case because the viewport is not always the devices fullscreen. I was able to fix this case by going back to your original suggestion @Piinks #138313 (comment) and bubbling through the viewports. I still think we should consider the calculated viewport rect for system overlays like the keyboard.

nit: use && instead of nested ifs

I think I can actually remove this nested check considering _valueWhenToolbarShowScheduled is only non-null when toolbarIsVisible.

nit: the implementation of this check is relatively expensive. Consider doing other checks first (e.g., filtering out notifications that aren't start/end.

nit: empty new line after. Also I think it's usually 3 #'s?

I think they are using 2 #'s from looking at the other TextField/CupertinoTextField/EditableText subsections.

nit: most of the time this is going to be the case I think? If so maybe move this to the beginning of the paragraph so that it's immediately clear that in most cases it just works.

ScrollNotificationObserver is an InheritedWidget. You need to update the subscription in didChangeDependencies right?

(Also consider adding a test for that).

Ah ok thanks for the explanation! But why do you need to adjust for system overlays? If the menu is too close to system overlays shouldn't it move to the other side of the text field to keep itself visible?

nit: you could just compare the ScrollableState?

Are you using _selectionOverlay?.toolbarIsVisible to tell whether showToolbar was called?

nit: needs an extra line.

maybe: If this text field is not a descendant of Scaffold, then consider doing ...., because ....

You're using ScrollNotificationObserver.maybeOf(context) to get the observer so I think there's a chance for it to be null? So _scrollNotificationObserver != null doesn't seem to be equivalent to "an observer is needed".