Skip to content

Cache FocusNode.enclosingScope, clean up descendantsAreFocusable#144207

Merged
auto-submit[bot] merged 1 commit intoflutter:masterfrom
LongCatIsLooong:focus-cleanup
Feb 27, 2024
Merged

Cache FocusNode.enclosingScope, clean up descendantsAreFocusable#144207
auto-submit[bot] merged 1 commit intoflutter:masterfrom
LongCatIsLooong:focus-cleanup

Conversation

@LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Feb 27, 2024

FocusNode.canRequestFocus was doing a double traversal if no ancestor disallows focus. The last for loop only has to reach as far as the enclosing scope.

Also this caches the FocusNode.enclosingScope since the getter access happens much more frequently than node reparenting.

Pre-launch Checklist

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: focus Focus traversal, gaining or losing focus labels Feb 27, 2024
gspencergoog
gspencergoog approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Excellent! Thanks for doing this.

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 27, 2024
@auto-submit auto-submit bot merged commit c353cb0 into flutter:master Feb 27, 2024
@LongCatIsLooong LongCatIsLooong deleted the focus-cleanup branch February 27, 2024 21:48
@CaseyHillers CaseyHillers added the revert Autorevert PR (with "Reason for revert:" comment) label Feb 28, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Feb 28, 2024

A reason for requesting a revert of flutter/flutter/144207 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Feb 28, 2024
@CaseyHillers
Copy link
Contributor

CaseyHillers commented Feb 28, 2024

Reason for revert: b/327301206 - Breaking a customer test

