Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TextField context menu should fade on scroll on mobile devices #138313

Merged
merged 68 commits into from
Feb 6, 2024

Conversation

Renzo-Olivares
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares commented Nov 12, 2023

This change affects Android and iOS devices using the TextField's context menu. After this change the context menu will fade out when scrolling the text and fade in when the scroll ends.

If the scroll ends and the selection is outside of the view, then the toolbar will be scheduled to show in a future scroll end. This toolbar scheduling can be invalidated if the TextEditingValue changed anytime between the scheduling and when the toolbar is ready to be shown.

This change also fixes a regression where the TextField context menu would not fade when the selection handles where not visible.

When using the native browser context menu this behavior is not controlled by Flutter.

Screen.Recording.2023-11-13.at.1.36.49.PM.mov

Fixes #52425
Fixes #105804
Fixes #52426

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 12, 2023
@github-actions github-actions bot added the f: gestures flutter/packages/flutter/gestures repository. label Nov 20, 2023
hideToolbar(false);
}
} else if (notification is ScrollEndNotification) {
final Rect paintBounds = renderEditable.paintBounds;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
} else if (notification is ScrollEndNotification) {
final bool selectionIsVisible = renderEditable.selectionStartInViewport.value || renderEditable.selectionEndInViewport.value;
final bool renderEditableInView = renderEditable.getTransformTo(null) != Matrix4.zero();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

bool _toolbarVisibleAtScrollStart = false;
bool _showToolbarOnScreenScheduled = false;
bool _internalScrolling = false;
final bool _webContextMenuEnabled = kIsWeb && BrowserContextMenu.enabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can BrowserContextMenu.enabled change?

Copy link
Contributor Author

@Renzo-Olivares Renzo-Olivares Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch, I can make this a getter.

@@ -3536,11 +3553,67 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
}
}

final bool _platformSupportsFadeOnScroll = defaultTargetPlatform == TargetPlatform.android
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
} else if (_toolbarVisibleAtScrollStart) {
_showToolbarOnScreenScheduled = true;
_valueWhenShowToolbarOnScreenScheduled = _value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this is a bit hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// did not change between the time the toolbar was first
// scheduled to be shown to when it is ready to be shown.
if (notification is ScrollStartNotification) {
_toolbarVisibleAtScrollStart = _selectionOverlay != null && _selectionOverlay!.toolbarIsVisible;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ?.toolbarIsVisible ?? false

if (_toolbarVisibleAtScrollStart) {
hideToolbar(false);
}
} else if (notification is ScrollEndNotification) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 3556 to 3673
TargetPlatform.android => true,
TargetPlatform.iOS => true,
TargetPlatform.fuchsia => false,
TargetPlatform.linux => false,
TargetPlatform.macOS => false,
TargetPlatform.windows => false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
TargetPlatform.android => true,
TargetPlatform.iOS => true,
TargetPlatform.fuchsia => false,
TargetPlatform.linux => false,
TargetPlatform.macOS => false,
TargetPlatform.windows => false,
TargetPlatform.android ||
TargetPlatform.iOS => true,
TargetPlatform.fuchsia ||
TargetPlatform.linux ||
TargetPlatform.macOS ||
TargetPlatform.windows => false,

}

final Rect renderEditableBounds = MatrixUtils.transformRect(renderEditable.getTransformTo(null), renderEditable.paintBounds);
final Rect viewportRect = _calculateViewportRect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the Viewport as in the scrollable RenderViewport rect?

_handleContextMenuOnScroll(notification);
}

Rect _calculateViewportRect() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

