Skip to content

Conversation

@willlockwood
Copy link
Contributor

@willlockwood willlockwood commented Dec 24, 2021

This PR fixes #54090

Summary

Currently, the Flutter implementation of TransitionRoute/ModalRoute ensures that, for all material and Cupertino page route transitions, if the route is the current route, then the route is interactive. When the route isn't current, it can't be interacted with. This is the correct logic for android, but on iOS, transitions must be fully complete before the current route can process tap events.

This PR adds in the ability for routes to opt-in to the behavior described for native iOS: when the current route is transitioning (e.g., a route has just been pushed on top of it, or the route is in its own entrance animation, etc.), it can be set to ignore pointers until the transition is over.

This behavior would now be configurable via the ignorePointerDuringTransitions field on ModalRoute.

The implementation here is based on routes tracking whether pointers should be ignored via a notifier that could update any time the animation's or secondaryAnimation's status changes. This should be probably the most performant approach, firstly because rebuilds on the overlay's _ModalScope's existing IgnorePointer will only happen when the value of whether the route should intercept pointers changes, and because whether routes should be interactive could actually change any time there is a change in an animation status.

With this change, it's no longer necessary to track for user gestures in _ModalScope, because that logic is already covered by the AnimationStatus-based approach described above.

Alternatives considered:

  • Not doing this at all (but as a developer, personally I'd really like the ability to configure this to minimize errant extra navigation during transitions)
  • Track whether transitions are in progress on the navigator, similarly to how its done for userGestureInProgress (but this gets very messy because of how often secondary animations get updated without an animation completing its full cycle)
  • Not changing TransitionRoute at all (this became necessary because we need to consult the nextRoute's animation status to decide whether the current route should ignore pointers, for example, after popping a fullscreen dialog route)

Recordings (slowed down for ease of observation):

Native iOS

Native iOS behavior:

Screen.Recording.2021-12-23.at.4.58.03.PM.mov
Flutter (before)

Cupertino:

Screen.Recording.2021-12-23.at.5.06.38.PM.mov

Material:

Screen.Recording.2021-12-23.at.5.07.42.PM.mov
Flutter (after)

Cupertino:

Screen.Recording.2021-12-23.at.5.01.12.PM.mov

Material:

Screen.Recording.2021-12-23.at.5.03.49.PM.mov

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. (fixing up a few now)

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. labels Dec 24, 2021
@willlockwood willlockwood force-pushed the lockwood/ignore-pointer-route-transitions-fidelity branch from 7d513c3 to be7690f Compare December 24, 2021 01:56
@flutter-dashboard flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Dec 24, 2021
@chunhtai chunhtai self-requested a review December 28, 2021 17:26
Copy link
Contributor

Choose a reason for hiding this comment

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

what about _shouldIgnoreFocusRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes thank you for bringing this up, I had this question as well and forgot to comment on it—the code before was written such that it's following the same logic, but I couldn't quite tell if that was incidental to the fact that they happened to share the same conditions at the time.

It sounds like you'd suggest that we have the focusNodeScope should also be set to the value of the notifier? For context, the cases where _ignorePointer is true is currently a super set of the cases where _shouldIgnoreFocusRequest is true

Copy link
Contributor

Choose a reason for hiding this comment

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

For context, the cases where _ignorePointer is true is currently a super set of the cases where _shouldIgnoreFocusRequest is true

Is this true? I am not sure if it is already fixed, but if I remember correctly, the cupertino back swipe can swipe the animation to completed/dismissed if you swipe to the edges but still holding your finger...

Copy link
Contributor Author

@willlockwood willlockwood Dec 30, 2021

Choose a reason for hiding this comment

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

Is this true? I am not sure if it is already fixed, but if I remember correctly, the cupertino back swipe can swipe the animation to completed/dismissed if you swipe to the edges but still holding your finger...

Ah ok good point—I'm not sure. From what I can tell looking at the code, it seems like you're probably right about that, but I'm having trouble verifying. If that's the case, then I think I'll also need to update _maybeUpdateIgnorePointer to take this into account, because at the moment that logic is assuming that all user gestures in progress are caught by the logic of whether the animation statuses are forward/reverse. I'll run an example and double-check.

So if that's the case, what does that mean for this focusScopeNode?

--

I do also get some failing tests when I do update the logic:

  • focus traverse correct when pop multiple page simultaneously
  • focus traversal is correct when popping multiple pages simultaneously - with focused children

Thoughts? I'm a bit unfamiliar with how the focus stuff works but happy to look into it further

Copy link
Contributor Author

@willlockwood willlockwood Dec 30, 2021

Choose a reason for hiding this comment

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

Yup you're correct: when you start a gesture, and then take it back to the start, if you hold your finger down then the gesture will be in progress but my current logic wouldn't ignore pointers there. I'll make an update to that, because I think it probably should continue to block interactions since that's the current behavior

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

secondary LGTM

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

@nt4f04uNd
Copy link
Member

Hi @willlockwood, I'm sorry, the bot didn't land your PR in time for some reason. Can you resolve the merge conflicts and we will try to land it again?

@Hixie
Copy link
Contributor

Hixie commented May 17, 2022

@willlockwood This is so close to landing, just needs conflicts to be resolved — any chance you have a moment to do that?

@willlockwood willlockwood force-pushed the lockwood/ignore-pointer-route-transitions-fidelity branch from c4e7e55 to 4ceb00d Compare May 18, 2022 04:47
@willlockwood
Copy link
Contributor Author

willlockwood commented May 18, 2022

Looks like the checks are failing for what seems to be stuff unrelated to this PR? It says Google testing failed—@goderbauer any chance you could help me understand that part?

Nevermind, looks like it's mine 😅

@chunhtai
Copy link
Contributor

Can you see if this is a real failure?

01:33 +1577: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/routes_test.dart: TransitionRoute does not ignore pointers when route on top of it pops                                               
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
Finder specifies a widget that would not receive pointer events.
A call to tap() with finder "exactly one widget with text "Home" (ignoring offstage widgets):
Text("Home", dependencies: [MediaQuery, DefaultTextStyle])" derived an Offset (Offset(400.0, 300.0))
that would not hit test on the specified widget.
Maybe the widget is actually off-screen, or another widget is obscuring it, or the widget cannot
receive pointer events.
The finder corresponds to this RenderBox: RenderParagraph#c3006(creator: RichText ← Text ← Semantics ← Builder ← RepaintBoundary-[GlobalKey#e912a] ← IgnorePointer ← AnimatedBuilder ← Transform ← ScaleTransition ← FadeTransition ← _ZoomExitTransition ← Transform ← ⋯, parentData: <none> (can use size), constraints: BoxConstraints(w=800.0, h=600.0), semantics node: SemanticsNode#4, size: Size(800.0, 600.0), textAlign: start, textDirection: ltr, softWrap: wrapping at box width, overflow: clip, locale: en_US, maxLines: unlimited)
The hit test result at that offset is: HitTestResult(_RenderColoredBox#fa30f@Offset(400.0, 300.0),
RenderAnimatedOpacity#7116f@Offset(400.0, 300.0), RenderAnimatedOpacity#19675@Offset(400.0, 300.0),
_RenderColoredBox#6aa69@Offset(400.0, 300.0), RenderRepaintBoundary#1f91e@Offset(400.0, 300.0),
_RenderFocusTrap#b76a9@Offset(400.0, 300.0), RenderSemanticsAnnotations#cd2b9@Offset(400.0, 300.0),
RenderOffstage#fbca5@Offset(400.0, 300.0), RenderSemanticsAnnotations#7aaa5@Offset(400.0, 300.0),
_RenderTheatre#2be3c@Offset(400.0, 300.0), RenderSemanticsAnnotations#cd3d7@Offset(400.0, 300.0),
RenderAbsorbPointer#d0047@Offset(400.0, 300.0), RenderPointerListener#f6439@Offset(400.0, 300.0),
RenderCustomPaint#61a55@Offset(400.0, 300.0), RenderSemanticsAnnotations#1807a@Offset(400.0, 300.0),
RenderSemanticsAnnotations#ee4e3@Offset(400.0, 300.0),
RenderSemanticsAnnotations#c7738@Offset(400.0, 300.0),
RenderSemanticsAnnotations#5a43b@Offset(400.0, 300.0),
HitTestEntry<HitTestTarget>#eecb1(RenderView#371da),
HitTestEntry<HitTestTarget>#ae8b1(<AutomatedTestWidgetsFlutterBinding>))
If you expected this target not to be able to receive pointer events, pass "warnIfMissed: false" to
"tap()".
To make this error into a non-fatal warning, set WidgetController.hitTestWarningShouldBeFatal to
false.

When the exception was thrown, this was the stack:
#0      WidgetController._getElementPoint (package:flutter_test/src/controller.dart:937:11)
#1      WidgetController.getCenter (package:flutter_test/src/controller.dart:839:12)
#2      WidgetController.tap (package:flutter_test/src/controller.dart:273:18)
#3      main.<anonymous closure>.<anonymous closure> (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/routes_test.dart:1393:20)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

The test description was:
  does not ignore pointers when route on top of it pops
════════════════════════════════════════════════════════════════════════════════════════════════════

01:33 +1578 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/gesture_detector_semantics_test.dart: ... delegate should update semantics notations when switching to the default delegate        
01:33 +1578 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/routes_test.dart: TransitionRoute does not ignore pointers when route on top of it pops [E]                                        
  Test failed. See exception logs above.
  The test description was: does not ignore pointers when route on top of it pops
  

@willlockwood
Copy link
Contributor Author

The implementation was correct, but the new test broke because the default android transitions builder changed, making the widget I was tapping in my test no longer visible during the transition 👍🏻

@willlockwood
Copy link
Contributor Author

@chunhtai Do you know if this can now be again marked waiting for the tree to go green?

@zanderso
Copy link
Member

@chunhtai
Copy link
Contributor

@zanderso One possibility may be the IgnorePointer of the Route is rebuilt less frequently after this patch. but I am not sure whether it would cause the drop

XilaiZhang added a commit to XilaiZhang/flutter that referenced this pull request May 24, 2022
…itions and do so on `Cupertino` routes (flutter#95757)"

This reverts commit 4c0b0be.
XilaiZhang added a commit that referenced this pull request May 24, 2022
…itions and do so on `Cupertino` routes (#95757)" (#104520)

This reverts commit 4c0b0be.
XilaiZhang added a commit to XilaiZhang/flutter that referenced this pull request May 24, 2022
…itions and do so on `Cupertino` routes (flutter#95757)" (flutter#104520)

This reverts commit 4c0b0be.
CaseyHillers pushed a commit that referenced this pull request May 24, 2022
…itions and do so on `Cupertino` routes (#95757)" (#104520) (#104532)

This reverts commit 4c0b0be.
@willlockwood
Copy link
Contributor Author

So is the main issue here the suspicious perf improvement? Just wanna make sure I'm focused on the right thing when I revisit this

@zanderso
Copy link
Member

No, I don't think that was the reason for the revert. @chunhtai and/or @XilaiZhang will need to give more details about that.

@XilaiZhang
Copy link
Contributor

yes this was the error we saw earlier

══╡ EXCEPTION CAUGHT BY FOUNDATION LIBRARY ╞════════════════════════════════════════════════════════
The following assertion was thrown while dispatching notifications for ValueNotifier<bool>:
setState() or markNeedsBuild() called during build.
This AnimatedBuilder widget cannot be marked as needing to build because the framework is already in
the process of building widgets. A widget can be marked as needing to be built during the build
phase only if one of its ancestors is currently building. This exception is allowed because the
framework builds parent widgets before children, which means a dirty descendant will always be
built. Otherwise, the framework might not visit this widget during this build phase.
The widget on which setState() or markNeedsBuild() was called was:
  AnimatedBuilder
The widget which was currently being built when the offending call was made was:
  Builder

the error was generated by some internal tests written by teams other than flutter. I am not sure if there are open source version of these tests?

@willlockwood
Copy link
Contributor Author

I am not sure if there are open source version of these tests?

Gotcha, thank you for the information! @XilaiZhang or @chunhtai do you know if that's something you can help me figure out? I'm looking at the implementation again, trying to piece together exactly how this exception would be thrown, but it's difficult without some more information (e.g., even just knowing which AnimatedBuilder and Builder are causing this).

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
…itions and do so on `Cupertino` routes (flutter#95757)" (flutter#104520)

This reverts commit 4c0b0be.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: tests "flutter test", flutter_test, or one of our tests f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cupertino route/transition should block touch events

8 participants