Support ensureVisible/showOnScreen/showInViewport for 2D viewport #128812

Piinks · 2023-06-13T18:01:03Z

This adds support for the static viewport method ensureVisible to work in the 2D viewport. This means default focus traversal will work as well out of the box! :)

Screen.Recording.2023-06-13.at.2.49.46.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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Piinks · 2023-06-13T19:58:57Z

Ok, this is ready for review. I know there is a breakage in flutter/cocoon from the change in function signature. I am looking to see what options there are to land this with that.

Piinks · 2023-06-13T22:10:54Z

Looking to resolve the customer failure in flutter/cocoon#2838

This removes the _FakeViewport from the build dashboard implementation. It was added in #741, and we think it's sole purpose was the preserve the 1D expectations of widgets in the framework like scrollbars and overscroll indicators. Since adding support for 2D in the framework, the framework has been updated so that the fake viewport should no longer be necessary. All of the tests pass, so I think this is safe to remove now that the framework is 2D compatible. This came up as a test failure in flutter/flutter#128812, where the function signature of getOffsetToReveal is being changed.

Piinks · 2023-06-13T22:32:47Z

Customer patch landed, updating customer tests in flutter/tests#271

packages/flutter/lib/src/rendering/viewport.dart

goderbauer · 2023-06-14T16:48:05Z

packages/flutter/lib/src/rendering/viewport.dart

So, if I need to know where to scroll my 2d scrollable to to make something visible, I have to call this twice, once for each axis?

What offset will I get if I ask a 2D scroller without providing an axis?

So, if I need to know where to scroll my 2d scrollable to to make something visible, I have to call this twice, once for each axis?

Wouldn't this make RevealedOffset.rect for both individually somewhat meaningless since, when the offsets in both axises are applied (which I think is the goal - would you ever just want to only reveal in once axis?), the target will be located at neither rects? (I forget what exactly we use those rects for, I think it is relevant for a11y scrolling, see next comment?)

What offset will I get if I ask a 2D scroller without providing an axis?

It will assert.

Since ensureVisible must go through the scroll position, which calls getOffsetToReveal, returning two offsets is irrelevant to only one of the two scroll positions.

packages/flutter/lib/src/widgets/scrollable.dart

goderbauer · 2023-06-14T16:56:25Z

packages/flutter/lib/src/widgets/scrollable.dart

nit: this and line 458 will find the same renderobject, maybe only find it once and store it in a variable since it cannot change between search operations?

goderbauer · 2023-06-14T17:09:04Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

The comment on line 906 said that pivot is the last RenderBox before the ViewPort. Doesn't that mean that its parent is this Viewport? In 1D scrollables there are slivers between the Viewport and the last RenderBox, what do we have in the 2d case there where we don't have slivers?

What am I missing about how this code works?

Now that I am re-reading the comment on lone 905/906: In a 2d viewport, what is the difference between child and pivot ? Wouldn't they always point to the same?

In the case of the code in the PR description, after 7 loops the child is the repaint boundary and pivot is the actual 200x200 constrained box it wraps. The target the method is called with is the focus node of the actual checkbox within, so we bubble up from there to the top level child.

What does the render tree look like for that scenario?

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

goderbauer · 2023-06-14T17:24:46Z

packages/flutter/lib/src/rendering/viewport.dart

I am wondering how this change plays into implicit accessibility scrolling. This method is basically the workhorse behind implicit a11y scrolling powering RenderViewportBase.showInViewport, which is called from RenderObject.showOnScreen.

(Implicit a11y scrolling is if you move the a11y focus beyond the edge of a scrollable and it then automatically reveals the item that has gotten a11y focus.)

I've added the equivalent implementations of showInViewport and showOnScreen, I need to add more tests, but wanted to check first to see what you thought of the approach. I am carrying through the concept of providing which axis of the two. We did a similar change to Scrollable.of, and I think it makes sense to stay consistent here.

packages/flutter/lib/src/widgets/scrollable.dart

goderbauer · 2023-06-14T17:40:16Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

After reading through this PR, I wonder if instead of adding the axis param (and then somewhat awkwardly asserting whether you passed in the right one (for 1D scrollers) or any axis at all (for 2D scrollers)), we could "just" extend the return type RevealedOffset to include a second offset. The existing RevealedOffset.offset would be the offset for the mainAxis (= only axis for 1D scrollers and whatever was declared as main for 2D scrollers) and the newly added notMainAxisOffset property (name TBD) would be either 0 for 1D scrollers or the offset required to reveal the thing in the other axis for 2D scrollers.

It would still need to be called twice since is is called for the individual scroll position of each axis. If it returns two offsets, the vertical scroll position cannot do anything about the horizontal offset that would be returned if we returned both.

goderbauer · 2023-06-14T17:48:19Z

This means default focus traversal will work as well out of the box! :)

I forget, do these use the same APIs as implicit a11y scrolling? Or did we wire that up differently?

Piinks · 2023-06-15T15:38:26Z

I am going to mark this as a draft while I make some changes. I thought I would add showInViewport and showOnScreen to the 2DViewport as separate changes, but I think they are more tightly coupled than I originally thought and should probably be included here.

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

packages/flutter/lib/src/rendering/viewport.dart

packages/flutter/lib/src/widgets/scrollable.dart

goderbauer · 2023-07-17T18:00:23Z

packages/flutter/lib/src/widgets/scrollable.dart

