Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
guy-plentific opened this issue Apr 25, 2022 · 10 comments
Closed

Comments

@guy-plentific
Copy link

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Contributor

coreysprague commented Apr 27, 2022

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
Copy link
Author

guy-plentific commented Apr 27, 2022

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

@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
Copy link

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants