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

Fix background color covering content and bottom sheet / dialogs rendering for accessibility #1637

Merged
merged 4 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.wrapContentSize
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.ModalBottomSheet
import androidx.compose.material3.SheetState
Expand All @@ -19,7 +20,6 @@ import androidx.compose.ui.unit.dp
import app.cash.paparazzi.DeviceConfig
import app.cash.paparazzi.Paparazzi
import app.cash.paparazzi.accessibility.AccessibilityRenderExtension
import org.junit.Ignore
import org.junit.Rule
import org.junit.Test

Expand All @@ -37,7 +37,6 @@ class ComposeA11yTest {
paparazzi.snapshot(mixedView)
}

@Ignore("LayoutLib changed order of window manager views render. Needs investigation https://github.com/cashapp/paparazzi/issues/1634")
@Test
@OptIn(ExperimentalMaterial3Api::class)
fun modalBottomSheetMaterial3() {
Expand All @@ -52,7 +51,7 @@ class ComposeA11yTest {
) {
Text(text = "Text 2")
}
Text(text = "Text 1")
Text(modifier = Modifier.wrapContentSize(), text = "Text 1")
Copy link
Collaborator Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

}
}

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -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 val accessibilityElements = mutableSetOf<AccessibilityElement>()
private val paint = Paint().apply {
isAntiAlias = true
style = Paint.Style.FILL
Expand All @@ -51,8 +51,9 @@ internal class AccessibilityOverlayDetailsView(context: Context) : FrameLayout(c
setBackgroundColor(RenderSettings.DEFAULT_DESCRIPTION_BACKGROUND_COLOR.toColorInt())
}

fun addElements(elements: Collection<AccessibilityElement>) {
accessibilityElements.addAll(elements)
fun updateElements(elements: Collection<AccessibilityElement>) {
accessibilityElements.clear()
accessibilityElements += elements
invalidate()
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package app.cash.paparazzi.accessibility

import android.graphics.Canvas
import android.graphics.ColorFilter
import android.graphics.Paint
import android.graphics.PixelFormat
import android.graphics.drawable.Drawable
import app.cash.paparazzi.accessibility.RenderSettings.toColorInt

internal class AccessibilityOverlayDrawable : Drawable() {
Copy link
Collaborator Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

private var accessibilityElements = mutableSetOf<AccessibilityElement>()
private val paint = Paint().apply {
isAntiAlias = true
style = Paint.Style.FILL
}
private val strokePaint = Paint().apply {
strokeWidth = 2f
style = Paint.Style.STROKE
}

fun updateElements(elements: Collection<AccessibilityElement>) {
accessibilityElements.clear()
accessibilityElements += elements
invalidateSelf()
}

override fun draw(canvas: Canvas) {
accessibilityElements.forEach {
paint.color = it.color.toColorInt()

canvas.drawRect(it.displayBounds, paint)

strokePaint.color = it.color.toColorInt()
strokePaint.alpha = RenderSettings.DEFAULT_RENDER_ALPHA * 2
canvas.drawRect(it.displayBounds, strokePaint)
}
}

override fun setAlpha(alpha: Int) = Unit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
override fun setAlpha(alpha: Int) = Unit
override fun setAlpha(alpha: Int) = Unit

override fun setColorFilter(p0: ColorFilter) = Unit

@Deprecated("Not used", ReplaceWith("255"))
override fun getOpacity(): Int = PixelFormat.OPAQUE
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package app.cash.paparazzi.accessibility

import android.graphics.Rect
import android.view.Gravity
import android.view.View
import android.view.View.VISIBLE
import android.view.ViewGroup
Expand All @@ -39,33 +40,41 @@ public class AccessibilityRenderExtension : RenderExtension {
override fun renderView(
contentView: View
): View {
// WindowManager needed to access accessibility elements for views that draw to other windows.
val windowManager = contentView.context.getSystemService(WindowManager::class.java)

return LinearLayout(contentView.context).apply {
orientation = LinearLayout.HORIZONTAL
weightSum = 2f
layoutParams = ViewGroup.LayoutParams(MATCH_PARENT, MATCH_PARENT)

val overlay = AccessibilityOverlayView(context).apply {
addView(contentView, FrameLayout.LayoutParams(MATCH_PARENT, MATCH_PARENT))
}

val contentLayoutParams = contentView.layoutParams ?: generateLayoutParams(null)
addView(overlay, LinearLayout.LayoutParams(contentLayoutParams.width, contentLayoutParams.height, 1f))
addView(contentView, LinearLayout.LayoutParams(MATCH_PARENT, MATCH_PARENT, 1f))

val overlayDetailsView = AccessibilityOverlayDetailsView(context)
addView(overlayDetailsView, LinearLayout.LayoutParams(MATCH_PARENT, MATCH_PARENT, 1f))

OneShotPreDrawListener.add(this) {
// Window Manager needed to access accessibility elements for views that draw to other windows
val windowManager = context.getSystemService(WindowManager::class.java)
val overlayDrawable = AccessibilityOverlayDrawable()
viewTreeObserver.addOnGlobalLayoutListener {
// The root of the view hierarchy is rendered at full width.
// We need to restrict it when taking accessibility snapshots.
val windowManagerRootView = (windowManager as WindowManagerImpl).currentRootView

val elements = buildList {
windowManagerRootView?.processAccessibleChildren { add(it) }
processAccessibleChildren { add(it) }
if (windowManagerRootView != null) {
windowManagerRootView.foreground = overlayDrawable
windowManagerRootView.layoutParams =
FrameLayout.LayoutParams(contentView.measuredWidth, MATCH_PARENT, Gravity.START)
} else {
[email protected] = overlayDrawable
}

overlayDetailsView.addElements(elements)
overlay.addElements(elements)
OneShotPreDrawListener.add(this@apply) {
val elements = buildSet {
windowManagerRootView?.processAccessibleChildren { add(it) }
processAccessibleChildren { add(it) }
}

overlayDrawable.updateElements(elements)
overlayDetailsView.updateElements(elements)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package app.cash.paparazzi.internal

import android.content.Context
import android.graphics.Color
import android.util.AttributeSet
import android.widget.FrameLayout

Expand All @@ -13,4 +14,12 @@ import android.widget.FrameLayout
internal class ComposeViewAdapter(
context: Context,
attrs: AttributeSet
) : FrameLayout(context, attrs)
) : FrameLayout(context, attrs) {
init {
/**
* 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)
Comment on lines +19 to +23
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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!

}
}