Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@auto-submit
Copy link
Contributor

@auto-submit auto-submit bot commented Nov 26, 2024

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

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

  1. Nested scroll views (have not tried to verify)

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

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

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

  • 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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@auto-submit auto-submit bot requested a review from cbracken as a code owner November 26, 2024 00:38
@auto-submit auto-submit bot added the revert of Bot Only: Tracking label for bot. Tracks new revert of pull requests. label Nov 26, 2024
@auto-submit auto-submit bot requested a review from chinmaygarde as a code owner November 26, 2024 00:38
@auto-submit auto-submit bot merged commit 50174be into main Nov 26, 2024
7 checks passed
@auto-submit auto-submit bot deleted the revert_7a8c1e8031fff0cc05776c54e08712a580fe0699 branch November 26, 2024 00:39
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 26, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 26, 2024
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 26, 2024
…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
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 26, 2024
…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]>
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 26, 2024
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 26, 2024
…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
LongCatIsLooong added a commit that referenced this pull request Nov 27, 2024
auto-submit bot pushed a commit that referenced this pull request Dec 2, 2024
Reverts #56802

flutter/flutter#159517 should address the engine roll failure.

I'm not planning to land this until the coming Monday.
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
…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
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
…r/engine#56842)

Reverts flutter/engine#56802

flutter#159517 should address the engine roll failure.

I'm not planning to land this until the coming Monday.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-android platform-fuchsia platform-ios platform-web Code specifically for the web engine revert of Bot Only: Tracking label for bot. Tracks new revert of pull requests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants