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

AccessibliityRenderExtension causes layout bug when content uses animateContentSize modifier #1165

Open
drewhamilton opened this issue Dec 1, 2023 · 9 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@drewhamilton
Copy link

Description
When any part of Compose UI content uses the animateContentSize modifier, and AccessibilityRenderExtension is used, the content is offset 50% to the left of where it should be, leaving a blank gap in the remaining 50% of space in between the shifted content and the rendered accessibility info. This occurs even if InstantAnimationsRule is used.

It does not occur if AccessibilityRenderExtension is not used.

Steps to Reproduce

  1. Check out my animate-content-size-accessibility-bug branch
  2. Run ./gradlew sample:recordPaparazziDebug --tests=ComposeA11yTest
  3. Compare app.cash.paparazzi.sample_ComposeA11yTest_animateContentSize[false].png and app.cash.paparazzi.sample_ComposeA11yTest_animateContentSize[true].png

Note that the image with renderAccessibilityInfo set to false renders correctly, while the one with renderAccessibilityInfo set to true incorrectly offsets the content.

Expected behavior
The content should not be offset.

Additional information:

  • Paparazzi Version: master branch
  • OS: MacOS (M1 Max)
  • Compile SDK: Paparazzi sample
  • Gradle Version: 8.3
  • Android Gradle Plugin Version: 8.1.1

Screenshots
app cash paparazzi sample_ComposeA11yTest_animateContentSize true

@drewhamilton drewhamilton added the bug Something isn't working label Dec 1, 2023
@adamalyyan
Copy link
Collaborator

I'll be able to dig into this within the next week!

@adamalyyan
Copy link
Collaborator

Wanted to provide an update on what I've found so far. animateContentSize is meant to animate size changes to a composable when a value changes on the next composition. In theory this shouldn't really have any impact since in the examples where this is reproduced, there are no size changes.

From my testing the issue only seems to happen when the contentView we want to take a snapshot of is added to a container where it is no longer the full size, which occurs with the AccessibilityRenderExtension. Here we're creating a new LinearLayout x2 the size of the original snapshot size with a weight of 2 and adding the contentView and the accessibility legend output. For some reason that I haven't yet discovered, this causes the contentView to get offset by exactly half the width.

After messing with the LayoutParams to see if there is something at play there, I was able to "solve" the issue by setting the width and height of the contentView and the accessibility legend to be the exact dimensions we expect (using the device width and height). This seems to force the content to be placed correctly in the parent LinearLayout. Previously we grabbed the layoutParams from the passed in contentView to see the width and height of the contentView inside the LinearLayout container and used MATCH_PARENT for width and height of the accessibility legend - I'm not quite sure why adding animateContentSize() causes a strange interaction with these values.

@drewhamilton
Copy link
Author

Where are you changing these params? Is that something that is feasible to do by default inside AccessibilityRenderExtension, or will that break behavior in some other case?

@adamalyyan
Copy link
Collaborator

adamalyyan commented Dec 19, 2023

I was setting these directly inside AccessibilityRenderExtension#renderView. We would need to pass in the expected size of the layout which the current RenderExtension api doesn't supply so that would be some potential breaking behavior (unless we provide a default value). Although even if we pass the size, I'm not sure if it would solve the problem when renderingMode is set to SHRINK since we then technically don't know the size of the layout until it's done a layout pass (which happens after #renderView is called, if I'm not mistaken.

Perhaps we could run a measure() call on the contentView when inside the renderExtension to get the size and then use that. I haven't tried this yet, but sounds like it could work? Looks like the composable can't be measured without being attached to a window...

@adamalyyan
Copy link
Collaborator

After many, many different trial and error attempts, I do finally have a solution that doesn't involve changing the RenderExtension api but does require 2 things to work:

  1. Composable Animations do not use the duration scale that InstantAnimationRule sets, instead they (at least animateContentSize animations) use a MotionDurationScale which is pulled from the coroutine context. We can provide an implementation of this when setting the WindowRecomposerPolicy. However this alone still doesn't fix the issue since there's still technically 2 frames involved (2 compositions happen), and the last frame is the one where the layout is actually correct.
  2. Adding a SnapshotHandler which captures the last frame when setting up the Paparazzi rule allows us to capture the final composition where the "animation" is complete and thus the snapshot is correct.

@jrodbx what are your thoughts on this. I could have a PR with a change to add a MotionDurationScale that always uses a value of 0, but thinking out loud; this might not be the best way to do it since if you want to snapshot a gif of an animation, it would disable that. I don't think this can be done via a rule either since it needs to be supplied to the coroutine context inside Paparazzi..

@drewhamilton
Copy link
Author

For 1, maybe Paparazzi could read the MotionDurationScale and apply the same value to the WindowRecomposePolicy on each snapshot? Then if the InstantAnimationRule is in use, it would be made to effectively apply to Compose UI too, regardless of whether a regular snapshot or a gif is being taken.

@adamalyyan
Copy link
Collaborator

Unfortunately I don't think there's a way for us to know within Paparazzi if InstantAnimationRule is being used since it applies independently. It may require a new parameter on the Paparazzi builder to specify instant animations which we could then apply both InstantAnimationRule and set MotionDurationScale to 0 together.

@drewhamilton
Copy link
Author

I meant checking the effects of InstantAnimationRule rather than knowing directly whether it has been applied. That would mean reading ValueAnimator.getDurationScale, probably with reflection.

@adamalyyan
Copy link
Collaborator

Oh gotcha, yeah that definitely seems like it would work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants