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.

Sign up for GitHub

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

Jump to bottom

Add TYPE_VIEW_HOVER_ENTER to AccessibilityNodeInfo sendAccessibilityEvent #34969

Closed
wants to merge 31 commits into from
Closed

Add TYPE_VIEW_HOVER_ENTER to AccessibilityNodeInfo sendAccessibilityEvent #34969

Changes from 19 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
fd0923e
adding accessibilityTitle prop to ReactAndroid
fabOnReact Oct 13, 2022
accbbfc
improve types in java and js
fabOnReact Oct 17, 2022
17d7827
implementing alternative solution for iOS/Android
fabOnReact Oct 24, 2022
5e1e72f
Merge branch 'main' into modal-title
fabOnReact Oct 24, 2022
42ab164
moving Title logic to modal rn-tester example
fabOnReact Oct 25, 2022
5c7ed5e
removing changes from Modal
fabOnReact Oct 25, 2022
98b4a94
fix issues with sendAccessibilityEvent
fabOnReact Oct 25, 2022
0fe1e3e
refactor useTimer with useEffect
fabOnReact Oct 25, 2022
4213491
minor fix to example
fabOnReact Oct 25, 2022
f89a01f
fix ref flow type in example
fabOnReact Oct 26, 2022
9e7fe9f
eslint fix check
fabOnReact Oct 26, 2022
129d152
minor change
fabOnReact Oct 26, 2022
036e4b5
Merge branch 'main' into modal-title
fabOnReact Oct 26, 2022
8a25a6f
minor change
fabOnReact Oct 26, 2022
dbd01c6
minor change
fabOnReact Oct 26, 2022
3c0f531
avoid passing vars to setTimeout
fabOnReact Oct 26, 2022
23bf335
Merge branch 'main' into modal-title
fabOnReact Nov 18, 2022
43e2ffa
remove sendAccessibilityEvent setTimeout as not required on iOS
fabOnReact Nov 18, 2022
8f2e0f0
adding flowfixme
fabOnReact Nov 18, 2022
d351891
Merge branch 'main' into modal-title
fabOnReact Nov 28, 2022
3126e49
Add viewHoverEnter to sendAccessibilityEventFromJS
fabOnReact Nov 28, 2022
ef5e490
remove state modalOpened
fabOnReact Nov 28, 2022
4cf1605
remove state modalOpened
fabOnReact Nov 28, 2022
e7f0023
adding viewHoverInfo to AccessiblityInfo
fabOnReact Nov 28, 2022
fd152d2
minor change
fabOnReact Nov 28, 2022
bf37a34
using correct flow type
fabOnReact Nov 28, 2022
dc4c54e
remove example
fabOnReact Nov 29, 2022
69f6a18
Merge branch 'main' into modal-title
fabOnReact Nov 29, 2022
a5c0cd8
Merge branch 'main' into modal-title
fabOnReact Feb 13, 2023
8debb60
remove AccInfo.flow.js
fabOnReact Feb 13, 2023
b5f3b21
Merge branch 'main' into modal-title
fabOnReact Feb 21, 2023
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
60 changes: 58 additions & 2 deletions packages/rn-tester/js/examples/Modal/ModalPresentation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,22 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict-local
* @flow
* @format
*/

/* eslint-disable no-alert */

