cashapp / paparazzi

Render your Android screens without a physical device or emulator
https://cashapp.github.io/paparazzi/
Apache License 2.0
2.23k stars 212 forks source link

Tests fails when using AndroidX font-family #488

Open magnusvs opened 2 years ago

magnusvs commented 2 years ago

Description It seems like ResourcesInterceptor overrides to use the platform Resources.getFont instead of the AndroidX ResourcesCompat. This causes a problem when using a font-family that only specifies app: attributes, instead of android: attributes.

Steps to Reproduce Use an AppTheme that sets textAppearance with a font-family like:

<?xml version="1.0" encoding="utf-8"?>
<font-family xmlns:app="http://schemas.android.com/apk/res-auto">
    <font
        app:font="@font/fk_grotesk_neue_regular"
        app:fontStyle="normal"
        app:fontWeight="400" />

    <font
        app:font="@font/fk_grotesk_neue_medium"
        app:fontStyle="normal"
        app:fontWeight="500" />

    <font
        app:font="@font/fk_grotesk_neue_bold"
        app:fontStyle="normal"
        app:fontWeight="700" />

</font-family>

By adding android: prefix the tests can inflate everything again.

<font-family xmlns:app="http://schemas.android.com/apk/res-auto"
xmlns:android="http://schemas.android.com/apk/res/android">
<font
    android:font="@font/fk_grotesk_neue_regular"
    android:fontStyle="normal"
    android:fontWeight="400"
    app:font="@font/fk_grotesk_neue_regular"
    app:fontStyle="normal"
    app:fontWeight="400" />

<font
    android:font="@font/fk_grotesk_neue_medium"
    android:fontStyle="normal"
    android:fontWeight="500"
    app:font="@font/fk_grotesk_neue_medium"
    app:fontStyle="normal"
    app:fontWeight="500" />

<font
    android:font="@font/fk_grotesk_neue_bold"
    android:fontStyle="normal"
    android:fontWeight="700"
    app:font="@font/fk_grotesk_neue_bold"
    app:fontStyle="normal"
    app:fontWeight="700" />

</font-family>

Expected behavior It seems like the behaviour was added because of this bug https://issuetracker.google.com/issues/156065472 which now is supposed to be fixed. Could the Interception be removed?

Additional information:

Screenshots

fcduarte commented 2 years ago

@magnusvs thanks for reporting! I tried to reproduce your issue with the current codebase and wasn't able to. Here's what I did:

  1. On file paparazzi-gradle-plugin/src/test/projects/custom-fonts-xml/src/main/res/font/cashmarket_medium.xml, I removed all references to android:font
  2. On file paparazzi-gradle-plugin/src/test/projects/custom-fonts-xml/src/main/res/layout/textviews.xml, I replaced android:fontFamily with app:fontFamily and changed all references@font/cashmarket_medium to @font/cashmarket_medium_normal
  3. Then ran ./gradlew clean :paparazzi-gradle-plugin:check and the test in question -- customFontsInXml worked (just had a small change on the image diff due to the font change I believe)

Could you try the same on our local and post your results? Also, if that is not the right way to simulate your issue could you provide some test cases?

Thanks!

magnusvs commented 1 year ago

Hi,

Sorry for the late response @fcduarte . I'm trying it out locally today but I'm just hitting errors compiling and can't manage to run tests at the moment. First couldn't sync project with VERSION_NAME=1.2.0-SNAPSHOT, after downgrading to VERSION_NAME=1.1.0 I could sync but then fails with

Cannot perform signing task ':paparazzi-agent:signMavenPublication' because it has no configured signatory

Not too familiar with this maven publishing plugin so haven't gotten around it yet.

Back to the issue, if I remember everything correctly, I think on your step 2 is where it's different from my own project setup.

"and changed all references@font/cashmarket_medium to @font/cashmarket_medium_normal" I believe shouldn't be done. We adjust cashmarket_medium in step 1, to not have android:font prefixes but should still be able to use it, I believe that's why you didn't hit any errors.

Just to expand on the initial report: Paparazzi does registerFontLookupInterceptionIfResourceCompatDetected which seems to intercept requests to androidx.core.content.res.ResourcesCompat.getFont (which should handle app:fontFamily I guess) and replaces it with android.content.res.Resources.getFont (which only handles android:fontFamily). Now this registerFontLookupInterceptionIfResourceCompatDetected was added as a workaround to a bug that should be fixed now according to it's comment.

jrodbx commented 1 year ago

Tried removing the method interceptor and the test still breaks due to the original issue. See comment here: https://issuetracker.google.com/issues/156065472#comment3

jrodbx commented 1 year ago

Solution is to use ASM to apply a transform similar to the one used by Android Studio: https://issuetracker.google.com/issues/156065472#comment4. Any volunteers?

aaalaniz commented 1 year ago

Hey @jrodbx 👋🏼

I would like to give this issue a try.

Solution is to use ASM to apply a transform similar to the one used by Android Studio

Can you elaborate on this approach?

TWiStErRob commented 1 year ago

@aaalaniz, @tevjef created a PR: #679

nileshteji commented 1 year ago

Picking this up