Remove `textScaleFactor` dependent logic from `AppBar` #128112

LongCatIsLooong · 2023-06-02T11:18:26Z

I am trying to remove textScaleFactor-dependent logic from the framework since it's likely going to be deprecated, hopefully the original logic isn't from the material spec.

I stole the sample code from #125038 and here are the screenshots (textScaleFactor = 3.0).

Internal Tests: no relevant test failures

Medium	Large

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

guidezpl · 2023-06-02T19:02:50Z

For context, a lot of text scale fixes were introduced ~3 years ago (b/hotlists/2302728) to address overscaling of text in certain components. It's not been reintegrated in spec (I believe) due to this coming up, which is likely to negate the need for special handling for that platform, at least.

HansMuller

This LGTM although I'm only assuming that the layout remains compatible because the existing tests pass.

HansMuller · 2023-06-02T22:47:55Z

dev/tools/gen_defaults/lib/app_bar_template.dart

@@ -79,7 +79,7 @@ class _MediumScrollUnderFlexibleConfig with _ScrollUnderFlexibleConfig {
    ${textStyle('md.comp.top-app-bar.medium.headline')}?.apply(color: ${color('md.comp.top-app-bar.medium.headline.color')});

  @override
-  EdgeInsetsGeometry? get expandedTitlePadding => const EdgeInsets.fromLTRB(16, 0, 16, 20);
+  EdgeInsetsGeometry get expandedTitlePadding => const EdgeInsets.fromLTRB(16, 0, 16, 20);


HansMuller · 2023-06-02T23:14:59Z

packages/flutter/lib/src/material/app_bar.dart

-      }
-      return padding;
-    }
+    final Widget? expandedTitle = switch ((title, expandedTextStyle)) {


This is slick but I'm not sure the logic is the same or even simpler than before.

if (title != null) { ... expandedTitle = config.expandedTextStyle != null ? DefaultTextStyle( style: expandedTextStyle!, child: title!, ) : title; }

Would only wrap the title with the DefaultTextStyle if config.expandedTextStyle was non-null. The new code checks expandedTextStyle which incorporates the widget/theme defaults. I don't know why the code was the way it was, so I suppose if the change doesn't break anything it's OK.

It's from https://github.com/flutter/flutter/pull/122542/files#diff-20c19d254b0a82027625f1246ba51cb2264bfabf105fe91d532c19e6a6d77478L2141-L2151

@TahaTesser does this change look ok to you?

This looks good to me!

config.expandedTextStyle != null existed before I added title ?? appBarTheme.titleTextStyle. I should have updated it to just expandedTextStyle != null. Yours refactor makes it much cleaner and hard to miss.

TahaTesser

Thanks for doing this. @LongCatIsLooong

LGTM with minor nits.

TahaTesser · 2023-06-05T09:35:18Z

dev/tools/gen_defaults/lib/app_bar_template.dart

@@ -79,7 +79,7 @@ class _MediumScrollUnderFlexibleConfig with _ScrollUnderFlexibleConfig {
    ${textStyle('md.comp.top-app-bar.medium.headline')}?.apply(color: ${color('md.comp.top-app-bar.medium.headline.color')});

  @override
-  EdgeInsetsGeometry? get expandedTitlePadding => const EdgeInsets.fromLTRB(16, 0, 16, 20);
+  EdgeInsetsGeometry get expandedTitlePadding => const EdgeInsets.fromLTRB(16, 0, 16, 20);


packages/flutter/lib/src/material/app_bar.dart

…tter/flutter#128112)

Roll Flutter from 0b7415356e07 to 8a5c22e282db (46 revisions) flutter/flutter@0b74153...8a5c22e 2023-06-07 5236035+fzyzcjy@users.noreply.github.com Super tiny MediaQuery doc update (flutter/flutter#127904) 2023-06-07 jacksongardner@google.com Revert "Make inspector weakly referencing the inspected objects." (flutter/flutter#128436) 2023-06-07 leigha.jarett@gmail.com Update menu API docs to help developers migrate to m3 (flutter/flutter#128351) 2023-06-07 andrewrkolos@gmail.com [tools] allow explicitly specifying the JDK to use via a new config setting (flutter/flutter#128264) 2023-06-07 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6f9df0f988c1 to 59d5444cf06c (3 revisions) (flutter/flutter#128376) 2023-06-07 andrewrkolos@gmail.com Do not try to load main/default asset image if only higher-res variants exist (flutter/flutter#128143) 2023-06-07 92602467+99spark@users.noreply.github.com Addressed Ambiguity in transform.scale constructor docs (flutter/flutter#128182) 2023-06-07 5236035+fzyzcjy@users.noreply.github.com Super tiny fix of dead link (flutter/flutter#128160) 2023-06-07 goderbauer@google.com Refactor tests (flutter/flutter#128371) 2023-06-07 polinach@google.com Make inspector weakly referencing the inspected objects. (flutter/flutter#128095) 2023-06-07 goderbauer@google.com Add viewId to PointerEvents (flutter/flutter#128287) 2023-06-07 5236035+fzyzcjy@users.noreply.github.com Show error message in release mode when box is not laid out without losing performance (flutter/flutter#126302) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from ca499463ec2e to 6f9df0f988c1 (1 revision) (flutter/flutter#128363) 2023-06-06 khanhnwin@gmail.com Update Draggable YouTube video link (flutter/flutter#128078) 2023-06-06 31859944+LongCatIsLooong@users.noreply.github.com Remove more rounding hacks from TextPainter (flutter/flutter#127826) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 4571695f9e76 to ca499463ec2e (1 revision) (flutter/flutter#128356) 2023-06-06 43054281+camsim99@users.noreply.github.com [Android] Update plugin and module templates to use Flutter constant for `compileSdkVersion` (flutter/flutter#128073) 2023-06-06 rmolivares@renzo-olivares.dev handleSelectWord in MultiSelectableSelectionContainerDelegate should handle rects inside of rects (flutter/flutter#127478) 2023-06-06 christopherfujino@gmail.com [flutter_tools] never tree shake 0x20 (space) font codepoints on web (flutter/flutter#128302) 2023-06-06 31859944+LongCatIsLooong@users.noreply.github.com Remove `textScaleFactor` dependent logic from `AppBar` (flutter/flutter#128112) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from b6d37f8f74ad to 4571695f9e76 (6 revisions) (flutter/flutter#128350) 2023-06-06 5236035+fzyzcjy@users.noreply.github.com Fix `Null check operator used on a null value` on TextField with contextMenuBuilder (flutter/flutter#128114) 2023-06-06 engine-flutter-autoroll@skia.org Roll Packages from db4e5c2 to da72219 (10 revisions) (flutter/flutter#128348) 2023-06-06 91688203+yusuf-goog@users.noreply.github.com Updating cirrus docker image to ubuntu focal. (flutter/flutter#128291) 2023-06-06 leigha.jarett@gmail.com Adding example for migrating to navigation drawer (flutter/flutter#128295) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 722aad83e5fe to b6d37f8f74ad (2 revisions) (flutter/flutter#128341) 2023-06-06 goderbauer@google.com Clean-up viewId casts in flutter_test (flutter/flutter#128256) 2023-06-06 kevinjchisholm@google.com Update cherry-pick issue template to more uniform labels. (flutter/flutter#128333) 2023-06-06 hans.muller@gmail.com Use Material3 in the 2D viewport tests (flutter/flutter#128155) 2023-06-06 nbosch@google.com Use a `show` over a `hide` for `test_api` exports (flutter/flutter#128298) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from aaa7574375a6 to 722aad83e5fe (1 revision) (flutter/flutter#128307) 2023-06-06 leigha.jarett@gmail.com Migration guide for moving from BottomNavigationBar to NavigationBar (flutter/flutter#128263) 2023-06-06 mdebbar@google.com [web] Use 'Uri' instead of 'dart:html' to extract pathname (flutter/flutter#127983) 2023-06-06 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2b353ae90731 to aaa7574375a6 (4 revisions) (flutter/flutter#128301) 2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 220ece4d9faa to 2b353ae90731 (1 revision) (flutter/flutter#128293) 2023-06-05 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 4.0.4 to 4.1.0 (flutter/flutter#128290) 2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7f12e3497428 to 220ece4d9faa (6 revisions) (flutter/flutter#128282) 2023-06-05 jonahwilliams@google.com [framework] attempt non-key solution (flutter/flutter#128273) 2023-06-05 chillers@google.com [labeler] Fix adding labels when name is directory (flutter/flutter#128243) 2023-06-05 katelovett@google.com Remove scrollbar deprecations isAlwaysShown and hoverThickness (flutter/flutter#127351) 2023-06-05 katelovett@google.com Fix update drag error that made NestedScrollView un-scrollable (flutter/flutter#127718) 2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9f72388a4da to 7f12e3497428 (4 revisions) (flutter/flutter#128271) 2023-06-05 goderbauer@google.com Migrate SemanticsBinding to onSemanticsActionEvent (flutter/flutter#128254) 2023-06-05 engine-flutter-autoroll@skia.org Roll Flutter Engine from c838a1b05924 to f9f72388a4da (19 revisions) (flutter/flutter#128252) 2023-06-05 jonahwilliams@google.com [framework] force flexible space background to rebuild. (flutter/flutter#128138) 2023-06-05 engine-flutter-autoroll@skia.org Roll Packages from 75085ed to db4e5c2 (4 revisions) (flutter/flutter#128246) ...

…tter/flutter#128112)

LongCatIsLooong added 3 commits June 1, 2023 22:05

Remove some textScaleFactor references from AppBar

ac04719

fix tests

d5eef82

comments

4a08d5a

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Jun 2, 2023

github-actions bot removed the f: material design flutter/packages/flutter/material repository. label Jun 2, 2023

LongCatIsLooong requested review from TahaTesser, Piinks and guidezpl June 2, 2023 21:06

LongCatIsLooong marked this pull request as ready for review June 2, 2023 21:08

flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Jun 2, 2023

Piinks requested a review from HansMuller June 2, 2023 22:09

HansMuller approved these changes Jun 2, 2023

View reviewed changes

tests

ff025bf

LongCatIsLooong force-pushed the remove-text-scale-factor-app-bar branch from 617d348 to ff025bf Compare June 2, 2023 23:34

github-actions bot removed the f: material design flutter/packages/flutter/material repository. label Jun 2, 2023

TahaTesser approved these changes Jun 5, 2023

View reviewed changes

TahaTesser added the f: material design flutter/packages/flutter/material repository. label Jun 5, 2023

guidezpl approved these changes Jun 5, 2023

View reviewed changes

Merge branch 'master' into remove-text-scale-factor-app-bar

f97dd31

github-actions bot removed the f: material design flutter/packages/flutter/material repository. label Jun 5, 2023

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 6, 2023

auto-submit bot merged commit 73e1f23 into flutter:master Jun 6, 2023
139 checks passed

LongCatIsLooong deleted the remove-text-scale-factor-app-bar branch June 6, 2023 20:48

engine-flutter-autoroll mentioned this pull request Jun 7, 2023

Roll Flutter from 0b7415356e07 to 6848110ff3a9 (44 revisions) flutter/packages#4156

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2023

73e1f2342 Remove textScaleFactor dependent logic from AppBar (flu…

960771a

…tter/flutter#128112)

engine-flutter-autoroll mentioned this pull request Jun 7, 2023

Roll Flutter from 0b7415356e07 to 2830f0734d6c (45 revisions) flutter/packages#4158

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2023

73e1f2342 Remove textScaleFactor dependent logic from AppBar (flu…

606b573

…tter/flutter#128112)

engine-flutter-autoroll mentioned this pull request Jun 7, 2023

Roll Flutter from 0b7415356e07 to 8a5c22e282db (46 revisions) flutter/packages#4160

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2023

73e1f2342 Remove textScaleFactor dependent logic from AppBar (flu…

39bb193

…tter/flutter#128112)

HansMuller mentioned this pull request Jun 9, 2023

Flutter Framework Team: Notable Commits #121415

Open

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

73e1f2342 Remove textScaleFactor dependent logic from AppBar (flu…

c7a66df

…tter/flutter#128112)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

73e1f2342 Remove textScaleFactor dependent logic from AppBar (flu…

37fccab

…tter/flutter#128112)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

73e1f2342 Remove textScaleFactor dependent logic from AppBar (flu…

c0f1106

…tter/flutter#128112)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

73e1f2342 Remove textScaleFactor dependent logic from AppBar (flu…

df4b669

…tter/flutter#128112)

TahaTesser mentioned this pull request Sep 25, 2023

SliverAppBar.large & SliverAppBar.medium title constraints infinite on 3.13 #134779

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `textScaleFactor` dependent logic from `AppBar` #128112

Remove `textScaleFactor` dependent logic from `AppBar` #128112

Remove textScaleFactor dependent logic from AppBar #128112

Remove textScaleFactor dependent logic from AppBar #128112

Conversation

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove `textScaleFactor` dependent logic from `AppBar` #128112

Remove `textScaleFactor` dependent logic from `AppBar` #128112