Chip label not vertically aligned when using sub themes

[davidmartos96]

I've opened PR #279 some time ago, but I'll leave the description as an issue if it helps discussion.

After upgrading to v8 I noticed that text chips were not aligned vertically.
After investigation it turned out to be that the following text style wasn't using the leadingDistribution as .even and textBaseline as .alphabetic.

flex_color_scheme/lib/src/flex_sub_themes.dart

Line 1995 in bbae097

final TextStyle baseLabelStyle = (labelStyle ?? const TextStyle()).copyWith(

Those properties come from the typography text theme geometry, and Flutter applies those styles when doing Theme.of(context), and inside ThemeData.localize is called.
The englishLike textTheme in the typography contains those properties.

The fix I propose is basically doing the same TextStyle.merge that Flutter does when calling Theme.of(context).

Theme.of(context)
https://github.com/flutter/flutter/blob/f6ff1529fd6d8af5f706051d9251ac9231c83407/packages/flutter/lib/src/material/theme.dart#L139

ThemeData.localize
https://github.com/flutter/flutter/blob/f6ff1529fd6d8af5f706051d9251ac9231c83407/packages/flutter/lib/src/material/theme_data.dart#L1763-L1766

To reproduce the issue you can try the following code with and without subThemesData. Hot reload won't work here, you need to hot restart to see the actual difference.

import 'package:flex_color_scheme/flex_color_scheme.dart';
import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    final theme = FlexThemeData.dark(
      scheme: FlexScheme.mandyRed,
      typography: Typography.material2021(),
      subThemesData: FlexSubThemesData(),
    );

    return MaterialApp(title: 'Flutter Demo', theme: theme, home: MyHomePage());
  }
}

class MyHomePage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            InputChip(
              avatar: Icon(Icons.emoji_emotions_rounded),
              onPressed: () {},
              label: ColoredBox(color: Colors.red, child: Text('My Chip')),
            ),
          ],
        ),
      ),
    );
  }
}

Current

Expected

This also automatically fixes the chip label fontWeight not using w500, which was forgotten in the copyWith linked above.

[rydmike]

Hi @davidmartos96, thank you for the issue and reminder about the long outstanding PR 😄
I will triage and review it tomorrow.

[rydmike]

Testing the PR

Putting the result with fix and without next to each other, I cannot see any visual difference:

Also, if I look at alignment of text in Chips when FlexColorScheme is used versus not used (vanilla Flutter), the results are identical, and as centered as can be:

Even when using the font size in Playground and enlarging it, the layout looks pretty well centered still:

This is ALL in-line with what I expected, why?
Let's look a that too.

Where are the geometry props?

It is a correct observation that the TextTheme in ThemeData created by FlexColorScheme.toTheme does not set mentioned geometry properties like leadingDistribution as .even and textBaseline as .alphabetic.

This is an effect of them not being created until after the localization step, which is done by MaterialApp that further modifies the ThemeData provided to it. The same is also the case with standard Flutter ThemeData factory.

To see the final result of the theme, you need to examine it in a context below MaterialApp.

If we examine the TextStyles in the final TextTheme we can see that the properties are indeed set. I added showing the above mentioned properties to the Themes Playground to demonstrate that the properties are indeed set (will be included in next release). It shows that these props are indeed already set and done by the localization step.

FlexColorScheme uses the vanilla Flutter ThemeData factory to create the theme, it does not use the raw constructor, so anything the Flutter ThemeData constructor does with props provided to it, are also done when using FlexColorScheme. Plus when you use put that into MaterialApp it gets passed into the standard localization steps, so what is mentioned here:

    // Lastly, in order to correctly customize text styles based on the default
    // TextTheme text styles, we need to use the typography geometry. This fixes
    // text disalignment issues in chip labels for example.
    // This is equivalent to what Flutter does under the hood in
    // Theme.of(context) which calls [ThemeData.localize].
    // Specifically, merging the geometry can use important text style
    // properties like [leadingDistribution] and [textBaseline] that affect text
    // rendering.

Is not really the case.

Issues with the PR

1. Does not change the result, as shown above.
2. Forces all geometry to be ScriptCategory.englishLike by providing a none null default as englishLike.

