-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Prevent LayoutBuilder from rebuilding more than once #147856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent LayoutBuilder from rebuilding more than once #147856
Conversation
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 change moves dirty element tracking / rebuilding logic from BuildOwner to BuildScope, and keeps GlobalKey management stuff in BuildOwner
6f7bae5 to
622f4c0
Compare
622f4c0 to
1fc709f
Compare
1fc709f to
cf7a872
Compare
| try { | ||
| for (int index = 0; index < _dirtyElements.length; index = _dirtyElementIndexAfter(index)) { | ||
| final Element element = _dirtyElements[index]; | ||
| if (identical(element.buildScope, 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.
For my own understanding: Why do we need to check that they are still assigned this build scope? Can an element switch to a different build scope between getting marked as dirty and flushing dirty elements? (I guess this could happen maybe with global key moves?)
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 global key reparenting is the sole reason (it's documented that currently an Element must not change its buildScope at runtime): a dirty Element can be re-activated in a subtree that has a different build scope, in which case the dirty Element will show up in the dirty lists of both its old buildscope and the new buildscope.
| // (the root node), the owner should have already been assigned. | ||
| // See RootRenderObjectElement.assignOwner(). | ||
| _owner = parent.owner; | ||
| _buildScope = parent.buildScope; |
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.
Sooo if I override buildScope to return something custom (e.g. as done in the LayoutBuilder) buildScope and _buildScope would return different things?
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. _buildScope is the parent.buildScope cache, and should not be read by anyone except buildScope.
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.
Makes sense, as suggested in another comment, maybe we should rename _buildScope to avoid that it is being confused as the backing store for the buildScope getter, which it only sometimes is.
| /// [SliverLayoutBuilder]s should apply the double rebuild fix. This flag is | ||
| /// for migration only and **SHOULD NOT BE USED**. | ||
| @Deprecated('This is a temporary migration flag. DO NOT USE THIS.') // flutter_ignore: deprecation_syntax (see analyze.dart) | ||
| static bool applyDoubleRebuildFix = false; |
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 is this breaking that requires this migration flag?
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.
and some scuba tests that I unfortunately didn't save the links to.
goderbauer
left a comment
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.
LGTM
Co-authored-by: Michael Goderbauer <[email protected]>
* master: (115 commits) Roll Flutter Engine from 4adf453b6d68 to 19707e811b60 (1 revision) (flutter#149291) disable Impeller on external texture test. (flutter#149292) Roll Flutter Engine from 8d5d14a1db95 to 4adf453b6d68 (12 revisions) (flutter#149290) Update 3.22.1 release notes to include missing fix. (flutter#148999) Manual roll Flutter Engine from 60968ee3bde7 to 8d5d14a1db95 (1 revision) (flutter#149263) Reverts "Prevent LayoutBuilder from rebuilding more than once (flutter#147856)" (flutter#149279) Unskip expression evaluation test (flutter#149253) temporarily disable SemanticsAction tests to unblock engine change (flutter#149274) Adds benchmark for rrect_blur. (flutter#149261) Prevent LayoutBuilder from rebuilding more than once (flutter#147856) Add test for inherited_theme.0.dart (flutter#149120) Update progress_indicator.dart to indicate the adaptive option is for both macOS and iOS (flutter#145246) _ModalScopeStatus as InheritedModel (flutter#149022) Add test for radio.toggleable.0.dart (flutter#149153) Add a sentinel value for `TextStyle.height` (flutter#149049) Remove dynamic_layouts from issue template (flutter#149252) Roll Flutter Engine from 30aa720d4999 to 60968ee3bde7 (1 revision) (flutter#149255) Roll Flutter Engine from b26e1b023cdb to 30aa720d4999 (7 revisions) (flutter#149249) Roll Packages from a933c30 to 31d3329 (6 revisions) (flutter#149246) Clean leak in editable_text_test.dart. (flutter#149223) ...
* master: (115 commits) Roll Flutter Engine from 4adf453b6d68 to 19707e811b60 (1 revision) (flutter#149291) disable Impeller on external texture test. (flutter#149292) Roll Flutter Engine from 8d5d14a1db95 to 4adf453b6d68 (12 revisions) (flutter#149290) Update 3.22.1 release notes to include missing fix. (flutter#148999) Manual roll Flutter Engine from 60968ee3bde7 to 8d5d14a1db95 (1 revision) (flutter#149263) Reverts "Prevent LayoutBuilder from rebuilding more than once (flutter#147856)" (flutter#149279) Unskip expression evaluation test (flutter#149253) temporarily disable SemanticsAction tests to unblock engine change (flutter#149274) Adds benchmark for rrect_blur. (flutter#149261) Prevent LayoutBuilder from rebuilding more than once (flutter#147856) Add test for inherited_theme.0.dart (flutter#149120) Update progress_indicator.dart to indicate the adaptive option is for both macOS and iOS (flutter#145246) _ModalScopeStatus as InheritedModel (flutter#149022) Add test for radio.toggleable.0.dart (flutter#149153) Add a sentinel value for `TextStyle.height` (flutter#149049) Remove dynamic_layouts from issue template (flutter#149252) Roll Flutter Engine from 30aa720d4999 to 60968ee3bde7 (1 revision) (flutter#149255) Roll Flutter Engine from b26e1b023cdb to 30aa720d4999 (7 revisions) (flutter#149249) Roll Packages from a933c30 to 31d3329 (6 revisions) (flutter#149246) Clean leak in editable_text_test.dart. (flutter#149223) ...
Fixes flutter#146379: introduces `Element.buildScope` which `BuildOwner.buildScope` uses to identify subtrees that need skipping (those with different `BuildScope`s). If `Element.update` calls `updateChild` then dirty children will still be rebuilt regardless of their build scopes. This also introduces `LayoutBuilder.applyDoubleRebuildFix` migration flag which should only live for a week or less. Caveats: `LayoutBuilder`'s render object calls `markNeedsLayout` if a descendant Element is dirty. Since `markNeedsLayout` also implies `markNeedsPaint`, the render object is going to be very repaint/relayout-happy. Tests: Presubmits with the migration flag set to true: https://github.com/flutter/flutter/pull/147856/checks?check_run_id=24629865893
…r#147856)" (flutter#149279) Reverts: flutter#147856 Initiated by: loic-sharma Reason for reverting: tree is closed with errors like: ``` test/integration.shard/break_on_framework_exceptions_test.dart: breaks when rebuilding dirty elements throws [E] Expected: <45> Actual: <2756> package:matcher expect test\integration.shard\break_on_framework_exceptions_test.dart 56:5 main.expectException ===== asynchronous gap === Original PR Author: LongCatIsLooong Reviewed By: {goderbauer} This change reverts the following previous change: Fixes flutter#146379: introduces `Element.buildScope` which `BuildOwner.buildScope` uses to identify subtrees that need skipping (those with different `BuildScope`s). If `Element.update` calls `updateChild` then dirty children will still be rebuilt regardless of their build scopes. This also introduces `LayoutBuilder.applyDoubleRebuildFix` migration flag which should only live for a week or less. Caveats: `LayoutBuilder`'s render object calls `markNeedsLayout` if a descendant Element is dirty. Since `markNeedsLayout` also implies `markNeedsPaint`, the render object is going to be very repaint/relayout-happy. Tests: Presubmits with the migration flag set to true: https://github.com/flutter/flutter/pull/147856/checks?check_run_id=24629865893
flutter/flutter@c85fa6a...7eebe29 2024-05-30 [email protected] Roll Packages from 31d3329 to 910fabb (11 revisions) (flutter/flutter#149321) 2024-05-30 [email protected] Roll Flutter Engine from fb64b9a4e6f2 to 2fedfd3cc6e5 (2 revisions) (flutter/flutter#149320) 2024-05-30 [email protected] Fix `Slider` throws an error when `_labelPainter` text is null (flutter/flutter#148462) 2024-05-30 [email protected] Roll Flutter Engine from 5500c1a3969a to fb64b9a4e6f2 (1 revision) (flutter/flutter#149307) 2024-05-30 [email protected] Enable `explicitChildNodes` for the `AlertDialog` content (flutter/flutter#149130) 2024-05-30 [email protected] Roll Flutter Engine from 0c95e85dfbf4 to 5500c1a3969a (1 revision) (flutter/flutter#149304) 2024-05-30 [email protected] Roll Flutter Engine from 19707e811b60 to 0c95e85dfbf4 (1 revision) (flutter/flutter#149300) 2024-05-30 [email protected] Roll Flutter Engine from 4adf453b6d68 to 19707e811b60 (1 revision) (flutter/flutter#149291) 2024-05-30 [email protected] disable Impeller on external texture test. (flutter/flutter#149292) 2024-05-30 [email protected] Roll Flutter Engine from 8d5d14a1db95 to 4adf453b6d68 (12 revisions) (flutter/flutter#149290) 2024-05-30 [email protected] Update 3.22.1 release notes to include missing fix. (flutter/flutter#148999) 2024-05-30 [email protected] Manual roll Flutter Engine from 60968ee3bde7 to 8d5d14a1db95 (1 revision) (flutter/flutter#149263) 2024-05-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Prevent LayoutBuilder from rebuilding more than once (#147856)" (flutter/flutter#149279) 2024-05-29 [email protected] Unskip expression evaluation test (flutter/flutter#149253) 2024-05-29 [email protected] temporarily disable SemanticsAction tests to unblock engine change (flutter/flutter#149274) 2024-05-29 [email protected] Adds benchmark for rrect_blur. (flutter/flutter#149261) 2024-05-29 [email protected] Prevent LayoutBuilder from rebuilding more than once (flutter/flutter#147856) 2024-05-29 [email protected] Add test for inherited_theme.0.dart (flutter/flutter#149120) 2024-05-29 [email protected] Update progress_indicator.dart to indicate the adaptive option is for both macOS and iOS (flutter/flutter#145246) 2024-05-29 [email protected] _ModalScopeStatus as InheritedModel (flutter/flutter#149022) 2024-05-29 [email protected] Add test for radio.toggleable.0.dart (flutter/flutter#149153) 2024-05-29 [email protected] Add a sentinel value for `TextStyle.height` (flutter/flutter#149049) 2024-05-29 [email protected] Remove dynamic_layouts from issue template (flutter/flutter#149252) 2024-05-29 [email protected] Roll Flutter Engine from 30aa720d4999 to 60968ee3bde7 (1 revision) (flutter/flutter#149255) 2024-05-29 [email protected] Roll Flutter Engine from b26e1b023cdb to 30aa720d4999 (7 revisions) (flutter/flutter#149249) 2024-05-29 [email protected] Roll Packages from a933c30 to 31d3329 (6 revisions) (flutter/flutter#149246) 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 [email protected],[email protected],[email protected] 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 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
LayoutBuilder migration: LayoutBuilders no longer do double layout (currently the fix is behind a temporary flag). Related flutter/flutter PR: flutter/flutter#147856
…pdate * master: (168 commits) Fix the scrolling layout deviation of `CupertinoActionSheet` (flutter#149439) Roll Flutter Engine from 60a7bb2353b6 to a6aa5d826649 (2 revisions) (flutter#149627) Roll Flutter Engine from ea72558be758 to 60a7bb2353b6 (2 revisions) (flutter#149623) Place `flutter_gpu` in the package cache. (flutter#149299) Switch to triage-* labels for platform package triage (flutter#149614) Roll pub packages (flutter#149617) Fixes multi line textfield hint text gets ellipsized (flutter#148423) Support failures-only and silent reporters in `flutter test` (flutter#148739) [CupertinoActionSheet] Fix overflow of the overscroll section when the user scrolls far (flutter#149542) Fix InputDecorator.prefixIcon color when disabled (flutter#149595) Added filter callback on dropdown menu (flutter#143939) update generated localized message files in the stocks test app (flutter#148741) Add a simplified SimpleCascadingMenuApp example (flutter#149147) Reland "Prevent LayoutBuilder from rebuilding more than once (flutter#147856)" (flutter#149303) Move some benchmarks from MotoG4 to Mokey (flutter#149567) Roll Packages from d8e8e8c to 11e192a (2 revisions) (flutter#149596) Cleanup triage reports from docs/ (flutter#149545) Roll Flutter Engine from d81edf635a9f to ea72558be758 (1 revision) (flutter#149590) Roll Flutter Engine from b0f4d7459708 to d81edf635a9f (1 revision) (flutter#149468) Roll Flutter Engine from 40b868efcc46 to b0f4d7459708 (1 revision) (flutter#149467) ...
…pdate * master: (168 commits) Fix the scrolling layout deviation of `CupertinoActionSheet` (flutter#149439) Roll Flutter Engine from 60a7bb2353b6 to a6aa5d826649 (2 revisions) (flutter#149627) Roll Flutter Engine from ea72558be758 to 60a7bb2353b6 (2 revisions) (flutter#149623) Place `flutter_gpu` in the package cache. (flutter#149299) Switch to triage-* labels for platform package triage (flutter#149614) Roll pub packages (flutter#149617) Fixes multi line textfield hint text gets ellipsized (flutter#148423) Support failures-only and silent reporters in `flutter test` (flutter#148739) [CupertinoActionSheet] Fix overflow of the overscroll section when the user scrolls far (flutter#149542) Fix InputDecorator.prefixIcon color when disabled (flutter#149595) Added filter callback on dropdown menu (flutter#143939) update generated localized message files in the stocks test app (flutter#148741) Add a simplified SimpleCascadingMenuApp example (flutter#149147) Reland "Prevent LayoutBuilder from rebuilding more than once (flutter#147856)" (flutter#149303) Move some benchmarks from MotoG4 to Mokey (flutter#149567) Roll Packages from d8e8e8c to 11e192a (2 revisions) (flutter#149596) Cleanup triage reports from docs/ (flutter#149545) Roll Flutter Engine from d81edf635a9f to ea72558be758 (1 revision) (flutter#149590) Roll Flutter Engine from b0f4d7459708 to d81edf635a9f (1 revision) (flutter#149468) Roll Flutter Engine from 40b868efcc46 to b0f4d7459708 (1 revision) (flutter#149467) ...
…pdate * master: (181 commits) Fix the scrolling layout deviation of `CupertinoActionSheet` (flutter#149439) Roll Flutter Engine from 60a7bb2353b6 to a6aa5d826649 (2 revisions) (flutter#149627) Roll Flutter Engine from ea72558be758 to 60a7bb2353b6 (2 revisions) (flutter#149623) Place `flutter_gpu` in the package cache. (flutter#149299) Switch to triage-* labels for platform package triage (flutter#149614) Roll pub packages (flutter#149617) Fixes multi line textfield hint text gets ellipsized (flutter#148423) Support failures-only and silent reporters in `flutter test` (flutter#148739) [CupertinoActionSheet] Fix overflow of the overscroll section when the user scrolls far (flutter#149542) Fix InputDecorator.prefixIcon color when disabled (flutter#149595) Added filter callback on dropdown menu (flutter#143939) update generated localized message files in the stocks test app (flutter#148741) Add a simplified SimpleCascadingMenuApp example (flutter#149147) Reland "Prevent LayoutBuilder from rebuilding more than once (flutter#147856)" (flutter#149303) Move some benchmarks from MotoG4 to Mokey (flutter#149567) Roll Packages from d8e8e8c to 11e192a (2 revisions) (flutter#149596) Cleanup triage reports from docs/ (flutter#149545) Roll Flutter Engine from d81edf635a9f to ea72558be758 (1 revision) (flutter#149590) Roll Flutter Engine from b0f4d7459708 to d81edf635a9f (1 revision) (flutter#149468) Roll Flutter Engine from 40b868efcc46 to b0f4d7459708 (1 revision) (flutter#149467) ...
* master: (181 commits) Fix the scrolling layout deviation of `CupertinoActionSheet` (flutter#149439) Roll Flutter Engine from 60a7bb2353b6 to a6aa5d826649 (2 revisions) (flutter#149627) Roll Flutter Engine from ea72558be758 to 60a7bb2353b6 (2 revisions) (flutter#149623) Place `flutter_gpu` in the package cache. (flutter#149299) Switch to triage-* labels for platform package triage (flutter#149614) Roll pub packages (flutter#149617) Fixes multi line textfield hint text gets ellipsized (flutter#148423) Support failures-only and silent reporters in `flutter test` (flutter#148739) [CupertinoActionSheet] Fix overflow of the overscroll section when the user scrolls far (flutter#149542) Fix InputDecorator.prefixIcon color when disabled (flutter#149595) Added filter callback on dropdown menu (flutter#143939) update generated localized message files in the stocks test app (flutter#148741) Add a simplified SimpleCascadingMenuApp example (flutter#149147) Reland "Prevent LayoutBuilder from rebuilding more than once (flutter#147856)" (flutter#149303) Move some benchmarks from MotoG4 to Mokey (flutter#149567) Roll Packages from d8e8e8c to 11e192a (2 revisions) (flutter#149596) Cleanup triage reports from docs/ (flutter#149545) Roll Flutter Engine from d81edf635a9f to ea72558be758 (1 revision) (flutter#149590) Roll Flutter Engine from b0f4d7459708 to d81edf635a9f (1 revision) (flutter#149468) Roll Flutter Engine from 40b868efcc46 to b0f4d7459708 (1 revision) (flutter#149467) ...
| Problem | Before | After | | --- | --- | --- | | Width | <img width="797" alt="Screenshot 2024-02-09 at 1 29 09 PM" src="https://github.com/flutter/flutter/assets/389558/c49fa584-2550-41f6-ab80-6c20d01412b1"> | <img width="794" alt="Screenshot 2024-02-09 at 1 23 59 PM" src="https://github.com/flutter/flutter/assets/389558/1326f797-9883-4916-9de3-1939e7648d46"> | | Overflow |  |  | Fixes #78746 Fixes #92851 Part of #101620 Fixes #147483 Fixes #153274 Part of Google b/317115348 Fixes optionsViewOpenDirection not working, mentioned in #143249 (comment). ### Requirements * [x] By default, the width of the options matches the width of the field. * [x] Options can be aligned to the start or end for LTR/RTL languages. * [x] The optionsViewOpenDirection parameter is respected. * [x] If the options would vertically exceed the top or bottom of the screen, they reposition themselves to fit on the screen while covering the field. At least enough to tap an option. This has accessibility implications, because sometimes an Autocomplete near the edge of a narrow screen can be unusable. * [x] If the Autocomplete is in a ScrollView, then the options move along with the field during scrolling. * [x] When the field moves or resizes, the options position and size change to match. Even if the field is animated. * [ ] The options layout updates on the same frame that the field layout changes. It's probably not possible to check all of these boxes so we'll probably need to compromise. #### Tools that I've used to try to achieve this * LayoutBuilder to provide the field constraints[^1]. * Looking up layout information of a widget via GlobalKey. * CompositedTransformFollower/Target. * CustomSingleChildLayout. [^1]: Originally this didn't work due to a bug when using LayoutBuilder with OverlayPortal (#147856). That has now been fixed. Co-authored-by: Victor Sanni <[email protected]>
| Problem | Before | After | | --- | --- | --- | | Width | <img width="797" alt="Screenshot 2024-02-09 at 1 29 09 PM" src="https://github.com/flutter/flutter/assets/389558/c49fa584-2550-41f6-ab80-6c20d01412b1"> | <img width="794" alt="Screenshot 2024-02-09 at 1 23 59 PM" src="https://github.com/flutter/flutter/assets/389558/1326f797-9883-4916-9de3-1939e7648d46"> | | Overflow |  |  | Fixes flutter#78746 Fixes flutter#92851 Part of flutter#101620 Fixes flutter#147483 Fixes flutter#153274 Part of Google b/317115348 Fixes optionsViewOpenDirection not working, mentioned in flutter#143249 (comment). ### Requirements * [x] By default, the width of the options matches the width of the field. * [x] Options can be aligned to the start or end for LTR/RTL languages. * [x] The optionsViewOpenDirection parameter is respected. * [x] If the options would vertically exceed the top or bottom of the screen, they reposition themselves to fit on the screen while covering the field. At least enough to tap an option. This has accessibility implications, because sometimes an Autocomplete near the edge of a narrow screen can be unusable. * [x] If the Autocomplete is in a ScrollView, then the options move along with the field during scrolling. * [x] When the field moves or resizes, the options position and size change to match. Even if the field is animated. * [ ] The options layout updates on the same frame that the field layout changes. It's probably not possible to check all of these boxes so we'll probably need to compromise. #### Tools that I've used to try to achieve this * LayoutBuilder to provide the field constraints[^1]. * Looking up layout information of a widget via GlobalKey. * CompositedTransformFollower/Target. * CustomSingleChildLayout. [^1]: Originally this didn't work due to a bug when using LayoutBuilder with OverlayPortal (flutter#147856). That has now been fixed. Co-authored-by: Victor Sanni <[email protected]>
Fixes #146379: introduces
Element.buildScopewhichBuildOwner.buildScopeuses to identify subtrees that need skipping (those with differentBuildScopes). IfElement.updatecallsupdateChildthen dirty children will still be rebuilt regardless of their build scopes.This also introduces
LayoutBuilder.applyDoubleRebuildFixmigration flag which should only live for a week or less.Caveats:
LayoutBuilder's render object callsmarkNeedsLayoutif a descendant Element is dirty. SincemarkNeedsLayoutalso impliesmarkNeedsPaint, the render object is going to be very repaint/relayout-happy.Tests:
Presubmits with the migration flag set to true: https://github.com/flutter/flutter/pull/147856/checks?check_run_id=24629865893
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.