import * as React from 'react';
import {Modal, Platform, StyleSheet, Switch, Text, View} from 'react-native';
import {
AccessibilityInfo,
Modal,
Platform,
StyleSheet,
Switch,
Text,
View,
} from 'react-native';
import type {RNTesterModuleExample} from '../../types/RNTesterTypes';
import RNTOption from '../../components/RNTOption';
const RNTesterButton = require('../../components/RNTesterButton');
Expand All @@ -35,6 +43,23 @@ const presentationStyles = [
const iOSActions = ['None', 'On Dismiss', 'On Show'];
const noniOSActions = ['None', 'On Show'];

const TitleComponent = React.forwardRef((props, forwardedRef) => {
return (
<Text
ref={forwardedRef}
style={{
width: '100%',
position: 'absolute',
top: 500,
textAlign: 'center',
backgroundColor: 'red',
zIndex: 20,
}}>
My custom title
</Text>
);
});

function ModalPresentation() {
const [animationType, setAnimationType] = React.useState('none');
const [transparent, setTransparent] = React.useState(false);
Expand All @@ -45,11 +70,14 @@ function ModalPresentation() {
React.useState('fullScreen');
const [supportedOrientationKey, setSupportedOrientationKey] =
React.useState('Portrait');
const [modalOpened, setModalOpened] = React.useState(null);
const [currentOrientation, setCurrentOrientation] = React.useState('unknown');
const [action, setAction] = React.useState('None');
let ref = React.useRef<?React.ElementRef<typeof Text>>(null);
const actions = Platform.OS === 'ios' ? iOSActions : noniOSActions;
const onDismiss = () => {
setVisible(false);
setModalOpened(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to leverage visible state instead of creating a new state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryancat Thanks. I moved the example to the documentation facebook/react-native-website#3438

if (action === 'onDismiss') {
alert('onDismiss');
}
Expand All @@ -59,7 +87,34 @@ function ModalPresentation() {
if (action === 'onShow') {
alert('onShow');
}
if (ref != null) {
setModalOpened(true);
}
};

React.useEffect(() => {
let timer;
if (ref != null && modalOpened === true) {
if (Platform.OS === 'ios') {
// $FlowFixMe
AccessibilityInfo.sendAccessibilityEvent(ref.current, 'focus');
} else {
// see https://github.com/facebook/react-native/issues/30097#issuecomment-1285927266
timer = setTimeout(() => {
if (ref.current != null) {
AccessibilityInfo.sendAccessibilityEvent(ref.current, 'focus');
}
}, 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's usually a red flag when I see setTimeout has to be used to work around async mounting issue. Is this something we can useLayoutEffect?

Copy link
Contributor Author

@fabOnReact fabOnReact Nov 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ryancat @blavalla My solution would be to modify the native implementation of sendAccessibilityEvent, to fix the original Talkback issue. The issue is #30097, but it is not in my sprint.

The Pull Request was maybe meant to be an API proposal.
I published the API proposal to receive feedback on my comment #34969 (comment).

I started testing a solution to the native method sendAccessibilityEvent in commit 17d7827#diff-d3d0a4709d26919cf2a549febf6cdb755a1274a489078154a80c04d4ba82e590R969-R980, I can publish and improve the solution in a separate PR (for ex. an alternative implementation of sendAccessibilityEvent in SurfaceMountingManager). The possible solutions are:

  • adding a method sendAccessibilityEventWithDelay
  • Implementing a queue mechanism to repeat the interrupted announcement (example included here, original comment).
  • adding lifecycle callbacks (for ex. afterFirstTalkbackFocus, which would trigger after TalkBack first focus) (similar issue with onMomentumScrollEnd, videos)

I can also further work improving this example or publish an example in the docs, as you suggest in the code-review. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree that generally speaking setTimeout is a red flag, in this case we're actually replicating behavior internal to Talkback that works the same way.

When Talkback gets a new "TYPE_WINDOW_STATE_CHANGED" event, it actually waits 550ms before notifying the rest of the system about it, to make sure that the window itself is in a final, idle, state. It also has an up to 1110ms delay while waiting for the screen to be stabilized before responding to that change. So this sort of setTimeout approach is unfortunately not uncommon in the accessibility space.

Since we're in Javascript here, we can't just send the native Android event here, but if there is a solution on the native Android side I think simply firing a "TYPE_WINDOW_STATE_CHANGED" when the modal appears would result in the same behavior of focus moving into it after a slight (talkback controlled) delay. This would leave the ugly setTimeout code to Talkback itself, so that we don't end up in a weird race condition later on if Talkback changes its internal logic.

Some code pointers that may be helpful:

Copy link
Contributor Author

@fabOnReact fabOnReact Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@blavalla Thanks. You are correct. I fixed the issue by adding AccessibilityEvent.TYPE_VIEW_HOVER_ENTER to the API sendAccessibilityEvent. More info in #34969 (comment)

}
}

return () => {
if (timer) {
clearTimeout(timer);
}
};
}, [modalOpened]);

/* $FlowFixMe[missing-local-annot] The type annotation(s) required by Flow's
* LTI update could not be added via codemod */
const onOrientationChange = event =>
Expand Down Expand Up @@ -87,6 +142,7 @@ function ModalPresentation() {
onOrientationChange={onOrientationChange}
onDismiss={onDismiss}
onShow={onShow}>
{TitleComponent != null && <TitleComponent ref={ref} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

When TitleComponent will be null? I think it's created unconditionally in line 46?

<View style={[styles.modalContainer, modalBackgroundStyle]}>
<View
style={[
Expand Down