if (targetOffset == null) {
// `descendant` is between leading and trailing edge and hence already
// fully shown on screen. No action necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM! 🎉

if (_valueWhenToolbarShowScheduled != null) {
return;
}
final bool toolbarIsVisible = _selectionOverlay?.toolbarIsVisible ?? false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like toolbarIsVisible also accounts for the spellcheck popup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Renzo-Olivares Renzo-Olivares Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

clipBehavior: widget.clipBehavior,
child: NotificationListener<ScrollNotification>(
onNotification: (ScrollNotification notification) {
_internalScrolling = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Renzo-Olivares Renzo-Olivares Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -2800,6 +2808,10 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
void didChangeDependencies() {
super.didChangeDependencies();

_scrollNotificationObserver?.removeListener(_handleContextMenuOnParentScroll);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Renzo-Olivares Renzo-Olivares force-pushed the textfield-fade-scroll branch 2 times, most recently from d2277d6 to 8b25fb4 Compare November 29, 2023 00:45
@github-actions github-actions bot added f: cupertino flutter/packages/flutter/cupertino repository and removed f: gestures flutter/packages/flutter/gestures repository. labels Dec 13, 2023

bool _isInternalScrollableNotification(BuildContext? notificationContext) {
final ScrollableState? scrollableState = notificationContext?.findAncestorStateOfType<ScrollableState>();
if (_scrollableKey.currentContext == scrollableState?.context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return _scrollableKey.currentContext == scrollableState?.context;

final Size screenSize = MediaQuery.sizeOf(context);
final ui.FlutterView view = View.of(context);
final double obscuredVertical = (view.padding.top + view.padding.bottom + view.viewInsets.bottom) / view.devicePixelRatio;
final double obscuredHorizontal = (view.padding.left + view.padding.right) / view.devicePixelRatio;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: view.padding.horizontal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
final bool toolbarIsVisible = _selectionOverlay?.toolbarIsVisible ?? false;
_valueWhenToolbarShowScheduled = toolbarIsVisible ? _value : null;
if (_valueWhenToolbarShowScheduled != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use && instead of nested ifs

Copy link
Contributor Author

@Renzo-Olivares Renzo-Olivares Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (_isInternalScrollableNotification(notification.context)) {
return;
}
if (!_scrollableNotificationIsFromSameSubtree(notification.context)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -164,6 +164,14 @@ class _CupertinoTextFieldSelectionGestureDetectorBuilder extends TextSelectionGe
///
/// {@macro flutter.widgets.editableText.showCaretOnScreen}
///
/// ## Scrolling Considerations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// When [CupertinoTextField] is placed within a [Scrollable] or within nested
/// [Scrollable]s, consider placing a [ScrollNotificationObserver] above the
/// root [Scrollable] to ensure proper scroll coordination for [CupertinoTextField]
/// and its components like [TextSelectionOverlay]. If a [Scaffold] is present
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// hidden during a scroll on supported platforms.
if (_platformSupportsFadeOnScroll) {
_scrollNotificationObserver?.removeListener(_handleContextMenuOnParentScroll);
_scrollNotificationObserver = ScrollNotificationObserver.maybeOf(context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also consider adding a test for that).

final Size screenSize = MediaQuery.sizeOf(context);
final ui.FlutterView view = View.of(context);
final double obscuredVertical = (view.padding.top + view.padding.bottom + view.viewInsets.bottom) / view.devicePixelRatio;
final double obscuredHorizontal = (view.padding.left + view.padding.right) / view.devicePixelRatio;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// The notification context of a ScrollNotification points to the RawGestureDetector
// of the Scrollable. We get the ScrollableState associated with this notification
// by looking up the tree.
final BuildContext? notificationScrollableContext = notificationContext.findAncestorStateOfType<ScrollableState>()?.context;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could just compare the ScrollableState?

if (_valueWhenToolbarShowScheduled != null) {
return;
}
final bool toolbarIsVisible = _selectionOverlay?.toolbarIsVisible ?? false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Renzo-Olivares Renzo-Olivares force-pushed the textfield-fade-scroll branch 2 times, most recently from dfc4638 to db15661 Compare January 5, 2024 22:05
@Renzo-Olivares
Copy link
Contributor Author

A friendly bump

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry looks like I forgot to press the submit review button.

@@ -163,6 +163,14 @@ class _SelectableTextSelectionGestureDetectorBuilder extends TextSelectionGestur
/// To make [SelectableText] react to touch events, use callback [onTap] to achieve
/// the desired behavior.
///
/// ## Scrolling Considerations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: needs an extra line.

@@ -164,6 +164,18 @@ class _CupertinoTextFieldSelectionGestureDetectorBuilder extends TextSelectionGe
///
/// {@macro flutter.widgets.editableText.showCaretOnScreen}
///
/// ## Scrolling Considerations
///
/// If a [Scaffold] is used as the parent of the [Scrollable]s that contain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -2915,6 +2933,15 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
hideToolbar();
}
}

if (_scrollNotificationObserver != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

@auto-submit auto-submit bot merged commit 0903bf7 into flutter:master Feb 6, 2024
69 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Feb 6, 2024
…6066)

Manual roll requested by tarrinneal@google.com

flutter/flutter@0b5cd50...e6ba809

2024-02-06 engine-flutter-autoroll@skia.org Roll Packages from ae3494d to 1a5a7ce (2 revisions) (flutter/flutter#142985)
2024-02-06 rmolivares@renzo-olivares.dev TextField context menu should fade on scroll on mobile devices (flutter/flutter#138313)
2024-02-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9c1b6c98f7f0 to 808886312e2b (1 revision) (flutter/flutter#142959)
2024-02-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9bd98bc2fcdf to 9c1b6c98f7f0 (4 revisions) (flutter/flutter#142954)
2024-02-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from f34c658b9600 to 9bd98bc2fcdf (1 revision) (flutter/flutter#142950)
2024-02-05 147121557+ShaunByrne-UniSA@users.noreply.github.com Grey out non-selectable days in CupertinoDatePicker (flutter/flutter#136181)
2024-02-05 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.0 to 4.3.1 (flutter/flutter#142944)
2024-02-05 magder@google.com Run examples_smoke_test on Linux (flutter/flutter#142736)
2024-02-05 233583+mossmana@users.noreply.github.com Update AGP version validation code to support KGP and kotlin build files. (flutter/flutter#142357)
2024-02-05 dustbin4ever@gmail.com Fixed test in language_version_test.dart that failed when shuffling, � (flutter/flutter#142904)
2024-02-05 15619084+vashworth@users.noreply.github.com Move Mac_build_test flutter_gallery__transition_perf_e2e_ios to staging (flutter/flutter#142918)
2024-02-05 engine-flutter-autoroll@skia.org Roll Packages from d37fb0a to ae3494d (3 revisions) (flutter/flutter#142915)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…lutter#6066)

Manual roll requested by tarrinneal@google.com

flutter/flutter@0b5cd50...e6ba809

2024-02-06 engine-flutter-autoroll@skia.org Roll Packages from ae3494d to 1a5a7ce (2 revisions) (flutter/flutter#142985)
2024-02-06 rmolivares@renzo-olivares.dev TextField context menu should fade on scroll on mobile devices (flutter/flutter#138313)
2024-02-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9c1b6c98f7f0 to 808886312e2b (1 revision) (flutter/flutter#142959)
2024-02-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9bd98bc2fcdf to 9c1b6c98f7f0 (4 revisions) (flutter/flutter#142954)
2024-02-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from f34c658b9600 to 9bd98bc2fcdf (1 revision) (flutter/flutter#142950)
2024-02-05 147121557+ShaunByrne-UniSA@users.noreply.github.com Grey out non-selectable days in CupertinoDatePicker (flutter/flutter#136181)
2024-02-05 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.3.0 to 4.3.1 (flutter/flutter#142944)
2024-02-05 magder@google.com Run examples_smoke_test on Linux (flutter/flutter#142736)
2024-02-05 233583+mossmana@users.noreply.github.com Update AGP version validation code to support KGP and kotlin build files. (flutter/flutter#142357)
2024-02-05 dustbin4ever@gmail.com Fixed test in language_version_test.dart that failed when shuffling, � (flutter/flutter#142904)
2024-02-05 15619084+vashworth@users.noreply.github.com Move Mac_build_test flutter_gallery__transition_perf_e2e_ios to staging (flutter/flutter#142918)
2024-02-05 engine-flutter-autoroll@skia.org Roll Packages from d37fb0a to ae3494d (3 revisions) (flutter/flutter#142915)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
3 participants