Skip to content

Bug 2045749: Add game in new dialog to prevent talkback issues.#290

Closed
pollym wants to merge 1 commit into
mozilla-firefox:autolandfrom
pollym:fix-talkback-visibility-issue
Closed

Bug 2045749: Add game in new dialog to prevent talkback issues.#290
pollym wants to merge 1 commit into
mozilla-firefox:autolandfrom
pollym:fix-talkback-visibility-issue

Conversation

@pollym

@pollym pollym commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

I've also changed the feature API to pass in Context instead of the container ViewGroup.
I don't love passing context as a parameter, but it is the thing we need to spawn a new dialog, so it makes more sense than passing in a ViewGroup and getting the Context from it.

there is a try here

@github-actions

Copy link
Copy Markdown
Contributor

View this pull request in Lando to land it once approved.

@lando-web lando-web Bot requested a review from a team June 12, 2026 15:53
@mozilla-code-review

Copy link
Copy Markdown

No new issues detected. This pull request is 🆗

@pollym pollym force-pushed the fix-talkback-visibility-issue branch from 6901793 to 1952908 Compare June 15, 2026 06:41
@mozilla-code-review

Copy link
Copy Markdown

No new issues detected. This pull request is 🆗

@segunfamisa segunfamisa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code-wise, this looks good to me. I don't know why this fixed it though, or what other options could have helped here - I wonder if requesting focus for the game would bring the screen reader's focus there and it won't get stuck in the background?

Either way, this looks good to me, and thanks for handling the activity clean up!

},
)

// On destroy, dismiss the window with the host so it can't leak if the activity goes away mid-game.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👌🏾

@pollym

pollym commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Code-wise, this looks good to me. I don't know why this fixed it though, or what other options could have helped here - I wonder if requesting focus for the game would bring the screen reader's focus there and it won't get stuck in the background?

Either way, this looks good to me, and thanks for handling the activity clean up!

that's fair. i would like to spend a bit of time looking at alternatives here for sure, there are probably nicer ways to handle it!
i probably didn't explain very well... the accessibility issue was happening because there were a bunch of other views that were siblings of the game composeview in the view hierarchy. it basically gets added into whatever view you pass in. this was a bit of a hangover from when i was playing around with putting it in smaller-than-fullscreen views, so you could embed it wherever you liked.
but we don't actually do this now, it's a full screen overlay. So making it a dialog makes sense, and it fixes the issue because it's shown on top of the other content rather than as a sibling. even though you couldn't see the other content visually, they were still visible for traversal for accessibility services.
claude suggested making the other views invisible for accessibility and then visible again when you dismiss the game, but i didn't really like that approach, made me nervous that we would miss the re-visible-ising 😅

@lando-worker

lando-worker Bot commented Jun 15, 2026

Copy link
Copy Markdown

Pull request closed by commit 6fb0376

lando-worker Bot pushed a commit that referenced this pull request Jun 15, 2026
…unfamisa

I've also changed the feature API to pass in `Context` instead of the container `ViewGroup`.
I don't love passing context as a parameter, but it is the thing we need to spawn a new dialog, so it makes more sense than passing in a `ViewGroup` and getting the `Context` from it.

[there is a try here](https://treeherder.mozilla.org/jobs?repo=try&revision=eddb7ec074a0856f90d1a1c2fa4ca10c4a6db99d)

Pull request: #290
@lando-worker lando-worker Bot closed this Jun 15, 2026
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jun 16, 2026
…unfamisa

I've also changed the feature API to pass in `Context` instead of the container `ViewGroup`.
I don't love passing context as a parameter, but it is the thing we need to spawn a new dialog, so it makes more sense than passing in a `ViewGroup` and getting the `Context` from it.

[there is a try here](https://treeherder.mozilla.org/jobs?repo=try&revision=eddb7ec074a0856f90d1a1c2fa4ca10c4a6db99d)

Pull request: mozilla-firefox/firefox#290

UltraBlame original commit: 903f31d691887c84e64461976043131d46827e82
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jun 16, 2026
…unfamisa

I've also changed the feature API to pass in `Context` instead of the container `ViewGroup`.
I don't love passing context as a parameter, but it is the thing we need to spawn a new dialog, so it makes more sense than passing in a `ViewGroup` and getting the `Context` from it.

[there is a try here](https://treeherder.mozilla.org/jobs?repo=try&revision=eddb7ec074a0856f90d1a1c2fa4ca10c4a6db99d)

Pull request: mozilla-firefox/firefox#290

UltraBlame original commit: 903f31d691887c84e64461976043131d46827e82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants