Fix textscaler clamp assertion error#181716
Conversation
github-actions
bot
added
a: text input
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an assertion error in TextScaler.clamp when called on an already-clamped scaler. The logic is now more robust by calculating the new clamp boundaries before deciding which type of scaler to return. The added tests effectively cover the bug fix and a related edge case. My only feedback is a suggestion to improve the organization of the new tests for better maintainability, as detailed in the comment.
Valansch
force-pushed
the
fix-textscaler-clamp-assertion
branch
2 times, most recently
from
48b4b5b to
8afaeec
Compare
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
Can you run dart format on all files in this PR to fix failing checks?
EDIT: Nevermind. Analyze caught it. |
|
@victorsanni nevermind my last comment. Flutter analyze should have caught the issue. I addressed your comments. Will squash the second commit after review. |
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
To fix, run dart format packages/flutter/test/painting/text_scaler_test.dart
Valansch
force-pushed
the
fix-textscaler-clamp-assertion
branch
2 times, most recently
from
0d5b1e8 to
0940969
Compare
|
@victorsanni Resolved |
| return minScaleFactor == maxScaleFactor | ||
| ? _LinearTextScaler(minScaleFactor) | ||
| : _ClampedTextScaler(scaler, max(minScaleFactor, minScale), min(maxScaleFactor, maxScale)); | ||
| : _ClampedTextScaler(scaler, minScaleFactor, maxScaleFactor); |
There was a problem hiding this comment.
I don't think this is correct. When you compose 2 clampers the first one shouldn't be ignored.
There was a problem hiding this comment.
View this comment and make up you mind you two :)
#174828 (comment)
I implemented the fix first respecting the old bounds. Then i was told to ignore them, now not?
There was a problem hiding this comment.
Ah I thought by "discard" @victorsanni meant the this.minScale and this.maxScale should be removed from the expression. But with this implementation I think the first clamp is just going to be ignored and that would create some surprising behaviors. I would expect text scalers compose like functions so clamp2 . clamp1 === clamp2 . identityScaler . clamp1 (where identifyScaler is just a text scaler that returns the input text size as-is).
There was a problem hiding this comment.
But what do you do it the clamps are incompatible? Please view my comment on the issue i explained my thoughts there
There was a problem hiding this comment.
If clamp1 has a range of [1, 2] and clamp2 [3, 4], what should the output size be when the input font size 9 goes through clamp1 and then clamp2? I'm expecting 3 instead of 4 (but I think this implementation would return 4).
The developer doesn't know the runtime type of the TextScaler they're calling clamp on (at least in most cases). And with this implementation I believe when you call clamp the exact behavior will depend on the runtime type of the receiver, if the receiver is a _ClampTextScaler then the receiver's scaling logic will be ignored but if it's not then the clamping will be applied after the receiver's scaling logic is applied.
There was a problem hiding this comment.
Thinking about this again: My current approach, as the previous (bugged) version is commutative. But clamping is not. So the current approach is not working. I will add another test and consider your suggestion.
There was a problem hiding this comment.
I tried to implement your approach to just nest the clamedScalers like this:
@override
TextScaler clamp({double minScaleFactor = 0, double maxScaleFactor = double.infinity}) {
return minScaleFactor == maxScaleFactor
? TextScaler.linear(minScaleFactor)
: _ClampedTextScaler(this, minScaleFactor, maxScaleFactor);
}
Unfortunately that breaks equality, because only the last clamps are compared. Now I can fix the equality by implementing a numeric solution to this problem, but then why not just implement the numeric solution directly in the clamp and don't nest? This is in addition to my points raised above.
There was a problem hiding this comment.
Unfortunately that breaks equality, because only the last clamps are compared
Could you elaborate? The "inner" scaler is also compared in the == implementation. Also it's correct to always return false for TextScaler.== implementations because it's just an optimization to prevent unnecessary rebuilds. Function equality is inherently tricky so the documentation only asks that subclasses should identify "equal" text scalers on a best effort basis.
There was a problem hiding this comment.
Nesting the clamed scalers can result in the same scalers not being equal. Example:
final scaler = TexScaler.linear(2)
scaler.clamp(2,3) != scaler.clamp(1,4).clamp(2,3)
scaler.clamp(1,5).clamp(2,3) != scaler.clamp(1,4).clamp(2,3)
I believe those should be equal.
There was a problem hiding this comment.
TextScaler.== is only used by the framework to skip unnecessary widget rebuilds so you can always choose to return false, it's totally optional. But if you really want to prevent rebuilds as much as possible I think you can switch to the original implementation (union of the two clamp intervals) when the intervals overlap.
Valansch
force-pushed
the
fix-textscaler-clamp-assertion
branch
2 times, most recently
from
e31a48d to
5517a32
Compare
Valansch
force-pushed
the
fix-textscaler-clamp-assertion
branch
from
5517a32 to
b7e7914
Compare
|
@LongCatIsLooong @victorsanni I implemented a new version now. Please mind my response to your comments under the comment. If you have a change suggestion I can try it against this test. I believe this is the best version. I removed the ternary to add a comment. |
|
|
||
| if (newMaxScale <= newMinScale) { | ||
| // Ranges don't overlap or collapse to a single point. | ||
| return TextScaler.linear(clampDouble(minScale, minScaleFactor, maxScaleFactor)); |
There was a problem hiding this comment.
When newMaxScale < newMinScale I don't think this matches the API docs: https://main-api.flutter.dev/flutter/painting/TextScaler/clamp.html
There was a problem hiding this comment.
Do you mean for the inputs minScaleFactor maxScaleFactor? You are right this implementation removes the assert when maxScaleFactor > minScaleFactor.
I will expend the test and fix it.
But the goal of this newMaxScale <= newMinScale is to check if the new and old bounds overlap. So this condition is required here.
There was a problem hiding this comment.
I fixed it in a new commit and added more tests. Now it's consistent with the other two clamp functions
Valansch
force-pushed
the
fix-textscaler-clamp-assertion
branch
2 times, most recently
from
5f1de64 to
690f3bd
Compare
LongCatIsLooong
left a comment
LongCatIsLooong
left a comment
There was a problem hiding this comment.
LGTM. Thank you for fixing this!
Valansch
force-pushed
the
fix-textscaler-clamp-assertion
branch
from
690f3bd to
93a455f
Compare
|
@LongCatIsLooong Thanks for the review. Squashed the commits. Ready to merge from my end. |
victorsanni
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
Roll Flutter from dad6f9d4107a to b31548feb941 (39 revisions) flutter/flutter@dad6f9d...b31548f 2026-02-25 [email protected] [web] Fix failure on Firefox 148 (flutter/flutter#182855) 2026-02-25 [email protected] Roll Fuchsia Linux SDK from KfPgw04T0OEADLJA5... to XI0Ax7fbtYE4XKYAQ... (flutter/flutter#182887) 2026-02-25 [email protected] Use AnimationStyle curve and reverseCurve in ModalBottomSheet animation (flutter/flutter#181403) 2026-02-25 [email protected] Roll Dart SDK from fd3dce5b6a4e to 5c57e75f1102 (9 revisions) (flutter/flutter#182801) 2026-02-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "refactor: remove material in context_menu_controller_test, icon_test, list_wheel_scroll_view_test, media_query_test, platform_menu_bar_test (#182697)" (flutter/flutter#182879) 2026-02-25 [email protected] Make sure that an AnimatedSlide doesn't crash in 0x0 environment (flutter/flutter#181535) 2026-02-24 [email protected] Reland Standardize on Test* widgets in *_tester.dart files (flutter/flutter#182632) 2026-02-24 [email protected] docs(Path): clarify that zero-length contours are excluded from computeMetrics (flutter/flutter#180165) 2026-02-24 [email protected] Fix typo in assert message (flutter/flutter#182843) 2026-02-24 [email protected] [win32] Fix overflow in TaskRunnerWindow. (flutter/flutter#182822) 2026-02-24 [email protected] feat: Add --no-uninstall flag to flutter test for integration tests (flutter/flutter#182714) 2026-02-24 [email protected] Rename noFrequencyBasedMinification to useFrequencyBasedMinification (flutter/flutter#182684) 2026-02-24 [email protected] [Impeller] Fix fail to render pixel buffer texture on Linux (flutter/flutter#181656) 2026-02-24 [email protected] Remove FlutterFramework app migration (flutter/flutter#182100) 2026-02-24 [email protected] Roll Packages from 12b43a1 to 062c8d4 (5 revisions) (flutter/flutter#182839) 2026-02-24 [email protected] [web] Run webparagraph tests in CI (flutter/flutter#182092) 2026-02-24 [email protected] Fix a race in EmbedderTest.CanSpecifyCustomUITaskRunner (flutter/flutter#182649) 2026-02-24 [email protected] flutter_tools: Use a super-parameter in several missed cases (flutter/flutter#182581) 2026-02-24 [email protected] Replace more references to `flutter/engine` with `flutter/flutter` (flutter/flutter#182654) 2026-02-24 [email protected] Carousel: Migration from Scrollable+Viewport to CustomScrollView (flutter/flutter#182475) 2026-02-24 [email protected] Refactor impellerc_main to better organize some of its logic (flutter/flutter#182783) 2026-02-24 [email protected] Remove unused `getPluginList ` (flutter/flutter#182660) 2026-02-24 [email protected] Refactor: Remove material from ticker provider test (flutter/flutter#181697) 2026-02-24 [email protected] Roll Skia from 26eebffe12bd to f44d7db68805 (3 revisions) (flutter/flutter#182821) 2026-02-24 [email protected] refactor: remove material in context_menu_controller_test, icon_test, list_wheel_scroll_view_test, media_query_test, platform_menu_bar_test (flutter/flutter#182697) 2026-02-24 [email protected] Roll Skia from 7dad66aae75a to 26eebffe12bd (5 revisions) (flutter/flutter#182810) 2026-02-24 [email protected] Update roadmap for 2026 (flutter/flutter#182798) 2026-02-24 [email protected] Marks Windows tool_tests_commands_1_2 to be unflaky (flutter/flutter#179670) 2026-02-23 [email protected] [web] scroll iOS iframe text input into view (flutter/flutter#179759) 2026-02-23 [email protected] Fix textscaler clamp assertion error (flutter/flutter#181716) 2026-02-23 [email protected] Roll Skia from 9a5a3c92c336 to 7dad66aae75a (4 revisions) (flutter/flutter#182779) 2026-02-23 [email protected] Move more getters from userMessages class to the appropriate places (flutter/flutter#182656) 2026-02-23 [email protected] Manual roll Dart SDK from f8fac50475b8 to fd3dce5b6a4e (6 revisions) (flutter/flutter#182768) 2026-02-23 [email protected] Copy Flutter framework to Add to App FlutterPluginRgistrant (flutter/flutter#182523) 2026-02-23 [email protected] Add progress indicator to artifact downloads (flutter/flutter#181808) 2026-02-23 [email protected] Clarify batch release mode requirements (flutter/flutter#182228) 2026-02-23 [email protected] [web] Remove --disable-gpu from flutter chrome tests (flutter/flutter#182618) 2026-02-23 [email protected] running-apps: update running-apps to use Duration.ago() (flutter/flutter#182172) 2026-02-23 [email protected] Refactor bin/ shell scripts for better performance and safety (flutter/flutter#182674) 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] on the revert to ensure that a human is aware of the problem. ...
Fix `TextScaler.clamp` assertion failure when clamping already-clamped scalers Fixes flutter#174828 ## Description When calling `clamp()` on an already-clamped `TextScaler`, the `_ClampedTextScaler.clamp()` method was checking if `minScaleFactor == maxScaleFactor` **before** calculating the actual new min/max values. This caused assertion failures (`'maxScale > minScale': is not true`) when the calculated `newMinScale` and `newMaxScale` became equal after applying `max()`/`min()` operations. ### Example scenario that triggers the bug: ```dart final textScaler = MediaQuery.of(context).textScaler; // First clamp - creates a _ClampedTextScaler textScaler.clamp(minScaleFactor: 1.0, maxScaleFactor: 1.1); // Internal framework code calls clamp() again (e.g., in BottomNavigationBar) // This could result in newMinScale == newMaxScale, triggering the assertion ``` ### The fix: Calculate `newMinScale` and `newMaxScale` **first**, then check for equality. If they are equal, return a `_LinearTextScaler` instead of creating a `_ClampedTextScaler` that would fail its constructor assertion. ### Important This breaks before undefined behaviour. We now through an assertion error when new bounds are out of bounds even when min and max are equal. For more detail view my comment on this [here](flutter#174828 (comment)) --------- Co-authored-by: Victor Sanni <[email protected]>
3 participants
Fix
TextScaler.clampassertion failure when clamping already-clamped scalersFixes #174828
Description
When calling
clamp()on an already-clampedTextScaler, the_ClampedTextScaler.clamp()method was checking ifminScaleFactor == maxScaleFactorbefore calculating the actual new min/max values. This caused assertion failures ('maxScale > minScale': is not true) when the calculatednewMinScaleandnewMaxScalebecame equal after applyingmax()/min()operations.Example scenario that triggers the bug:
The fix:
Calculate
newMinScaleandnewMaxScalefirst, then check for equality. If they are equal, return a_LinearTextScalerinstead of creating a_ClampedTextScalerthat would fail its constructor assertion.Important
This breaks before undefined behaviour. We now through an assertion error when new bounds are out of bounds even when min and max are equal. For more detail view my comment on this here