Fix textscaler clamp assertion error

[Valansch]

Fix TextScaler.clamp assertion failure when clamping already-clamped scalers

Fixes #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:

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

[gemini-code-assist]

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.

[victorsanni]

Can you run dart format on all files in this PR to fix failing checks?

[Valansch]

@victorsanni

Can you run dart format on all files in this PR to fix failing checks?

I don't understand why it would be failing. Looks good to me. Is using dart from /bin/ incorrect? Which version should I use. Sorry if I missed some instructions.

./bin/dart format packages/flutter/
Formatted 1685 files (0 changed) in 12.18 seconds

EDIT: Nevermind. Analyze caught it.

[Valansch]

@victorsanni nevermind my last comment. Flutter analyze should have caught the issue. I addressed your comments. Will squash the second commit after review.

[victorsanni]

To fix, run dart format packages/flutter/test/painting/text_scaler_test.dart

[Valansch]

@victorsanni Resolved

[LongCatIsLooong]

I don't think this is correct. When you compose 2 clampers the first one shouldn't be ignored.

[Valansch]

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?

[LongCatIsLooong]

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).

[Valansch]

But what do you do it the clamps are incompatible? Please view my comment on the issue i explained my thoughts there

[LongCatIsLooong]

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.

[Valansch]

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.

[Valansch]

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.

[LongCatIsLooong]

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.

[Valansch]

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.

[LongCatIsLooong]

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]

@LongCatIsLooong @victorsanni I implemented a new version now. Please mind my response to your comments under the comment.
I have also extended the test, it now tests all cases and equality.

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.

[LongCatIsLooong]

When newMaxScale < newMinScale I don't think this matches the API docs: https://main-api.flutter.dev/flutter/painting/TextScaler/clamp.html

[Valansch]

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.

[Valansch]

I fixed it in a new commit and added more tests. Now it's consistent with the other two clamp functions

[LongCatIsLooong]

LGTM. Thank you for fixing this!

[Valansch]

@LongCatIsLooong Thanks for the review. Squashed the commits. Ready to merge from my end.