-
Notifications
You must be signed in to change notification settings - Fork 6k
[iOS] Full keyboard access scrolling #56606
Conversation
49a8d81
to
bc501a2
Compare
@@ -174,7 +176,9 @@ - (void)accessibilityBridgeDidFinishUpdate { | |||
// contentOffset is 0.0, only the scroll down action is available. | |||
self.scrollView.frame = self.accessibilityFrame; | |||
self.scrollView.contentSize = [self contentSizeInternal]; | |||
[self.scrollView setContentOffset:[self contentOffsetInternal] animated:NO]; | |||
if (!self.scrollView.isDoingSystemScrolling) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means if the scroll view is in a FKA scrolling animation, all framework updates will be disregarded.
This is not optimal, if the user wants to interrupt the scrolling animation via swipe gestures, the UIScrollView
will be temporarily out of sync with the framework scroll offset, but once the animation is done, this UIScrollView
will send one last offset update to the framework, so it's guaranteed that the framework semantics tree will need another update and the new tree will also update the accessibility bridge and the offsets maintained by the scroll view and the framework will become consistent again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried removing this flag, and the offsets echoed back from the framework seem always to be one step behind and I am not sure exactly why:
It seems Shell::OnPlatformViewDispatchSemanticsAction
and Shell::OnEngineUpdateSemantics
both run tasks using RunNowOrPostTask
. Now that the UI thread is the same as the platform thread, there should be no delay, I was expecting:
<<<FKA setContentOffset: {0, 13.333333333333334}
<<<FKA setContentOffset: {0, 13.333333333333334}
>>>framework setContentOffset: {0, 13.333333333333343}
<<<FKA setContentOffset: {0, 15}
<<<FKA setContentOffset: {0, 15}
>>>framework setContentOffset: {0, 15}
but instead it's always delayed:
<<<FKA setContentOffset: {0, 13.333333333333334}
<<<FKA setContentOffset: {0, 13.333333333333334}
// ... framework sending back the previous outdated value
<<<FKA setContentOffset: {0, 15}
<<<FKA setContentOffset: {0, 15}
>>>framework setContentOffset: {0, 13.333333333333334}
full log
flutter: ▄▄▄▄▄▄▄▄ Frame 19 1m 36s 583.332ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 0.66666666666666663}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 0.7), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 3}
flutter: ▄▄▄▄▄▄▄▄ Frame 20 1m 38s 933.334ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 3}
>>>framework setContentOffset: {0, 0.66666666666666663}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 3.0), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 3.0), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 5}
flutter: ▄▄▄▄▄▄▄▄ Frame 21 1m 38s 949.999ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 5}
>>>framework setContentOffset: {0, 3}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 5.0), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 5.0), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 6.666666666666667}
flutter: ▄▄▄▄▄▄▄▄ Frame 22 1m 38s 966.665ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 6.666666666666667}
>>>framework setContentOffset: {0, 5}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 6.7), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 6.7), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 8.3333333333333339}
flutter: ▄▄▄▄▄▄▄▄ Frame 23 1m 38s 983.332ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 8.3333333333333339}
>>>framework setContentOffset: {0, 6.666666666666667}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 8.3), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 8.3), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 9.6666666666666661}
flutter: ▄▄▄▄▄▄▄▄ Frame 24 1m 38s 999.999ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 9.6666666666666661}
>>>framework setContentOffset: {0, 8.3333333333333339}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 9.7), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 9.7), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 11}
flutter: ▄▄▄▄▄▄▄▄ Frame 25 1m 39s 16.665ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 11}
>>>framework setContentOffset: {0, 9.6666666666666661}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 11.0), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 11.0), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 12.333333333333334}
flutter: ▄▄▄▄▄▄▄▄ Frame 26 1m 39s 33.333ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 12.333333333333334}
>>>framework setContentOffset: {0, 11}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 12.3), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 12.3), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 13.333333333333334}
flutter: ▄▄▄▄▄▄▄▄ Frame 27 1m 39s 49.999ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 13.333333333333334}
>>>framework setContentOffset: {0, 12.333333333333334}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 13.3), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 13.3), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 14.333333333333334}
flutter: ▄▄▄▄▄▄▄▄ Frame 28 1m 39s 66.666ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 14.333333333333334}
>>>framework setContentOffset: {0, 13.333333333333334}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 14.3), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 14.3), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 15.333333333333334}
flutter: ▄▄▄▄▄▄▄▄ Frame 29 1m 39s 83.333ms ▄▄▄▄▄▄▄▄
<<<FKA setContentOffset: {0, 15.333333333333334}
>>>framework setContentOffset: {0, 14.333333333333334}
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 15.3), SchedulerPhase.idle
flutter: _RenderScrollSemantics#0a6f4 relayoutBoundary=up1 => Offset(0.0, 15.3), SchedulerPhase.idle
<<<FKA setContentOffset: {0, 16}
flutter: ▄▄▄▄▄▄▄▄ Frame 30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i think this makes sense since FKA set content will change the scrollcontroller, but it will have to wait for one frame to get the layout and semantics update back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment covering this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like I was looking at an older checkout, and it was changed here. The event ordering becomes the way I expected it to be if we run DispatchSemanticsAction
synchronously like before. Jonah said RunNowOrPostTask
semantics action dispatch is fine we'll just need to post an empty task afterwards to flush the dart futures. I think if we do that instead we could eliminate this flag.
That seems to be the better option because we don't need the flag and the scroll view will be responsive to user gestures during the FKA scroll animation, but I think I'll make the attempt a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious to see if it will work, but I think even if the action is send to framework synchronously, it may still take one frame for flutter to rebuild layout and flush semantics back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious to see if it will work, but I think even if the action is send to framework synchronously, it may still take one frame for flutter to rebuild layout and flush semantics back
That's fine I think. The problem with DispatchSemanticsAction
being async is that we are essentially maintaining two sources of truth so it becomes a distributed system where the communication channels only guarantee FIFO ordering. If the UIScrollView here and the framework semantics tree having different opinion about what the content offset should be, the update takes time to travel and the inconsistency can be observed.
If the communication is synchronous then the inconsistent state won't be observable as the updates would arrive instantaneously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android code LGTM
/// This flag is set by the `FlutterSemanticsScrollView` itself, typically in | ||
/// one of the `UIScrollViewDelegate` methods. | ||
/// | ||
/// When this flag is true, the Flutter framework should typically cease |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have code from the dart side to enforce this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the update will be dropped. I will probably say that in the here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag is only read by the engine code and the framework side does not know about this. Yeah the wording is confusing I'll change it to "When this flag is true, the SemanticsObject ignores all incoming framework updates"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(updated the comment)
shell/platform/darwin/ios/framework/Source/SemanticsObject+UIFocusSystem.mm
Show resolved
Hide resolved
@@ -48,6 +55,9 @@ - (void)didUpdateFocusInContext:(UIFocusUpdateContext*)context | |||
} | |||
|
|||
- (id<UIFocusEnvironment>)parentFocusEnvironment { | |||
if ([self.parent isKindOfClass:[FlutterScrollableSemanticsObject class]]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this need to be recursively up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we recursively looks up in frame calculation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return the parent focus container, or nil
if none. Since [FlutterScrollableSemanticsObject parentFocusEnvironment]
returns its parent, and FlutterSemanticsScrollView
is a UIScrollView
, this looks right to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by recursively up? Returning the root node? This method is supposed to return the parent object in the focus hierarchy not the root (this method is part of the UIFocusEnvironment
protocol and the iOS focus engine will use this method to traverse the focus hierarchy).
shell/platform/darwin/ios/framework/Source/SemanticsObject+UIFocusSystem.mm
Outdated
Show resolved
Hide resolved
@@ -87,16 +145,99 @@ - (CGRect)frame { | |||
// | |||
// This method is only supposed to return items within the given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a note that I have run into weird bug before because i returned child something outside of the rect of accessibilityContainer. I am not sure if it applies here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can try adding culling here but I'll have to test it separately (and it seems to be working fine w/o the culling).
// to a Float64List in dart. | ||
// The size of a CGFloat is architecture-dependent and it's typically the word | ||
// size. The last iOS with 32-bit support was iOS 10. | ||
static_assert(sizeof(CGPoint) == sizeof(CGFloat) * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make it a soft-crash or short circuit if this condition does not hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a compile-time check. We typically deliver the engine as a prebuilt binary so it's not going to a concern for most users / devs. This assert is mostly for code readers to see the assumptions we made about CGFloat here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels pretty naughty. Even just for the sake of readability, I'd be tempted to create a double[]
, copy the values in, then encode and ship that. The extra CPU work is negligible and the one line of code will save you five lines of comment. Though I'd be tempted to still have a one line comment stating that you're encoding a Float64List
for Dart.
@@ -174,7 +176,9 @@ - (void)accessibilityBridgeDidFinishUpdate { | |||
// contentOffset is 0.0, only the scroll down action is available. | |||
self.scrollView.frame = self.accessibilityFrame; | |||
self.scrollView.contentSize = [self contentSizeInternal]; | |||
[self.scrollView setContentOffset:[self contentOffsetInternal] animated:NO]; | |||
if (!self.scrollView.isDoingSystemScrolling) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i think this makes sense since FKA set content will change the scrollcontroller, but it will have to wait for one frame to get the layout and semantics update back
lib/ui/semantics.dart
Outdated
@@ -86,6 +87,17 @@ class SemanticsAction { | |||
/// scrollable. | |||
static const SemanticsAction scrollDown = SemanticsAction._(_kScrollDownIndex, 'scrollDown'); | |||
|
|||
/// A request to scroll the scrollable container to a given scroll offset. | |||
/// | |||
/// The payload of this [SemanticsAction] is a flutter-stanard-encoded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The payload of this [SemanticsAction] is a flutter-stanard-encoded | |
/// The payload of this [SemanticsAction] is a flutter-standard-encoded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
- LGTM + codeowners approval for embedder.h change.
- Some comments and nits on the iOS side. Shout when you want a second pass!
@@ -48,6 +55,9 @@ - (void)didUpdateFocusInContext:(UIFocusUpdateContext*)context | |||
} | |||
|
|||
- (id<UIFocusEnvironment>)parentFocusEnvironment { | |||
if ([self.parent isKindOfClass:[FlutterScrollableSemanticsObject class]]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return the parent focus container, or nil
if none. Since [FlutterScrollableSemanticsObject parentFocusEnvironment]
returns its parent, and FlutterSemanticsScrollView
is a UIScrollView
, this looks right to me.
shell/platform/darwin/ios/framework/Source/SemanticsObject+UIFocusSystem.mm
Show resolved
Hide resolved
// to a Float64List in dart. | ||
// The size of a CGFloat is architecture-dependent and it's typically the word | ||
// size. The last iOS with 32-bit support was iOS 10. | ||
static_assert(sizeof(CGPoint) == sizeof(CGFloat) * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels pretty naughty. Even just for the sake of readability, I'd be tempted to create a double[]
, copy the values in, then encode and ship that. The extra CPU work is negligible and the one line of code will save you five lines of comment. Though I'd be tempted to still have a one line comment stating that you're encoding a Float64List
for Dart.
@@ -174,7 +176,9 @@ - (void)accessibilityBridgeDidFinishUpdate { | |||
// contentOffset is 0.0, only the scroll down action is available. | |||
self.scrollView.frame = self.accessibilityFrame; | |||
self.scrollView.contentSize = [self contentSizeInternal]; | |||
[self.scrollView setContentOffset:[self contentOffsetInternal] animated:NO]; | |||
if (!self.scrollView.isDoingSystemScrolling) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment covering this here.
shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/SemanticsObjectTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/SemanticsObject+UIFocusSystem.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/SemanticsObject+UIFocusSystem.mm
Show resolved
Hide resolved
[reversedItems addObject:((FlutterScrollableSemanticsObject*)child).scrollView]; | ||
} else { | ||
[reversedItems addObject:child]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting something a static like this for readability:
static id<UIFocusItem>* GetFocusItemForNode(SemanticsObject* node) {
if ([node isKindOfClass:[FlutterScrollableSemanticsObject class]]) {
return ((FlutterScrollableSemanticsObject*)node).scrollView;
}
return node;
}
That simplifies this code just doing one thing (reversing the order of the focus nodes), rather than two at the same time (reversing the order + determining the focus node).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this an instance method to avoid isKindOfClass:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better!
@@ -17,6 +18,11 @@ | |||
const float kFloatCompareEpsilon = 0.001; | |||
|
|||
@interface SemanticsObject (UIFocusSystem) <UIFocusItem, UIFocusItemContainer> | |||
@property(nonatomic) FlutterSemanticsScrollView* scrollView; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're re-declaring this property in three places now.
- On
SemanticsObject
inSemanticsObject.mm
for internal use in the class. - On the category on
FlutterScrollableSemanticsObject
, which is a subclass of SemanticsObject. - Here, for testing.
I think it's safe for us to push the property declaration into SemanticsObject.h
and remove it in the other places.
Reason for revert: flutter/flutter#159456 |
This reverts commit 7a8c1e8.
Reverts: #56606 Initiated by: LongCatIsLooong Reason for reverting: flutter/flutter#159456 Original PR Author: LongCatIsLooong Reviewed By: {chunhtai, cbracken} This change reverts the following previous change: This PR adds basic FKA scrolling support: when the iOS focus (the focus state is maintained separately from the framework focus, see the previous PR) switches to an item in a scrollable container that is too close to the edge of the viewport, the container will scroll to make sure the next item is visible. Previous PR for context: #55964 https://github.com/user-attachments/assets/84ae5153-f955-4d23-9901-ce942c0e98ac ### Why the UIScrollView subclass in the focus hierarchy The iOS focus system does not provide an API that allows apps to notify it of focus highlight changes. So if we were to keep using the transforms sent by the framework as-is and not introducing any UIViews in the focus hierarchy, the focus highlight will be positioned at the wrong location after scrolling (via FKA or via framework). That does not seem to be part of the public API and the focus system seems to only know how to properly highlight focusable UIViews. ### Things that currently may not work 1. Nested scroll views (have not tried to verify) The `UIScrollView`s are always subviews of the `FlutterView`. If there are nested scrollables the focus system may not be able to properly determine the focus hierarchy (in theory the iOS focus system should never depend on `UIView.parentView` but I haven't tried to verify that). 2. If the next item is too far below the bottom of the screen and there is a tab bar with focusable items, the focus will be transferred to tab bar instead of the next item in the list Video demo (as you can see the scrolling is really finicky): https://github.com/user-attachments/assets/51c2bfe4-d7b3-4614-aa49-4256214f8978 I've tried doing the same thing using a `UITableView` with similar configurations but it seems to have the same problem. I'll try to dig a bit deeper into this and see if there's a workaround. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
…159461) flutter/engine@fe45a66...fb64399 2024-11-26 [email protected] iOS: Migrate PlatformViewsController to Objective-C (flutter/engine#56790) 2024-11-26 [email protected] Started caching HandleGLES's hash and made them immutable (flutter/engine#56800) 2024-11-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[iOS] Full keyboard access scrolling (#56606)" (flutter/engine#56802) 2024-11-26 [email protected] Roll Skia from 12c8bd6ac1d9 to c1c8ff84997c (14 revisions) (flutter/engine#56801) 2024-11-25 [email protected] [Impeller] better handle allocation herustics of Android slide in page transition. (flutter/engine#56762) 2024-11-25 [email protected] iOS: Eliminate logging of non-zero origin platformviews (flutter/engine#56796) 2024-11-25 [email protected] [impeller] gles: started storing the number of handle deletions to avoid reallocation (flutter/engine#56799) 2024-11-25 [email protected] Roll Fuchsia Linux SDK from 9o0fWa2xVhmxV6Mtn... to 50xtjbMWWrqay_7m_... (flutter/engine#56795) 2024-11-25 [email protected] Roll Dart SDK from df716eaa6ed2 to 4b49546a1dfa (1 revision) (flutter/engine#56793) 2024-11-25 [email protected] [iOS] Full keyboard access scrolling (flutter/engine#56606) 2024-11-25 [email protected] [android] remove fml_check from surface_texture_external_texture (flutter/engine#56760) 2024-11-25 [email protected] removed unused variable for skia initialization (flutter/engine#56791) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 9o0fWa2xVhmx to 50xtjbMWWrqa If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
…visions) (#159461)" (#159498) <!-- start_original_pr_link --> Reverts: #159461 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: yjbanov <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: engine regression #159497 <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: engine-flutter-autoroll <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {fluttergithubbot} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: flutter/engine@fe45a66...fb64399 2024-11-26 [email protected] iOS: Migrate PlatformViewsController to Objective-C (flutter/engine#56790) 2024-11-26 [email protected] Started caching HandleGLES's hash and made them immutable (flutter/engine#56800) 2024-11-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[iOS] Full keyboard access scrolling (#56606)" (flutter/engine#56802) 2024-11-26 [email protected] Roll Skia from 12c8bd6ac1d9 to c1c8ff84997c (14 revisions) (flutter/engine#56801) 2024-11-25 [email protected] [Impeller] better handle allocation herustics of Android slide in page transition. (flutter/engine#56762) 2024-11-25 [email protected] iOS: Eliminate logging of non-zero origin platformviews (flutter/engine#56796) 2024-11-25 [email protected] [impeller] gles: started storing the number of handle deletions to avoid reallocation (flutter/engine#56799) 2024-11-25 [email protected] Roll Fuchsia Linux SDK from 9o0fWa2xVhmxV6Mtn... to 50xtjbMWWrqay_7m_... (flutter/engine#56795) 2024-11-25 [email protected] Roll Dart SDK from df716eaa6ed2 to 4b49546a1dfa (1 revision) (flutter/engine#56793) 2024-11-25 [email protected] [iOS] Full keyboard access scrolling (flutter/engine#56606) 2024-11-25 [email protected] [android] remove fml_check from surface_texture_external_texture (flutter/engine#56760) 2024-11-25 [email protected] removed unused variable for skia initialization (flutter/engine#56791) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 9o0fWa2xVhmx to 50xtjbMWWrqa If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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 <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <[email protected]>
…159504) flutter/engine@fe45a66...bd165dc 2024-11-26 [email protected] Revert "iOS: Migrate PlatformViewsController to Objective-C (#56790)" (flutter/engine#56817) 2024-11-26 [email protected] iOS: Rename FlutterPlatformViews_Internal.mm (flutter/engine#56816) 2024-11-26 [email protected] iOS: Eliminate global in platformviews controller (flutter/engine#56805) 2024-11-26 [email protected] Roll Skia from a276978ba7c8 to f149f852c70a (1 revision) (flutter/engine#56812) 2024-11-26 [email protected] Roll Skia from d7a267d88fd6 to a276978ba7c8 (1 revision) (flutter/engine#56811) 2024-11-26 [email protected] Roll Skia from 8d9d892657a7 to d7a267d88fd6 (1 revision) (flutter/engine#56810) 2024-11-26 [email protected] Roll Dart SDK from bdb76c714009 to ca02d403f1a8 (1 revision) (flutter/engine#56809) 2024-11-26 [email protected] Roll Skia from b697dd1b03b2 to 8d9d892657a7 (1 revision) (flutter/engine#56808) 2024-11-26 [email protected] Roll Skia from c1c8ff84997c to b697dd1b03b2 (1 revision) (flutter/engine#56807) 2024-11-26 [email protected] iOS: Delete FlutterPlatformViewsController.layerPoolSize (flutter/engine#56806) 2024-11-26 [email protected] Roll Dart SDK from 4b49546a1dfa to bdb76c714009 (1 revision) (flutter/engine#56803) 2024-11-26 [email protected] iOS: Migrate PlatformViewsController to Objective-C (flutter/engine#56790) 2024-11-26 [email protected] Started caching HandleGLES's hash and made them immutable (flutter/engine#56800) 2024-11-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[iOS] Full keyboard access scrolling (#56606)" (flutter/engine#56802) 2024-11-26 [email protected] Roll Skia from 12c8bd6ac1d9 to c1c8ff84997c (14 revisions) (flutter/engine#56801) 2024-11-25 [email protected] [Impeller] better handle allocation herustics of Android slide in page transition. (flutter/engine#56762) 2024-11-25 [email protected] iOS: Eliminate logging of non-zero origin platformviews (flutter/engine#56796) 2024-11-25 [email protected] [impeller] gles: started storing the number of handle deletions to avoid reallocation (flutter/engine#56799) 2024-11-25 [email protected] Roll Fuchsia Linux SDK from 9o0fWa2xVhmxV6Mtn... to 50xtjbMWWrqay_7m_... (flutter/engine#56795) 2024-11-25 [email protected] Roll Dart SDK from df716eaa6ed2 to 4b49546a1dfa (1 revision) (flutter/engine#56793) 2024-11-25 [email protected] [iOS] Full keyboard access scrolling (flutter/engine#56606) 2024-11-25 [email protected] [android] remove fml_check from surface_texture_external_texture (flutter/engine#56760) 2024-11-25 [email protected] removed unused variable for skia initialization (flutter/engine#56791) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 9o0fWa2xVhmx to 50xtjbMWWrqa If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
Reverts #56802 flutter/flutter#159517 should address the engine roll failure. I'm not planning to land this until the coming Monday.
This PR adds basic FKA scrolling support: when the iOS focus (the focus state is maintained separately from the framework focus, see the previous PR) switches to an item in a scrollable container that is too close to the edge of the viewport, the container will scroll to make sure the next item is visible. Previous PR for context: flutter/engine#55964 https://github.com/user-attachments/assets/84ae5153-f955-4d23-9901-ce942c0e98ac ### Why the UIScrollView subclass in the focus hierarchy The iOS focus system does not provide an API that allows apps to notify it of focus highlight changes. So if we were to keep using the transforms sent by the framework as-is and not introducing any UIViews in the focus hierarchy, the focus highlight will be positioned at the wrong location after scrolling (via FKA or via framework). That does not seem to be part of the public API and the focus system seems to only know how to properly highlight focusable UIViews. ### Things that currently may not work 1. Nested scroll views (have not tried to verify) The `UIScrollView`s are always subviews of the `FlutterView`. If there are nested scrollables the focus system may not be able to properly determine the focus hierarchy (in theory the iOS focus system should never depend on `UIView.parentView` but I haven't tried to verify that). 2. If the next item is too far below the bottom of the screen and there is a tab bar with focusable items, the focus will be transferred to tab bar instead of the next item in the list Video demo (as you can see the scrolling is really finicky): https://github.com/user-attachments/assets/51c2bfe4-d7b3-4614-aa49-4256214f8978 I've tried doing the same thing using a `UITableView` with similar configurations but it seems to have the same problem. I'll try to dig a bit deeper into this and see if there's a workaround. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
…er/engine#56802) Reverts: flutter/engine#56606 Initiated by: LongCatIsLooong Reason for reverting: flutter#159456 Original PR Author: LongCatIsLooong Reviewed By: {chunhtai, cbracken} This change reverts the following previous change: This PR adds basic FKA scrolling support: when the iOS focus (the focus state is maintained separately from the framework focus, see the previous PR) switches to an item in a scrollable container that is too close to the edge of the viewport, the container will scroll to make sure the next item is visible. Previous PR for context: flutter/engine#55964 https://github.com/user-attachments/assets/84ae5153-f955-4d23-9901-ce942c0e98ac ### Why the UIScrollView subclass in the focus hierarchy The iOS focus system does not provide an API that allows apps to notify it of focus highlight changes. So if we were to keep using the transforms sent by the framework as-is and not introducing any UIViews in the focus hierarchy, the focus highlight will be positioned at the wrong location after scrolling (via FKA or via framework). That does not seem to be part of the public API and the focus system seems to only know how to properly highlight focusable UIViews. ### Things that currently may not work 1. Nested scroll views (have not tried to verify) The `UIScrollView`s are always subviews of the `FlutterView`. If there are nested scrollables the focus system may not be able to properly determine the focus hierarchy (in theory the iOS focus system should never depend on `UIView.parentView` but I haven't tried to verify that). 2. If the next item is too far below the bottom of the screen and there is a tab bar with focusable items, the focus will be transferred to tab bar instead of the next item in the list Video demo (as you can see the scrolling is really finicky): https://github.com/user-attachments/assets/51c2bfe4-d7b3-4614-aa49-4256214f8978 I've tried doing the same thing using a `UITableView` with similar configurations but it seems to have the same problem. I'll try to dig a bit deeper into this and see if there's a workaround. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
This PR adds basic FKA scrolling support: when the iOS focus (the focus state is maintained separately from the framework focus, see the previous PR) switches to an item in a scrollable container that is too close to the edge of the viewport, the container will scroll to make sure the next item is visible.
Previous PR for context: #55964
Screen.Recording.2024-11-18.at.1.48.19.PM.mov
Why the UIScrollView subclass in the focus hierarchy
The iOS focus system does not provide an API that allows apps to notify it of focus highlight changes. So if we were to keep using the transforms sent by the framework as-is and not introducing any UIViews in the focus hierarchy, the focus highlight will be positioned at the wrong location after scrolling (via FKA or via framework). That does not seem to be part of the public API and the focus system seems to only know how to properly highlight focusable UIViews.
Things that currently may not work
The
UIScrollView
s are always subviews of theFlutterView
. If there are nested scrollables the focus system may not be able to properly determine the focus hierarchy (in theory the iOS focus system should never depend onUIView.parentView
but I haven't tried to verify that).Video demo (as you can see the scrolling is really finicky):
Screen.Recording.2024-11-18.at.2.09.09.PM.mov
I've tried doing the same thing using a
UITableView
with similar configurations but it seems to have the same problem. I'll try to dig a bit deeper into this and see if there's a workaround.Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.