[ui] Add null check in FontWeight.lerp #8274
Conversation
johnsonmh
added
prod: API break
Work in progress (WIP)
| @@ -80,6 +81,8 @@ class FontWeight { | |||
| /// an [AnimationController]. | |||
| static FontWeight lerp(FontWeight a, FontWeight b, double t) { | |||
| assert(t != null); | |||
| if (a == null && b == null) | |||
| return null; | |||
There was a problem hiding this comment.
In practice, since LibTxt's TextStyle defaults null font weights to w400, and dart:ui's FontWeight and TextStyle are only created as the final step before passing the data into native code, this change should cause minimal real-world impact.
The case where this makes a difference is when the user intended the font weight to inherit from a previous TextStyle in the stack. The behavior suggested here would allow it to inherit instead of be set to w400. On the framework side, the inherit flag can also be used to force inherit behavior.
There was a problem hiding this comment.
Was there any specific case where the old behavior was causing unexpected behavior?
There was a problem hiding this comment.
I only noticed it because I was experimenting with a change to BottomNavigationBar in the framework. Basically I wanted to add the fields: TextStyle selectedLabelStyle and TextStyle unselectedLabelStyle. Then, the bottom nav would lerp between the two text styles so that they would animate properly.
I noticed in my tests that this was adding a FontWeight.w400 somewhat unexpectedly, since pretty much every other [field on the TextStyle class] lerps to return null. Like lerpDouble and Color.lerp.
|
The change/code looks good, but wanted to run it through some people to better understand what the expected lerp behavior is. |
… fix-fontWeightLerp
|
@GaryQian Sounds good, let me know when you have any updates or hear any new opinions. Also, I added a couple unit tests. |
|
So @Hixie indicated he is ok with this change. We should make sure to go through the breaking change process at https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes before landing this. Let me know if you want to do it, if not, I can. |
|
I've actually already sent out a breaking change request: https://groups.google.com/forum/m/?utm_medium=email&utm_source=footer#!topic/flutter-announce/0uS_Hzq894I This went out a week ago, didn't get any comments on it, do you think it's OK to submit? |
|
Ahh ok, sure, that sounds good, go ahead! |
|
LGTM!!! |
…30352) flutter/engine@79a988f...3796d98 git log 79a988f..3796d98 --no-merges --oneline 3796d98 [ui] Add null check in FontWeight.lerp (flutter/engine#8274) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (bmparr@google.com), and stop the roller if necessary.
* Add null check in FontWeight.lerp * Add Unit tests for FontWeight.lerp
This reverts commit ae6574d.
This reverts commit ae6574d.
Add a check to see if both
aandbarenullinFontWeight.lerp. If both arenull, method should returnnull. Update comments to include this behavior.Closes flutter/flutter#29816