-
Notifications
You must be signed in to change notification settings - Fork 214
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
Fix background color covering content and bottom sheet / dialogs rendering for accessibility #1637
Conversation
2421d19
to
aadaed0
Compare
aadaed0
to
acca5a7
Compare
/** | ||
* Needed as [android.view.WindowManagerImpl] uses the view root background color to set the WindowManagerImpl view's background color. | ||
* If we set this as transparent, the WindowManagerImpl view will be transparent as well and correctly renders the window above content. | ||
*/ | ||
setBackgroundColor(Color.TRANSPARENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change is needed due to a view ordering change in WindowManagerImpl in 14.0.11.
(line 105) https://www.diffchecker.com/fHBDkOTq/
AS diff: https://android.googlesource.com/platform/frameworks/layoutlib/+/f86a9059e4e0fbc6908c32aa88bddc640de86831%5E%21/bridge/src/android/view/WindowManagerImpl.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timestamp of that change is dated 2021, so I am skeptic of this being the root cause since we have been on later layoutlib versions. What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update: I had only clicked on the AS diff per my comment above. I just viewed the diffcheck link and that makes more sense. Is it possible you posted the wrong AS diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes the one I posted isn't correct but according to the cs.android.com that is the last revision with that line change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the history for WindowManagerImpl: https://cs.android.com/android/platform/superproject/main/+/main:frameworks/layoutlib/bridge/src/android/view/WindowManagerImpl.java;bpv=1
in case there is something we can learn from their changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I noticed in the updated snapshot it seems like the whole screen is tinted pink for the "close sheet" accessibility label, and the highlights around the drag handle and text 2 seem to be missing. Either that or it's just being colored over by the close sheet highlight. Is this something we're fine with for now and can fix later or should we try to get that resolved in this PR?
Nice find!! this is most likely due to the same change of WindowManager index in the parent (boxes in overlay are rendered behind content) |
paparazzi/src/main/java/app/cash/paparazzi/accessibility/AccessibilityRenderExtension.kt
Outdated
Show resolved
Hide resolved
paparazzi/src/main/java/app/cash/paparazzi/accessibility/AccessibilityRenderExtension.kt
Outdated
Show resolved
Hide resolved
// Window manager is rendered at the root of the view hierarchy, rendering it full width. | ||
// We need to restrict it when taking accessibility snapshots. | ||
(windowManager as WindowManagerImpl).currentRootView?.let { | ||
layoutParams = FrameLayout.LayoutParams(contentView.measuredWidth, MATCH_PARENT, Gravity.START) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, is this how layoutlib does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: will this layout change affect subsequent non-accessibility tests, i.e., do we need to revert back to full width in some sort of shutdown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, is this how layoutlib does it?
No this is specifically to get AccessibilityRenderExtension to render as expected.
question: will this layout change affect subsequent non-accessibility tests, i.e., do we need to revert back to full width in some sort of shutdown?
Hmm, we can I guess. I hadn't thought about that. Whats the use case where this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I don't know if this is needed per se, thus the question.
I'm thinking of scenarios where you run two tests, an accessibility test and a non-accessibility test, in that order, and whether the second test may render incorrectly.
Actually, since this is mutating the currentRootView, which is presumably detached/re-attached to between tests, then this shouldn't be a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly the window manager impl view is only attached when needed. So it will be removed on window detach
@@ -70,6 +81,7 @@ public class AccessibilityRenderExtension : RenderExtension { | |||
} | |||
} | |||
|
|||
@SuppressLint("VisibleForTests") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: revert
You may recall this is because we should be moving away from ViewRootForTest
/** | ||
* Needed as [android.view.WindowManagerImpl] uses the view root background color to set the WindowManagerImpl view's background color. | ||
* If we set this as transparent, the WindowManagerImpl view will be transparent as well and correctly renders the window above content. | ||
*/ | ||
setBackgroundColor(Color.TRANSPARENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timestamp of that change is dated 2021, so I am skeptic of this being the root cause since we have been on later layoutlib versions. What am I missing?
c7575df
to
e6a146c
Compare
@@ -52,7 +51,7 @@ class ComposeA11yTest { | |||
) { | |||
Text(text = "Text 2") | |||
} | |||
Text(text = "Text 1") | |||
Text(modifier = Modifier.wrapContentSize(), text = "Text 1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to better illustrate the close scrim area
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
import android.graphics.drawable.Drawable | ||
import app.cash.paparazzi.accessibility.RenderSettings.toColorInt | ||
|
||
internal class AccessibilityOverlayDrawable : Drawable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up making this a foreground drawable instead of a view so it can be drawn overtop WindowManagerImpl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
|
||
val contentLayoutParams = contentView.layoutParams ?: generateLayoutParams(null) | ||
addView(overlay, LinearLayout.LayoutParams(contentLayoutParams.width, contentLayoutParams.height, 1f)) | ||
val overlayDrawable = AccessibilityOverlayDrawable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move this down closer to the usage site
@@ -29,7 +29,7 @@ import app.cash.paparazzi.accessibility.RenderSettings.toColorInt | |||
import java.lang.Float.max | |||
|
|||
internal class AccessibilityOverlayDetailsView(context: Context) : FrameLayout(context) { | |||
private val accessibilityElements = mutableListOf<AccessibilityElement>() | |||
private var accessibilityElements: Collection<AccessibilityElement> = emptySet() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private var accessibilityElements: Collection<AccessibilityElement> = emptySet() | |
private lateinit var accessibilityElements: Collection<AccessibilityElement> |
avoids the alloc of an emptySet that's never used
override fun setColorFilter(p0: ColorFilter) = Unit | ||
|
||
@Deprecated("Not used", ReplaceWith("255")) | ||
override fun getOpacity(): Int = 255 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: PixelFormat.OPAQUE?
canvas.drawRect(it.displayBounds, strokePaint) | ||
} | ||
} | ||
override fun setAlpha(alpha: Int) = Unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
override fun setAlpha(alpha: Int) = Unit | |
override fun setAlpha(alpha: Int) = Unit |
fun addElements(elements: Collection<AccessibilityElement>) { | ||
accessibilityElements.addAll(elements) | ||
fun updateElements(elements: Collection<AccessibilityElement>) { | ||
accessibilityElements = elements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep addAll
and clear on each add, if you're looking to avoid leaking memory? this allows the list to remain a val.
import android.graphics.drawable.Drawable | ||
import app.cash.paparazzi.accessibility.RenderSettings.toColorInt | ||
|
||
internal class AccessibilityOverlayDrawable : Drawable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
@@ -52,7 +51,7 @@ class ComposeA11yTest { | |||
) { | |||
Text(text = "Text 2") | |||
} | |||
Text(text = "Text 1") | |||
Text(modifier = Modifier.wrapContentSize(), text = "Text 1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
/** | ||
* Needed as [android.view.WindowManagerImpl] uses the view root background color to set the WindowManagerImpl view's background color. | ||
* If we set this as transparent, the WindowManagerImpl view will be transparent as well and correctly renders the window above content. | ||
*/ | ||
setBackgroundColor(Color.TRANSPARENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the history for WindowManagerImpl: https://cs.android.com/android/platform/superproject/main/+/main:frameworks/layoutlib/bridge/src/android/view/WindowManagerImpl.java;bpv=1
in case there is something we can learn from their changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it expected that the text centering is now different in the background view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya I moved that to better capture the text bounds and the close
tap area of the dialog. Before they were overlapping completely
e6a146c
to
fd0704e
Compare
Closes #1634. Related to #1577 and #1414.