Prevent material switch from recreating its render object when it becomes disabled by LongCatIsLooong · Pull Request #61398 · flutter/flutter

LongCatIsLooong · 2020-07-13T23:45:37Z

Description

FocusableActionDetector changes the shape of the tree so _SwitchRenderObjectWidget needs a GlobalKey to get reused. Preferred this GlobalKey solution over changing the implementation of FocusableActionDetector, because that latter would change the semantics of many other widgets.

Related Issues

fixes #61247

Tests

I added the following tests:

Material switch should not recreate its render object when disabled

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

gspencergoog · 2020-07-14T16:54:25Z

packages/flutter/lib/src/material/switch.dart

      child: Builder(
        builder: (BuildContext context) {
          return _SwitchRenderObjectWidget(
+            // FocusableActionDetector changes the shape of the tree when the


Should we instead fix FocusableActionDetector to not remove the Actions and Shortcuts widgets when disabled, and instead just empty out their maps? There is some performance cost to building them, but if they cause churn in the tree, that could be offset by not causing it anymore.

It would break some tests because that introduces a 1 frame delay to descendant Focus's semantics (isFocusable) . Currently since we change the shape of the tree, a new Focus widget gets inflated so there's no delay (line 1090):

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

Lines 1081 to 1103 in 4195ad4

@override

Widget build(BuildContext context) {

Widget child = MouseRegion(

onEnter: _handleMouseEnter,

onExit: _handleMouseExit,

cursor: widget.mouseCursor,

child: Focus(

focusNode: widget.focusNode,

autofocus: widget.autofocus,

canRequestFocus: _canRequestFocus,

onFocusChange: _handleFocusChange,

child: widget.child,

),

);

if (widget.enabled && widget.actions != null && widget.actions.isNotEmpty) {

child = Actions(actions: widget.actions, child: child);

}

if (widget.enabled && widget.shortcuts != null && widget.shortcuts.isNotEmpty) {

child = Shortcuts(shortcuts: widget.shortcuts, child: child);

}

return child;

}

}

(Is this expected?)

Well, I would think that anything that cares about the focus changing shouldn't care about the 1 frame delay, since focus changes already happen in a microtask, so that already happens often. And semantics shouldn't care about a 1 frame delay either.

I think I added that part because the focus changes were increasing a build benchmark, and I was trying to reduce the number of built items for things that were disabled.

If a switch is disabled then isFocusable should be removed from its semantics flags in the same frame, no?

The focus itself won't actually be removed until the microtask runs, so if the semantics removes it immediately, it might actually be early (and wrong until the task runs).

LongCatIsLooong · 2020-07-15T20:46:21Z

packages/flutter/test/material/radio_test.dart

+            SemanticsFlag.hasCheckedState,
+            SemanticsFlag.hasEnabledState,
+            SemanticsFlag.isInMutuallyExclusiveGroup,
+            SemanticsFlag.isFocusable,  // This flag is delayed by 1 frame.


Updated. But it still seems confusing to me that when the UI is disabled the semantics is telling us the component is focusable.

Yes, ideally it would be resolved immediately, but it causes too much of a performance hit to update focus while the tree is rebuilt, and would mean that sometimes (during the build) two things could have focus at the same time, or that nothing has focus, which complicates a lot of logic.

Would it be possible to update the focus tree after we finalize the layout (instead of in a post-frame callback)?

LongCatIsLooong · 2020-08-14T22:01:43Z

@gspencergoog updated the PR. Could you take another look?

gspencergoog · 2020-08-17T17:39:31Z

packages/flutter/test/material/radio_test.dart

+            SemanticsFlag.hasCheckedState,
+            SemanticsFlag.hasEnabledState,
+            SemanticsFlag.isInMutuallyExclusiveGroup,
+            SemanticsFlag.isFocusable,  // This flag is delayed by 1 frame.


Yes, ideally it would be resolved immediately, but it causes too much of a performance hit to update focus while the tree is rebuilt, and would mean that sometimes (during the build) two things could have focus at the same time, or that nothing has focus, which complicates a lot of logic.

…n it becomes disabled (#61398)" This reverts commit 579cab8.

…n it becomes disabled (#61398)" (#64062)

…n it becomes disabled (flutter#61398)", reverted in flutter#64062 This reverts commit 90908b0.

…n it becomes disabled (#61398)", reverted in #64062 (#64354)

…omes disabled (flutter#61398)

…n it becomes disabled (flutter#61398)" (flutter#64062)

…n it becomes disabled (flutter#61398)", reverted in flutter#64062 (flutter#64354)

…omes disabled (flutter#61398)

…n it becomes disabled (flutter#61398)" (flutter#64062)

…n it becomes disabled (flutter#61398)", reverted in flutter#64062 (flutter#64354)

use global key to maintain state

f15bee7

LongCatIsLooong requested a review from gspencergoog July 13, 2020 23:45

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 13, 2020

googlebot added the cla: yes label Jul 13, 2020

gspencergoog reviewed Jul 14, 2020

View reviewed changes

LongCatIsLooong added 2 commits July 15, 2020 13:24

keep tree shape

03f36cb

remove global key

f883f73

LongCatIsLooong commented Jul 15, 2020

View reviewed changes

gspencergoog approved these changes Aug 17, 2020

View reviewed changes

LongCatIsLooong added the waiting for tree to go green label Aug 17, 2020

fluttergithubbot merged commit 579cab8 into flutter:master Aug 17, 2020

LongCatIsLooong deleted the actions-keep-tree-shape branch August 18, 2020 17:38

LongCatIsLooong added a commit that referenced this pull request Aug 18, 2020

Revert "Prevent material switch from recreating its render object whe…

6ae823e

…n it becomes disabled (#61398)" This reverts commit 579cab8.

kf6gpe mentioned this pull request Aug 21, 2020

web_benchmarks_html build_material_checkbox.html.drawFrameDuration.average & build_material_checkbox.html.drawFrameDuration.average regressions #64332

Closed

LongCatIsLooong added a commit that referenced this pull request Aug 21, 2020

Revert "Prevent material switch from recreating its render object whe…

90908b0

…n it becomes disabled (#61398)" (#64062)

LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Aug 21, 2020

Reland "Prevent material switch from recreating its render object whe…

2196c28

…n it becomes disabled (flutter#61398)", reverted in flutter#64062 This reverts commit 90908b0.

LongCatIsLooong added a commit that referenced this pull request Aug 21, 2020

Reland "Prevent material switch from recreating its render object whe…

53e25a6

…n it becomes disabled (#61398)", reverted in #64062 (#64354)

smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020

Prevent material switch from recreating its render object when it bec…

45bd458

…omes disabled (flutter#61398)

smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020

Revert "Prevent material switch from recreating its render object whe…

b720a6c

…n it becomes disabled (flutter#61398)" (flutter#64062)

smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020

Reland "Prevent material switch from recreating its render object whe…

b84a3c2

…n it becomes disabled (flutter#61398)", reverted in flutter#64062 (flutter#64354)

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Prevent material switch from recreating its render object when it bec…

9e5d39a

…omes disabled (flutter#61398)

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Revert "Prevent material switch from recreating its render object whe…

fcb0ec0

…n it becomes disabled (flutter#61398)" (flutter#64062)

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Reland "Prevent material switch from recreating its render object whe…

71f9556

…n it becomes disabled (flutter#61398)", reverted in flutter#64062 (flutter#64354)

github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent material switch from recreating its render object when it becomes disabled#61398

Prevent material switch from recreating its render object when it becomes disabled#61398
fluttergithubbot merged 3 commits intoflutter:masterfrom
LongCatIsLooong:actions-keep-tree-shape

LongCatIsLooong commented Jul 13, 2020 •

edited

Loading

Uh oh!

gspencergoog Jul 14, 2020

Uh oh!

LongCatIsLooong Jul 14, 2020 •

edited

Loading

Uh oh!

LongCatIsLooong Jul 14, 2020

Uh oh!

gspencergoog Jul 14, 2020

Uh oh!

gspencergoog Jul 14, 2020

Uh oh!

LongCatIsLooong Jul 14, 2020

Uh oh!

gspencergoog Jul 14, 2020

Uh oh!

LongCatIsLooong Jul 15, 2020

Uh oh!

gspencergoog Aug 17, 2020

Uh oh!

LongCatIsLooong Aug 17, 2020

Uh oh!

LongCatIsLooong commented Aug 14, 2020

Uh oh!

gspencergoog Aug 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	@override
	Widget build(BuildContext context) {
	Widget child = MouseRegion(
	onEnter: _handleMouseEnter,
	onExit: _handleMouseExit,
	cursor: widget.mouseCursor,
	child: Focus(
	focusNode: widget.focusNode,
	autofocus: widget.autofocus,
	canRequestFocus: _canRequestFocus,
	onFocusChange: _handleFocusChange,
	child: widget.child,
	),
	);
	if (widget.enabled && widget.actions != null && widget.actions.isNotEmpty) {
	child = Actions(actions: widget.actions, child: child);
	}
	if (widget.enabled && widget.shortcuts != null && widget.shortcuts.isNotEmpty) {
	child = Shortcuts(shortcuts: widget.shortcuts, child: child);
	}
	return child;
	}
	}

Conversation

LongCatIsLooong commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

LongCatIsLooong commented Aug 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LongCatIsLooong commented Jul 13, 2020 •

edited

Loading

LongCatIsLooong Jul 14, 2020 •

edited

Loading