@CaseyHillers CaseyHillers added the revert Autorevert PR (with "Reason for revert:" comment) label Feb 28, 2024
auto-submit bot pushed a commit that referenced this pull request Feb 28, 2024
Revert "Cache FocusNode.enclosingScope, clean up `descendantsAreFoc…
931d699 
…usable` (#144207)"

This reverts commit c353cb0.
@auto-submit auto-submit bot mentioned this pull request Feb 28, 2024
Merged
9 tasks
@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Feb 28, 2024
auto-submit bot added a commit that referenced this pull request Feb 28, 2024
@auto-submit
Reverts "Cache FocusNode.enclosingScope, clean up `descendantsAreFo…
4505068 
…cusable` (#144207)" (#144292)

Reverts #144207

Initiated by: CaseyHillers

Reason for reverting: b/327301206 - Breaking a customer test

Original PR Author: LongCatIsLooong

Reviewed By: {gspencergoog}

This change reverts the following previous change:
Original Description:
`FocusNode.canRequestFocus` was doing a double traversal if no ancestor disallows focus. The last for loop only has to reach as far as the enclosing scope.

Also this caches the `FocusNode.enclosingScope` since the getter access happens much more frequently than node reparenting.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 28, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2024
@engine-flutter-autoroll
c353cb0 Cache FocusNode.enclosingScope, clean up `descendantsAreFoc…
3684088 
…usable` (flutter/flutter#144207)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 28, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2024
@engine-flutter-autoroll
c353cb0 Cache FocusNode.enclosingScope, clean up `descendantsAreFoc…
f74f605 
…usable` (flutter/flutter#144207)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 28, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2024
@engine-flutter-autoroll
c353cb0 Cache FocusNode.enclosingScope, clean up `descendantsAreFoc…
ed1c2a1 
…usable` (flutter/flutter#144207)
LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Feb 28, 2024
@LongCatIsLooong
Reland "Cache FocusNode.enclosingScope, clean up descendantsAreFocusa…
3145811 
…ble (flutter#144207)"
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 28, 2024
Merged
@LongCatIsLooong LongCatIsLooong mentioned this pull request Feb 28, 2024
Merged
9 tasks
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2024
@engine-flutter-autoroll
c353cb0 Cache FocusNode.enclosingScope, clean up `descendantsAreFoc…
10d259f 
…usable` (flutter/flutter#144207)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Feb 28, 2024
@engine-flutter-autoroll
Manual roll Flutter from c30f998 to d00bfe8 (32 revisions) (#6222)
6d02f03 
Manual roll Flutter from c30f998 to d00bfe8 (32 revisions)

Manual roll requested by bmparr@google.com

flutter/flutter@c30f998...d00bfe8

2024-02-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from c9381fb8ef4c to 455c814fe5de (1 revision) (flutter/flutter#144340)
2024-02-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 91898e397261 to c9381fb8ef4c (11 revisions) (flutter/flutter#144338)
2024-02-28 15619084+vashworth@users.noreply.github.com Reland "Add FlutterMacOS.xcframework artifact (#143244)" (flutter/flutter#144275)
2024-02-28 christopherfujino@gmail.com [flutter_tools] Catch rpc error in render frame with raster stats (flutter/flutter#144190)
2024-02-28 github@alexv525.com �� Guard Flutter Android app by disallow task affinity by default (flutter/flutter#144018)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 8acc96d405d0 to 91898e397261 (flutter/flutter#144316)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 64a375de9c8f to 8acc96d405d0 (flutter/flutter#144296)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine c79117b706e9 to 64a375de9c8f (flutter/flutter#144293)
2024-02-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (#144207)" (flutter/flutter#144292)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 2461280c38b7 to c79117b706e9 (flutter/flutter#144290)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 5e0d9ba35dd5 to 2461280c38b7 (flutter/flutter#144288)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine fe7ea6d9c34f to 5e0d9ba35dd5 (flutter/flutter#144285)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 0bc21ea7bc92 to fe7ea6d9c34f (flutter/flutter#144283)
2024-02-28 polinach@google.com Use const route for notAnnounced. (flutter/flutter#144050)
2024-02-28 tessertaha@gmail.com Add `tabs_utils.dart` class (flutter/flutter#143937)
2024-02-28 36861262+QuncCccccc@users.noreply.github.com Remove `bottomAppBarColor` from `ThemeData` (flutter/flutter#144080)
2024-02-27 richadlu001@gmail.com fix: unexpected chinese punctuation (flutter/flutter#143678)
2024-02-27 42216813+eliasyishak@users.noreply.github.com Clean up lint ignores (flutter/flutter#144229)
2024-02-27 jhy03261997@gmail.com Reland [a11y] Add isEnabled semantics flag to  text field  (flutter/flutter#143601)
2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Remove deprecated `CupertinoContextMenu.previewBuilder` (flutter/flutter#143990)
2024-02-27 chris@bracken.jp [iOS] Fix naming in platform_view example (flutter/flutter#144247)
2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland - Introduce tone-based surfaces and accent color add-ons - Part 2 (#144001)" (flutter/flutter#144262)
2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add FlutterMacOS.xcframework artifact (#143244)" (flutter/flutter#144253)
2024-02-27 kustermann@google.com [web] Make flutter web profile builds always keep wasm symbols (flutter/flutter#144130)
2024-02-27 36861262+QuncCccccc@users.noreply.github.com Reland - Introduce tone-based surfaces and accent color add-ons - Part 2 (flutter/flutter#144001)
2024-02-27 tjrogertj@gmail.com bind missing add icon in platform_view example (flutter/flutter#132028)
2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (flutter/flutter#144207)
2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Remove strut migration flag from `TextPainter` (flutter/flutter#144242)
2024-02-27 15619084+vashworth@users.noreply.github.com Remove force Xcode debug workflow (flutter/flutter#144185)
2024-02-27 54558023+keyonghan@users.noreply.github.com Mark two other firebase targets as bringup: true (flutter/flutter#144234)
2024-02-27 15619084+vashworth@users.noreply.github.com Add FlutterMacOS.xcframework artifact (flutter/flutter#143244)
2024-02-27 jonahwilliams@google.com Re-enable Impeller goldens blocking.  (flutter/flutter#144210)

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 bmparr@google.com,rmistry@google.com,stuartmorgan@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

...
auto-submit bot pushed a commit that referenced this pull request Feb 29, 2024
@LongCatIsLooong
Reland "Cache FocusNode.enclosingScope, clean up descendantsAreFocusa…
dddbd04 
…ble (#144207)" (#144330)

The [internal test failure](#144207 (comment)) was caused by `Focus.withExternalFocusNode` modifying the external node's attributes. The extra changes are in this commit:  e53d98b

CL with (almost) passing TGP: cl/611157582
LouiseHsu pushed a commit to LouiseHsu/packages that referenced this pull request Mar 7, 2024
@engine-flutter-autoroll @LouiseHsu
Manual roll Flutter from c30f998 to d00bfe8 (32 revisions) (flutter#6222
ffa05c1 
)

Manual roll Flutter from c30f998 to d00bfe8 (32 revisions)

Manual roll requested by bmparr@google.com

flutter/flutter@c30f998...d00bfe8

2024-02-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from c9381fb8ef4c to 455c814fe5de (1 revision) (flutter/flutter#144340)
2024-02-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from 91898e397261 to c9381fb8ef4c (11 revisions) (flutter/flutter#144338)
2024-02-28 15619084+vashworth@users.noreply.github.com Reland "Add FlutterMacOS.xcframework artifact (#143244)" (flutter/flutter#144275)
2024-02-28 christopherfujino@gmail.com [flutter_tools] Catch rpc error in render frame with raster stats (flutter/flutter#144190)
2024-02-28 github@alexv525.com �� Guard Flutter Android app by disallow task affinity by default (flutter/flutter#144018)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 8acc96d405d0 to 91898e397261 (flutter/flutter#144316)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 64a375de9c8f to 8acc96d405d0 (flutter/flutter#144296)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine c79117b706e9 to 64a375de9c8f (flutter/flutter#144293)
2024-02-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (#144207)" (flutter/flutter#144292)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 2461280c38b7 to c79117b706e9 (flutter/flutter#144290)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 5e0d9ba35dd5 to 2461280c38b7 (flutter/flutter#144288)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine fe7ea6d9c34f to 5e0d9ba35dd5 (flutter/flutter#144285)
2024-02-28 zanderso@users.noreply.github.com Manual roll Flutter Engine 0bc21ea7bc92 to fe7ea6d9c34f (flutter/flutter#144283)
2024-02-28 polinach@google.com Use const route for notAnnounced. (flutter/flutter#144050)
2024-02-28 tessertaha@gmail.com Add `tabs_utils.dart` class (flutter/flutter#143937)
2024-02-28 36861262+QuncCccccc@users.noreply.github.com Remove `bottomAppBarColor` from `ThemeData` (flutter/flutter#144080)
2024-02-27 richadlu001@gmail.com fix: unexpected chinese punctuation (flutter/flutter#143678)
2024-02-27 42216813+eliasyishak@users.noreply.github.com Clean up lint ignores (flutter/flutter#144229)
2024-02-27 jhy03261997@gmail.com Reland [a11y] Add isEnabled semantics flag to  text field  (flutter/flutter#143601)
2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Remove deprecated `CupertinoContextMenu.previewBuilder` (flutter/flutter#143990)
2024-02-27 chris@bracken.jp [iOS] Fix naming in platform_view example (flutter/flutter#144247)
2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland - Introduce tone-based surfaces and accent color add-ons - Part 2 (#144001)" (flutter/flutter#144262)
2024-02-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add FlutterMacOS.xcframework artifact (#143244)" (flutter/flutter#144253)
2024-02-27 kustermann@google.com [web] Make flutter web profile builds always keep wasm symbols (flutter/flutter#144130)
2024-02-27 36861262+QuncCccccc@users.noreply.github.com Reland - Introduce tone-based surfaces and accent color add-ons - Part 2 (flutter/flutter#144001)
2024-02-27 tjrogertj@gmail.com bind missing add icon in platform_view example (flutter/flutter#132028)
2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (flutter/flutter#144207)
2024-02-27 31859944+LongCatIsLooong@users.noreply.github.com Remove strut migration flag from `TextPainter` (flutter/flutter#144242)
2024-02-27 15619084+vashworth@users.noreply.github.com Remove force Xcode debug workflow (flutter/flutter#144185)
2024-02-27 54558023+keyonghan@users.noreply.github.com Mark two other firebase targets as bringup: true (flutter/flutter#144234)
2024-02-27 15619084+vashworth@users.noreply.github.com Add FlutterMacOS.xcframework artifact (flutter/flutter#143244)
2024-02-27 jonahwilliams@google.com Re-enable Impeller goldens blocking.  (flutter/flutter#144210)

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 bmparr@google.com,rmistry@google.com,stuartmorgan@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

...
@b3nni97 b3nni97 mentioned this pull request Apr 16, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024
@engine-flutter-autoroll
c353cb0 Cache FocusNode.enclosingScope, clean up `descendantsAreFoc…
4cf432c 
…usable` (flutter/flutter#144207)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gspencergoog gspencergoog gspencergoog approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: focus Focus traversal, gaining or losing focus framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LongCatIsLooong @CaseyHillers @gspencergoog