Repository navigation
Return null from RenderProxyBoxMixin.computeDryBaseline when the child has no baseline - #189723
Conversation
…d has no baseline
github-actions
Bot
added
the
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates RenderProxyBoxMixin.computeDryBaseline to return null when the child render box exists but has no baseline, rather than falling back to the superclass's baseline. A regression test and a helper class _RenderNoBaseline have been added to proxy_box_test.dart to verify this behavior. I have no feedback to provide.
elliette
left a comment
elliette
left a comment
There was a problem hiding this comment.
I triggered a re-run of the timed out test, otherwise LGTM! Thanks for your contribution!
justinmc
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/189723, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/189723, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.
|
chunhtai
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/189723, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
auto-submit
Bot
removed
the
autosubmit
chunhtai
added
autosubmit
| @protected | ||
| double? computeDryBaseline(covariant BoxConstraints constraints, TextBaseline baseline) { | ||
| final double? result = child?.getDryBaseline(constraints, baseline); | ||
| return result ?? super.computeDryBaseline(constraints, baseline); |
There was a problem hiding this comment.
@chunhtai do you remember why we're falling back to the super implementation at all, instead of just returning null when child is null? This is a mixin the behavior will be a bit difficult to reason about without knowing who the actual base class is.
There was a problem hiding this comment.
(This isn't blocking since tests are passing. Just curious why we are calling the super implementation, without knowing who super is).
There was a problem hiding this comment.
I can see problem both way, if we return null and somehow the base class does have fallback logic when there is no child, returning null here will be undesirable and also a breaking change.
that being said, other dry layout calculation do have hardcode returns if no child. We should probably unify those one way or the other.
as the scope of this PR, i think we should keep it as is.
There was a problem hiding this comment.
actually now thinking of this. I can't think of a good reason people will want to implement layout calculation in the base class before mixing in renderproxyboxmixin, because all those calculation will be overriden if child is not null. so I think returning null probably makes more sense?
Let's do that in a separate pr once this land, because there are more methods that call super
There was a problem hiding this comment.
Yeah it is not blocking, feel free to merge this.
My pedantic concern is just that I would expect a ProxyBox to have no baseline if it doesn't have a child (it's a proxy after all. If a subclass can have a baseline without a child then they should override this method themselves), and also since the mixin itself does not know who super is so the code is a bit difficult to reason about locally / on its own, without knowing who's using this mixin and what the base classes are.
chunhtai
added
autosubmit
|
autosubmit label was removed for flutter/flutter/189723, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
Bot
removed
the
autosubmit
chunhtai
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
flutter/flutter#189723 merged this morning, closing #189711. Adds the entry with its six locale translations and refreshes the narrative stage baselines, which moved by one card now that the ledger holds twenty contributions.
…12401) Manual roll Flutter from 2757a77a73df to e52f01c920ad (47 revisions) Manual roll requested by [email protected] flutter/flutter@2757a77...e52f01c 2026-07-31 [email protected] [Impeller] Execute PipelineCompileQueue jobs in the order that they were added (flutter/flutter#190262) 2026-07-31 [email protected] Roll Packages from 7d17fc8 to 5351d8c (5 revisions) (flutter/flutter#190351) 2026-07-31 [email protected] Roll Skia from dcbd015c9646 to 7ef86a5b0eb9 (4 revisions) (flutter/flutter#190347) 2026-07-31 [email protected] [Impeller] Do not acquire an unneeded image in KHRSwapchainVK if the swapchain must be recreated due to a size change (flutter/flutter#190116) 2026-07-31 [email protected] Remove the --enable-serial-gc engine flag (flutter/flutter#190310) 2026-07-31 [email protected] Migrate platform_channel_swift example to material_ui (flutter/flutter#190308) 2026-07-31 [email protected] macOS: Set static text accessibility role for read-only text fields (flutter/flutter#190330) 2026-07-31 [email protected] Roll Dart SDK from 96f49fc43f59 to c3acfc2479f6 (1 revision) (flutter/flutter#190336) 2026-07-31 [email protected] Roll Skia from 07f08414dd66 to dcbd015c9646 (1 revision) (flutter/flutter#190333) 2026-07-31 [email protected] [web] Autofill all AutofillGroup fields in semantics mode (flutter/flutter#187108) 2026-07-31 [email protected] iOS: Configure test task runner thread at display QoS (flutter/flutter#190283) 2026-07-31 [email protected] Roll Skia from a1109355de78 to 07f08414dd66 (5 revisions) (flutter/flutter#190328) 2026-07-31 [email protected] Support reduceMotion accessibility feature in MediaQuery (flutter/flutter#190287) 2026-07-30 [email protected] Roll Dart SDK from 43f7de5f4977 to 96f49fc43f59 (4 revisions) (flutter/flutter#190312) 2026-07-30 [email protected] fix: text selection two handles directionality. (flutter/flutter#179928) 2026-07-30 [email protected] Add java/gradle check when gradle crashes (flutter/flutter#189997) 2026-07-30 [email protected] [Impeller] Add filterQuality support to ImageFilter.shader (flutter/flutter#188544) 2026-07-30 [email protected] [flutter/flutter] Migrate multiple_windows example to material_ui (flutter/flutter#190313) 2026-07-30 [email protected] Roll Skia from 50cf67910462 to a1109355de78 (6 revisions) (flutter/flutter#190306) 2026-07-30 [email protected] Migrate swift xctests to Swift testing tests (flutter/flutter#187801) 2026-07-30 [email protected] Migrates the platform_channel example to material_ui (flutter/flutter#190253) 2026-07-30 [email protected] iOS: Drop unnecessary @mainactor from VSyncClientTests (flutter/flutter#190285) 2026-07-30 [email protected] Add nullptr check to isOpaque (flutter/flutter#189796) 2026-07-30 [email protected] [Flutter GPU] Bind uniforms and textures by cached reflection index (flutter/flutter#189820) 2026-07-30 [email protected] Migrate layers example to material_ui (flutter/flutter#190237) 2026-07-30 [email protected] Add `ShapeBorder` hit test fast paths (flutter/flutter#187962) 2026-07-30 [email protected] ci: mac-must-wait part duex (flutter/flutter#190295) 2026-07-30 [email protected] Roll Skia from 491301f393d1 to 50cf67910462 (3 revisions) (flutter/flutter#190294) 2026-07-30 [email protected] Roll Packages from dd11626 to 7d17fc8 (1 revision) (flutter/flutter#190293) 2026-07-30 [email protected] Roll Skia from 177de1940bf2 to 491301f393d1 (1 revision) (flutter/flutter#190279) 2026-07-30 [email protected] Roll Skia from b8bda91f02db to 177de1940bf2 (5 revisions) (flutter/flutter#190272) 2026-07-30 [email protected] Roll Fuchsia Linux SDK from J8rVTlBnjnUpuHYk3... to _J8wM3kyQpLN9wvRD... (flutter/flutter#190273) 2026-07-30 [email protected] Roll Skia from 78afc18c9ba0 to b8bda91f02db (3 revisions) (flutter/flutter#190268) 2026-07-30 [email protected] Revert: Split the platform and cpuArch part of TargetPlatform (flutter/flutter#190258) 2026-07-30 [email protected] Return null from RenderProxyBoxMixin.computeDryBaseline when the child has no baseline (flutter/flutter#189723) 2026-07-30 [email protected] Roll Skia from 6fea713d32ec to 78afc18c9ba0 (4 revisions) (flutter/flutter#190257) 2026-07-30 [email protected] Roll pub packages (flutter/flutter#190259) 2026-07-29 [email protected] [cross imports] Check examples cross imports (flutter/flutter#187662) 2026-07-29 [email protected] ci: mac verify binaries still has to wait on linux_host_engine (flutter/flutter#190239) 2026-07-29 [email protected] Adds analytics to macos impeller opt in/out (flutter/flutter#190220) 2026-07-29 [email protected] Roll Skia from 0f35bba4945c to 6fea713d32ec (6 revisions) (flutter/flutter#190230) 2026-07-29 [email protected] iOS: Fix Swift warning in TracingTests (flutter/flutter#190203) 2026-07-29 [email protected] Convert all gradle-wrapper.properties in /dev to use distribution 'bin' (flutter/flutter#190226) 2026-07-29 [email protected] Roll Packages from 3e63635 to dd11626 (11 revisions) (flutter/flutter#190216) ...
5 participants
RenderProxyBoxMixin.computeDryBaselineforwards the child's dry baseline asresult ?? super.computeDryBaseline(constraints, baseline). When a child is present but has no baseline, the null falls through toRenderBox.computeDryBaseline, which asserts that the class "does not implement computeDryBaseline". Returning null is explicitly allowed by thecomputeDryBaselinecontract, so any proxy box above such a child crashes when queried for a dry baseline, for example a baseline-alignedWidgetSpanwrappingSemantics.The mixin now returns the child's answer as-is when a child is present, and only falls back to the base implementation when there is no child. This matches what
RenderConstrainedBoxalready does in the same file.Added a rendering-layer regression test. The widget-level reproduction from the issue also passes with this change.
Fixes #189711
Pre-launch Checklist
///).