eBay / flutter_glove_box

Various eBay tools for Flutter development
BSD 3-Clause "New" or "Revised" License
317 stars 63 forks source link

Need to add to 'overridable fonts' for package font to be displayed #154

Closed guy-plentific closed 2 years ago

guy-plentific commented 2 years ago

We have an issue in our app that requires us to add overridable fonts for the fonts to be rendered in golden tests, even though they work fine when running the app. The fonts come from one of our own packages, and the styles which we refer to from our app are also defined within the package.

Our current app setup/structure is as follows: In the app pubspec.yaml we have fonts defined as follows

flutter:
  fonts:
    - family: AvenirNext
      fonts:
        - asset: packages/package_name/fonts/AvenirNext-Regular.ttf
          weight: 400

We use the fonts within our app as follows:

Text(
   'blah blah',
   style: context.textThemeDefinedInPackage.bodyDefault,
),

Our current package setup/structure is as follows: In the package pubspec.yaml we have fonts defined as follows

flutter:
  fonts:
    - family: AvenirNext
      fonts:
        - asset: lib/fonts/AvenirNext-Regular.ttf
          weight: 400

We define styles within our package as follows:

TextStyle(fontFamily: 'AvenirNext')

When we generate golden files (when using the style define in the package as shown above), we just see the squares instead of the fonts being rendered correctly. If we explicitly create a style in our text widgets the font is displayed in the golden test. For example:

Text(
   'this works'
   style: TextStyle(fontFamily: 'AvenirNext', package: 'plentific_ui_core'),
),
Text(
   'this also works'
   style: TextStyle(fontFamily: 'packages/plentific_ui_core/AvenirNext'),
),
Text(
   'this DOES NOT work in golden tests but does in the app'
   style: TextStyle(fontFamily: 'AvenirNext'),
),

The only way I have been able to get the fonts rendering in golden tests (when using the styles defined in the package) is to add 'AvenirNext' to the list of overridableFonts in the golden toolkit package font loader file. Now I have found a more elegant way to enable this, by adding an argument, additionalOverridableFonts to the loadAppFonts() method, however I thought I would run it by you guys first and see if anyone else has experienced similar issues, or if there are any other workarounds I have not found. If not, I can create a non-breaking PR with my fix.

guy-plentific commented 2 years ago

@coreysprague Hey bud I notice you have worked on this package and have experience with font issues - do you have any ideas about this?

coreysprague commented 2 years ago

Hey @guy-plentific , sorry for the delay in responding.

Can you clarify a few things? What package are you running the golden tests in, the app package? or in the package that defines the fonts?

We do a very similar thing in the eBay Motors app, but haven't ran into this problem. But, we also don't redundantly specify the fonts in the app package.

Golden Toolkit tries to read all the font specifications from the package under test and all transitive dependencies. It could be that for some reason, the two are colliding/conflicting?

coreysprague commented 2 years ago

Actually, I just re-read this. In our case, we have our sub-package defines the text styles, as extensions on MaterialTheme, and no consuming packages ever have to reference the font family.

For example... a consuming package might say:

Theme.of(context).customTextStyle

That being said, I'm open to exposing another means to inject additional fonts into the font loader.

guy-plentific commented 2 years ago

@coreysprague no worries thanks for getting back to me! The golden tests are in our app, rather than the package which contains the font files and styles.

When I debug loadAppFonts() in font_loader.dart I can see that both fonts are picked up in the font manifest, one with family 'AvenirNext' and one with family 'packages/package_name/AvenirNext'.

It could be that for some reason, the two are colliding/conflicting? - I don't know exactly what's happening under the hood but this seems like the most likely scenario. What are your thoughts? I would happily create a short PR that fixes the issue as described above but wanted to double check there isn't a better way around this first.

we have our sub-package defines the text styles, as extensions on MaterialTheme, and no consuming packages ever have to reference the font family - yeah that's pretty much what we are doing; we define the themes in the package, and other than defining fonts in pubspec.yaml in the app (as described above) our app doesn't explicitly reference the fonts, just the themes in the package.

coreysprague commented 2 years ago

