Skip to content

Commit 77d2138

Browse files
Fix Modal not invoking onRequestClose on hardware ESC key on Android
- Root cause: the platform's default Dialog.onKeyUp routed KEYCODE_ESCAPE straight to onBackPressed, dismissing the dialog before our OnKeyListener observed ACTION_UP, so onRequestClose never fired and React state stayed stuck at visible=true. - Fix: consume both ACTION_DOWN and ACTION_UP (return true) for KEYCODE_BACK/KEYCODE_ESCAPE in ReactModalHostView's dialog OnKeyListener, while still invoking handleCloseAction() exactly once on ACTION_UP. Other keys continue to flow through to currentActivity.onKeyUp on ACTION_UP only. - Adds Robolectric regression tests (escapeKeyOnDialog_invokesOnRequestCloseListener, backKeyOnDialog_invokesOnRequestCloseListener) that dispatch DOWN+UP through the real dialog and assert the OnRequestCloseListener fires exactly once. Fixes #56411
1 parent ccff70b commit 77d2138

2 files changed

Lines changed: 115 additions & 16 deletions

File tree

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/modal/ReactModalHostView.kt

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -305,23 +305,27 @@ public class ReactModalHostView(context: ThemedReactContext) :
305305
newDialog.setOnKeyListener(
306306
object : DialogInterface.OnKeyListener {
307307
override fun onKey(dialog: DialogInterface, keyCode: Int, event: KeyEvent): Boolean {
308-
if (event.action == KeyEvent.ACTION_UP) {
309-
// We need to stop the BACK button and ESCAPE key from closing the dialog by default
310-
// so we capture that event and instead inform JS so that it can make the decision as
311-
// to whether or not to allow the back/escape key to close the dialog. If it chooses
312-
// to, it can just set visible to false on the Modal and the Modal will go away
313-
if (keyCode == KeyEvent.KEYCODE_BACK || keyCode == KeyEvent.KEYCODE_ESCAPE) {
308+
// We need to stop the BACK button and ESCAPE key from closing the dialog by default
309+
// so we capture that event and instead inform JS so that it can make the decision as
310+
// to whether or not to allow the back/escape key to close the dialog. If it chooses
311+
// to, it can just set visible to false on the Modal and the Modal will go away.
312+
// We consume both ACTION_DOWN and ACTION_UP for these keys so that the platform
313+
// cannot dismiss the dialog via a separate path (e.g. Dialog.onKeyUp routing
314+
// KEYCODE_ESCAPE directly to onBackPressed) before our listener observes ACTION_UP.
315+
if (keyCode == KeyEvent.KEYCODE_BACK || keyCode == KeyEvent.KEYCODE_ESCAPE) {
316+
if (event.action == KeyEvent.ACTION_UP) {
314317
handleCloseAction()
315-
return true
316-
} else {
317-
// We redirect the rest of the key events to the current activity, since the
318-
// activity expects to receive those events and react to them, ie. in the case of
319-
// the dev menu
320-
val innerCurrentActivity =
321-
(this@ReactModalHostView.context as ReactContext).currentActivity
322-
if (innerCurrentActivity != null) {
323-
return innerCurrentActivity.onKeyUp(keyCode, event)
324-
}
318+
}
319+
return true
320+
}
321+
if (event.action == KeyEvent.ACTION_UP) {
322+
// We redirect the rest of the key events to the current activity, since the
323+
// activity expects to receive those events and react to them, ie. in the case of
324+
// the dev menu
325+
val innerCurrentActivity =
326+
(this@ReactModalHostView.context as ReactContext).currentActivity
327+
if (innerCurrentActivity != null) {
328+
return innerCurrentActivity.onKeyUp(keyCode, event)
325329
}
326330
}
327331
return false
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
// TODO T207169925: Migrate CatalystInstance to Reacthost and remove the Suppress("DEPRECATION")
9+
// annotation
10+
@file:Suppress("DEPRECATION")
11+
12+
package com.facebook.react.views.modal
13+
14+
import android.app.Activity
15+
import android.view.KeyEvent
16+
import androidx.core.content.res.ResourcesCompat.ID_NULL
17+
import com.facebook.react.bridge.BridgeReactContext
18+
import com.facebook.react.bridge.CatalystInstance
19+
import com.facebook.react.bridge.ReactTestHelper.createMockCatalystInstance
20+
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests
21+
import com.facebook.react.uimanager.ThemedReactContext
22+
import org.assertj.core.api.Assertions.assertThat
23+
import org.junit.Before
24+
import org.junit.Test
25+
import org.junit.runner.RunWith
26+
import org.robolectric.Robolectric
27+
import org.robolectric.RobolectricTestRunner
28+
import org.robolectric.RuntimeEnvironment
29+
import org.robolectric.android.controller.ActivityController
30+
31+
/**
32+
* Verifies that hardware key events on the modal's dialog are routed to the registered
33+
* [ReactModalHostView.OnRequestCloseListener]. Regression test for issue #56411 where pressing the
34+
* hardware ESC key on Android (e.g. external/Bluetooth keyboard) closed the dialog without ever
35+
* invoking the JS `onRequestClose` callback.
36+
*/
37+
@RunWith(RobolectricTestRunner::class)
38+
class ReactModalHostViewTest {
39+
40+
private lateinit var reactContext: BridgeReactContext
41+
private lateinit var catalystInstanceMock: CatalystInstance
42+
private lateinit var activityController: ActivityController<Activity>
43+
private lateinit var themedContext: ThemedReactContext
44+
45+
@Before
46+
fun setUp() {
47+
ReactNativeFeatureFlagsForTests.setUp()
48+
49+
catalystInstanceMock = createMockCatalystInstance()
50+
reactContext = BridgeReactContext(RuntimeEnvironment.getApplication())
51+
reactContext.initializeWithInstance(catalystInstanceMock)
52+
53+
activityController = Robolectric.buildActivity(Activity::class.java).create().start().resume()
54+
// Attach the activity to the React context so ReactModalHostView can locate it via
55+
// `currentActivity` when constructing the dialog.
56+
reactContext.onHostResume(activityController.get())
57+
58+
themedContext = ThemedReactContext(reactContext, activityController.get(), null, ID_NULL)
59+
}
60+
61+
@Test
62+
fun escapeKeyOnDialog_invokesOnRequestCloseListener() {
63+
val view = ReactModalHostView(themedContext)
64+
var requestCloseInvocations = 0
65+
view.onRequestCloseListener =
66+
ReactModalHostView.OnRequestCloseListener { requestCloseInvocations++ }
67+
68+
// Build the underlying ComponentDialog and register the modal's key handlers.
69+
view.showOrUpdate()
70+
val dialog = checkNotNull(view.dialog) { "Dialog should have been created by showOrUpdate" }
71+
72+
// Dispatching ACTION_DOWN + ACTION_UP for KEYCODE_ESCAPE through the dialog should be treated
73+
// the same as KEYCODE_BACK and trigger onRequestClose exactly once.
74+
dialog.dispatchKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_ESCAPE))
75+
dialog.dispatchKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_ESCAPE))
76+
77+
assertThat(requestCloseInvocations).isEqualTo(1)
78+
}
79+
80+
@Test
81+
fun backKeyOnDialog_invokesOnRequestCloseListener() {
82+
val view = ReactModalHostView(themedContext)
83+
var requestCloseInvocations = 0
84+
view.onRequestCloseListener =
85+
ReactModalHostView.OnRequestCloseListener { requestCloseInvocations++ }
86+
87+
view.showOrUpdate()
88+
val dialog = checkNotNull(view.dialog) { "Dialog should have been created by showOrUpdate" }
89+
90+
dialog.dispatchKeyEvent(KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_BACK))
91+
dialog.dispatchKeyEvent(KeyEvent(KeyEvent.ACTION_UP, KeyEvent.KEYCODE_BACK))
92+
93+
assertThat(requestCloseInvocations).isEqualTo(1)
94+
}
95+
}

0 commit comments

Comments
 (0)