fix(Android, Stack v4): fix keyboard navigation focus for form sheet#3245
fix(Android, Stack v4): fix keyboard navigation focus for form sheet#3245
Conversation
kkafar
left a comment
There was a problem hiding this comment.
Okay, so important thing here is that the form sheet on Android is not a modal view byt it's own.
We use something called "standard bottom sheet" & it is a set of views attached directly to the hierarchy in place where the Screen component is mounted. It does not behave like modal: it does not create separate DecorView & does not mount above all contents (that's something we'd like to support in next major).
Therefore I'm not all for emulating all modal features here.
What we could do is to expose a prop Android only prop to control this. OR alternatively block the focusability ONLY when dimming view is present. The presence of dimming view indicates to the user that he should focus on the sheets contents utterly, not on the background.
So yeah, what do you think @kligarski? Does that sound sensible?
|
Are there any news on this matter...? We'd still like to use the form sheets, but this focus issue is a blocker for us. Thanks! 🙏 |
c8602b6 to
168529d
Compare
Hi, sorry for the delay. I've updated the PR and hopefully we'll proceed soon. |
kligarski
left a comment
There was a problem hiding this comment.
While implementing the fix, I noticed some unrelated issues (also appearing on main):
| // Note: There's no good reason that Screen should be direct target for focus, we're rather | ||
| // prefer its children to gain it. | ||
| screen.descendantFocusability = ViewGroup.FOCUS_AFTER_DESCENDANTS |
There was a problem hiding this comment.
This should be handled now with a11y so I removed it here.
| if (isNativeStackScreen) { | ||
| (container as ScreenStack).updateA11yForVisibleScreens() | ||
| } |
There was a problem hiding this comment.
This is ugly & I'm open for suggestions here how to handle this case.
There was a problem hiding this comment.
If we stick to that, we need a more descriptive comment
There was a problem hiding this comment.
Let me know if this is what you meant:
| .dropWhile { it.isTranslucent() } | ||
| .firstOrNull() | ||
| ?.takeUnless { it === newTop } | ||
| currentVisibleBottom = visibleBottom |
There was a problem hiding this comment.
I need visibleBottom in updateA11yForVisibleScreens and now it might be also called outside of onUpdate. When I tried to use only the property, I faced errors related to concurrent mutability so I kept local variable in the method. If you have any suggestions here, let me know.
| fragmentWrapper.screen.changeAccessibilityMode(accessibilityMode) | ||
|
|
||
| // Keyboard navigation focus is separate from screen reader focus, that's | ||
| // why we need to use focusable and descendantFocusability. | ||
| changeScreenFocusability(fragmentWrapper.screen, !shouldDisableFocusability) |
There was a problem hiding this comment.
As the dimming view might appear and disappear with detent changes, we need to set and reset the focusability.
There was a problem hiding this comment.
I don't see how to improve the code in any major way. Left some nitpicks. But I also found some bugs.
- You can still move focus under dimming view. When you go to second sheet & back, the focus is correct.
Nagranie.z.ekranu.2026-02-16.o.10.17.15.mov
- Here, you can move everywhere but to the "go back" button.
Nagranie.z.ekranu.2026-02-16.o.10.31.45.mov
| if (isNativeStackScreen) { | ||
| (container as ScreenStack).updateA11yForVisibleScreens() | ||
| } |
There was a problem hiding this comment.
If we stick to that, we need a more descriptive comment
| if (it.screen.usesFormSheetPresentation()) { | ||
| val screenStackFragment = it.fragment.asScreenStackFragment() | ||
| screenStackFragment.sheetDelegate?.let { delegate -> | ||
| return delegate.lastStableDetentIndex > it.screen.sheetLargestUndimmedDetentIndex | ||
| } | ||
| } else { | ||
| return it.isTranslucent() | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
| if (it.screen.usesFormSheetPresentation()) { | |
| val screenStackFragment = it.fragment.asScreenStackFragment() | |
| screenStackFragment.sheetDelegate?.let { delegate -> | |
| return delegate.lastStableDetentIndex > it.screen.sheetLargestUndimmedDetentIndex | |
| } | |
| } else { | |
| return it.isTranslucent() | |
| } | |
| } | |
| return false | |
| if (!it.screen.usesFormSheetPresentation()) { | |
| return it.isTranslucent() | |
| } | |
| val screenStackFragment = it.fragment.asScreenStackFragment() | |
| screenStackFragment.sheetDelegate?.let { delegate -> | |
| return delegate.lastStableDetentIndex > it.screen.sheetLargestUndimmedDetentIndex | |
| } | |
| } | |
| return false |
nit: I would like to reduce nesting whenever possible, but do as you see fit.
There was a problem hiding this comment.
While fixing the bug you mentioned in review description, I managed to refactor this function:
Thank you for checking this. I have no idea how I missed this as 1. is the basic test case I used when adding the change, maybe I missed this in one of the refactors. It turns out that Screen_recording_20260216_121731.mp4 |
There was a problem hiding this comment.
Pull request overview
This PR fixes keyboard navigation focus handling for form sheets on Android by preventing focus from reaching elements in views below the form sheet. Previously, keyboard navigation could incorrectly focus elements in obstructed screens.
Changes:
- Added logic to control focusability and descendant focusability based on whether a dimming view is present for form sheets
- Introduced helper methods to set focusability properties for different Android API levels (≤25 and ≥26)
- Made DimmingView non-focusable to prevent focus issues when navigating between screens
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| SheetUtils.kt | Added helper function to determine if a form sheet should use a dimming view based on detent index |
| SheetDelegate.kt | Removed redundant descendantFocusability setting (now handled centrally in ScreenStack) |
| DimmingView.kt | Set DimmingView to be non-focusable to prevent focus capture |
| ScreenStack.kt | Refactored accessibility logic to also handle keyboard navigation focusability, including API-level-specific handling |
| Screen.kt | Added methods to change focusability for API ≥26 and ≤25, and trigger accessibility updates when sheet detent changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| topScreen?.let { changeScreenFocusability(it, true) } | ||
| } | ||
|
|
||
| private fun shouldDisableFocusabilityForVisibleScreens(): Boolean { |
There was a problem hiding this comment.
The function name shouldDisableFocusabilityForVisibleScreens is misleading because it returns a boolean indicating whether to disable focusability, but it's only checking the top screen's properties. A more accurate name would be shouldDisableFocusabilityBeneathTopScreen or topScreenBlocksFocusBeneath.
There was a problem hiding this comment.
I changed it to shouldDisableFocusabilityBeneathTopScreen, in theory it's still not precise enough as we don't disable all screens beneath top but only screens that are visible but new name is probably better than the previous one. Open for suggestions as well.
Description
Fixes keyboard navigation focus handling for form sheets on Android.
Previously, it was possible to focus elements in the view displayed below the form sheet.
Closes #3188.
Closes https://github.com/software-mansion/react-native-screens-labs/issues/929.
focus_before.mp4
focus_withRequestFocus.mp4
Context
In
ScreenStack, there is already logic to handle accessibility focus viaimportantForAccessibilityprop.I stumbled upon an issue in
react-nativerepo where one of the users explained that Android handles screen readers focus and keyboard navigation focus differently. In order to block keyboard navigation focus, you need to setfocusableanddescendantFocusabilityproperties - when implemented inScreenStack, this started working correctly (after clickingarrow-down, focus did not leave the formsheet).As form sheet isn't a modal form of presentation in current Android implementation, we decided to block the focusability and set
importantForAccessibilitytonowhen dimming view is present.Important
Change above might be considered a breaking change -
importantForAccessibilityis not set tonoif no dimming view is present.One problem I noticed is that due to usage of
requestFocus()introduced in #3617, input is focused instead of e.g. the first button on the screen (as seen in video at the top of PR description; note that no IME appears). Prior to changes in 3617, I experimented with clearing focus on previous screen (look at the video below) but this solution breaks fix introduced in 3617 so I decided to keeprequestFocus.focus_after.mp4
Also, if form sheet is opened without any focusable elements, the focus is not cleared from the obstructed screen (clearing focus might help with this) but this is not a regression so I decided to leave it for now.
Screen_recording_20260213_145846.mp4
Let me know if this is something that we need to address and add to our board (both incorrect view being focused & not clearing focus when no focusable elements are present in the sheet).
Also note that to match logic introduced in 3617, I decided to set
descendantFocusabilitytoFOCUS_AFTER_DESCENDANTS. This however isn't the default:But due to screen not being focusable (I think that
AUTOfocusability handles it for us), there should be no changes introduced by this.Another thing I noticed is that when you go back from a screen, nothing is focused (I checked with layout inspector) - this is not a regression but we should have a look at it in the future. Native app (settings) on API 36 keeps focus when screen is popped. I created an issue to investigate this: https://github.com/software-mansion/react-native-screens-labs/issues/474.
Changing focusability on Android
Starting from API 26,
focusablecan be set toNOT_FOCUSABLE/FOCUSABLE/FOCUSABLE_AUTO. Prior to API 26, this was a boolean prop - that's why there is some extra code to handle both cases (changeFocusabilityandchangeFocusabilityCompatfunctions inScreen.kt).Changes
Screenfor API <= 25 and API >= 26importantForAccessibilitydepending on dimming viewfocusabletofalse/NOT_FOCUSABLEforDimmingView(this caused missing focus after popping apushscreen that was pushed over form sheet)Test code and steps to reproduce
Run
TestFormSheetand use the keyboard to navigate. CheckTest3617for regressions.Checklist