Really I think the clunkiness is that Flutter doesn't have great mechanisms for depending on my resources from other packages.

my two cents is that the way they want you to reference them is by specifying the package name (e.g. your first example).

That behavior is true for fonts, images, and other assets you might reference. If you want to avoid having to specify the package name everywhere, I'd suggest making a re-usable API to encapsulate it.

That being said... if you wanted to enhance fontLoader to allow for additional injection of font families at runtime via a parameter, I think that would allow you to keep your current code and get the goldens working in your app package.

guy-plentific commented 2 years ago

Really I think the clunkiness is that Flutter doesn't have great mechanisms for depending on my resources from other packages. - agreed!

The examples of explicitly defining text styles within the app package was just to demonstrate what works and what doesn't work in golden tests. We do specify the package in the pubspec.yaml where we define the fonts, by using the full package path to the font. This causes no issues when running our app, only when we try to generate goldens, therefore I feel like it's more of a bug in the golden_toolkit library.

If I created a PR to enhance fontLoader to allow for additional injection of font families at runtime via a parameter, would you consider it for this package? We are trying to avoid using forks etc. where possible as it's a large enterprise project.

coreysprague commented 2 years ago

To your last question: yes, we would consider it.

Back to the issues about the font resolution. It's definitely a quirk of how flutter_test works, and the loadAppFonts() mechanism is an attempt to simulate how things would resolve in SKIA.

Right now, the current implementation interprets the auto generated Font manifest, and dynamically loads all of those package qualified font names into memory using the asset specified. We MIGHT be able to get it working by ALSO loading the same asset using a non package specified font name.

For example... when we load this from package

  fonts:
    - family: AvenirNext
      fonts:
        - asset: lib/fonts/AvenirNext-Regular.ttf
          weight: 400

it gets loaded as packages/packageName/AvenirNext. We could load it as that AND as AvenirNext

There might be some quirks, but I think it could work?

That aside, I think you can remove the redundant specification of the font in your app package (from your example)

guy-plentific commented 2 years ago

@coreysprague Hey Corey thanks for your detailed message and explanation. I have done some digging and realise that the issue was that within our package where we defined the themes/text styles, we had the font family defined as AvenirNext rather than packages/package_name/AvenirNext. This is why we had to explicitly define the font in the app pubspec.yaml. When we make this change in our package, the fonts render in the app without needing us to also define the font specification in our app's pubspec.yaml, and the golden tests work as expected! Once again thanks for helping me understand the issue, I'm going to close it now :)

mdikcinar commented 5 months ago

Hi, i am having the same issue and i couldn't clearly understand the solution. Here is my example Package side

  fonts:
    - family: Inter
      fonts:
        - asset: lib/fonts/Inter/Inter-Regular.ttf
          style: normal
          weight: 400

App side

    - family: Inter
      fonts:
        - asset: packages/frame_design_system/fonts/Inter/Inter-Regular.ttf
          style: normal
          weight: 400

App works well with this setting but goldens are failing because the font is not loading correctly in font_loader.dart derivedFontFamily method. Font name comes as "Inter" but this method manupulates its name to "packages/frame_design_system/Inter" because asset string starts with "package"

if (asset != null && asset.startsWith('packages')) {
        final packageName = asset.split('/')[1];
        return 'packages/$packageName/$fontFamily';
      }

When i read the solution i have changed my code like Package side

  fonts:
    - family: packages/frame_design_system/Inter
      fonts:
        - asset: lib/fonts/Inter/Inter-Regular.ttf
          style: normal
          weight: 400

After this change I didn't define the fonts on App side. But goldens are still failing because font_loader.dart derivedFontFamily method still not working correctly. Because font name comes as "packages/frame_design_system/packages/frame_design_system/Inter" and derivedFontFamily return directly this value as font family name. Could you help me, whats the correct solution here?

fatherOfLegends commented 5 months ago

It sounds like you have multiple packages. Locate and declare the fonts in the frame_design_system package. Do not declare them in your app or other packages. In the package with the goldens make sure the widget you are building is within a Theme widget providing the font family. If the package with the goldens is the same package you are declaring the font you have to do a bit more. I can provide some code when I have a bit more context.