Point 2 means that locales that uses scripts that are not supposed to use the EnglishLike geometry now also get it, and not the default one for their locale. This happens because the geometry is no longer null and the localized default values for them will no longer be applied in the MaterialApp localization step.

Potential use of ScriptCategory

We could potentially add a final ScriptCategory? scriptCategory; as a null prop, where you can provide an override if you really want to ignore using the default localized geometry and always set it to a fixed choice. But, I'm not really sure what its use case would be.

Use cases where the Geometry issue applies

If you are using custom "extra" TextStyle for the Chip component theme, then it is true that the correct localized geometry is not applied to it. This also applies with Flutter's vanilla ThemeData if you define a component theme for Chips.

For example, when you get a TextTheme or TextStyle from Google fonts, they come without this geometry, so you may need to adjust it as needed. If you use them as TextStyle on the labelStyle in Chip component theme, it will not get the default geometry in the localization step, and may not be centered correctly. This is a correct observation, for both vanilla Flutter and FCS, but it is not demonstrated by the issue, nor is it a use case we can easily cover in FCS.

I suspect this might be the use case that made you notice this, but it is not shown by the reported issue, nor fixed by the PR.

Below we can see an example with custom TextStyle used directly on Chip label style.

Above there is indeed a slight difference in the center alignment, since some of the geometry is missing if we do not define it.

The code used to demo this:

import 'package:flex_color_scheme/flex_color_scheme.dart';
import 'package:flutter/material.dart';
import 'package:google_fonts/google_fonts.dart';

final TextStyle customChipTextStyle = GoogleFonts.leagueScript().copyWith(
  fontSize: 40,
  // Comment the two lines below to visually test the difference
  leadingDistribution: TextLeadingDistribution.even,
  textBaseline: TextBaseline.alphabetic,
);

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    final theme = FlexThemeData.dark(
      scheme: FlexScheme.mandyRed,
      typography: Typography.material2021(),
      subThemesData: FlexSubThemesData(
        chipLabelStyle: customChipTextStyle,
      ),
    );

    return MaterialApp(title: 'Flutter Demo', theme: theme, home: MyHomePage());
  }
}

class MyHomePage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final TextStyle? style = Theme.of(context).chipTheme.labelStyle;
    debugPrint('Style: $style');
    return Scaffold(
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            InputChip(
              avatar: Icon(Icons.emoji_emotions_rounded),
              onPressed: () {},
              label: ColoredBox(color: Colors.red, child: Text('My Chip')),
            ),
          ],
        ),
      ),
    );
  }
}

Summary

I am aware of this issue, but at the moment I do not know of any good way of solving it. It happens in both vanilla Flutter and FCS.

The ThemeData object is typically defined before the localization step and is context less, thus we cannot query the context for current locale and provide correct geometry to custom TextStyles used in component themes at this stage.

It should in fact be the task of the component itself, to in Flutter merge a provided custom provided TextStyle with current localized prevailing geometry. If I recall correctly (not sure, I would need to check for examples) some Flutter Material components actually do this, but at least the Chip does not do this, as also demonstrated above.

Conclusion

Open to suggestions on how to really fix this, but as it stands the PR does not fix anything that could be observed and won't be merged in its current state.

The current solution when using custom TextStyles in component themes is to make sure you provide the needed geometry, if the component does not merge it in itself.

It could be classified as an issue for components that do not do this, but I doubt it will be fixed.

[davidmartos96]

I will revisit this, as maybe the behavior has changed since the Flutter or FCS versions back 10 months ago. The images were taken back then.
From your images it doesn't look as different as mines. And the example I was using is the one shared, which doesn't use a custom label style.

Thanks!

[davidmartos96]

@rydmike I have run the example in the issue description on latest 8.4.0 version of FCS and Flutter 3.38.5 with and without the subThemesData property (with cold run in between) and this is the result. It was run in Android 16 emulator

I also executed without the PR to compare.

Where could the differences between our tests come from? The demo isn't using custom label styles, just opting in/out of the subthemes option

(Please ignore the differences in the red color, it's just a side effect of my desaturation OS settings when the window doesn't have the focus)