+    throw FlutterError(
+      'The _performEnsureVisible method was called for the vertical scrollable '
+      'of a TwoDimensionalScrollable. This should not happen as the horizontal '
+      'scrollable handles both axes.'
+    );


Should this be in an assert? Or is it important that we can throw this error in release mode as well?

packages/flutter/lib/src/widgets/scrollable.dart

goderbauer · 2023-07-17T18:18:45Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+  ///
+  /// * [RenderObject.showOnScreen], overridden by
+  ///   [RenderTwoDimensionalViewport] to delegate to this method.
+  static Rect? showInViewportForAxisDirection({


This likely relates somehow to RenderViewportBase.showInViewport. Should the methods link to each other explaining the difference?

Also, what's the behavior if you call RenderViewportBase.showInViewport for a 2D scroller? AFICT, there's nothing stopping you from doing that?

goderbauer · 2023-07-17T18:23:15Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+    }
+
+    offset.moveTo(targetOffset.offset, duration: duration, curve: curve);
+    return targetOffset.rect;


Looks like the implementation is exactly the same as RenderViewportBase.showInViewport - except that we pass axisDirection in and to getOffsetToReveal. Can we either just add that functionality to RenderViewportBase.showInViewport or find other ways to share this not exactly trivial logic? The other method also has a more extensive implementation comment that would help in understanding what is happening here.

goderbauer · 2023-07-17T18:42:02Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

+        viewport: this,
+        offset: horizontalOffset,
+        axisDirection: horizontalAxisDirection,
+        rect: newRect,


My brain broke thinking about how newRect is used here. Technically, newRect here isn't the area of descendant that is to be made visible because newRect has already been adjusted for the vertical scroll above. I think, for some cases it still ends up working, because we know (or assume) that the first call to showInViewportForAxisDirection only adjusts one dimension of the rect and this call only cares about the other un-adjusted dimension. However, if we have any arbitrary transforms between descendant and this we'd run into trouble. One case I can see here is that the transform from line 1133 is essentially applied twice to the rect (once for the showInViewportForAxisDirection in line 1030 and then again for the call in 1042).

packages/flutter/lib/src/widgets/scrollable.dart

goderbauer · 2023-07-17T18:55:31Z

packages/flutter/lib/src/widgets/two_dimensional_viewport.dart

What does the render tree look like for that scenario?

Piinks · 2023-08-02T20:21:01Z

Note to self when I get back to this starting net week: https://github.com/flutter/flutter/pull/131641/files#r1280947388

Piinks · 2023-08-15T19:14:03Z

Closing this for now to take out of the review queue, I still need to rework a few things.

Piinks added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Jun 13, 2023

Piinks requested a review from goderbauer June 13, 2023 18:01

Piinks mentioned this pull request Jun 13, 2023

Remove fake viewport from dashboard flutter/cocoon#2838

Merged

8 tasks

Piinks force-pushed the ensureVisible2D branch from 37048a3 to 33c4de8 Compare June 13, 2023 22:10

Piinks mentioned this pull request Jun 13, 2023

Update cocoon version for flutter/tests flutter/tests#271

Merged

8 tasks

Piinks force-pushed the ensureVisible2D branch from 33c4de8 to 2b5a2b2 Compare June 14, 2023 02:34

goderbauer reviewed Jun 14, 2023

View reviewed changes

Piinks marked this pull request as draft June 15, 2023 15:38

Piinks added 8 commits July 12, 2023 14:23

Support ensureVisible for 2D viewport

ef8b682

Self review

100d019

Analyzer therapy

7b7ffab

Support ensureVisible for 2D viewport

7d9c5eb

Self review

7cae48b

Analyzer therapy

575eb0d

Updates

7483964

Merge branch 'master' of github.com:flutter/flutter into ensureVisible2D

6344f71

Piinks force-pushed the ensureVisible2D branch from 1db6757 to 6344f71 Compare July 13, 2023 19:38

Piinks commented Jul 13, 2023

View reviewed changes

packages/flutter/lib/src/rendering/list_wheel_viewport.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/rendering/viewport.dart Outdated Show resolved Hide resolved

Self review

d56be03

Piinks changed the title ~~Support ensureVisible for 2D viewport~~ Support ensureVisible/showOnScreen/showInViewport for 2D viewport Jul 13, 2023

Piinks requested a review from goderbauer July 13, 2023 20:19

Piinks marked this pull request as ready for review July 13, 2023 20:19

goderbauer reviewed Jul 17, 2023

View reviewed changes

Piinks mentioned this pull request Aug 2, 2023

Keep alive support for 2D scrolling #131641

Merged

8 tasks

Piinks closed this Aug 15, 2023

Piinks mentioned this pull request Sep 20, 2023

Support ensureVisible/showOnScreen/showInViewport for 2D Scrolling #135182

Merged

8 tasks

Piinks deleted the ensureVisible2D branch March 14, 2025 17:13

Support ensureVisible/showOnScreen/showInViewport for 2D viewport #128812

Support ensureVisible/showOnScreen/showInViewport for 2D viewport #128812

Uh oh!

Conversation

Piinks commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Piinks commented Jun 13, 2023

Uh oh!

Piinks commented Jun 13, 2023

Uh oh!

Piinks commented Jun 13, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Jun 14, 2023

Uh oh!

Piinks commented Jun 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks commented Aug 2, 2023

Uh oh!

Piinks commented Aug 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Piinks commented Jun 13, 2023 •

edited

Loading