wordpress-mobile / gutenberg-mobile

Mobile version of Gutenberg - native iOS and Android
GNU General Public License v2.0
242 stars 55 forks source link

VideoPress block: Focus is off after opening "Playback settings" with TalkBack #5699

Open SiobhyB opened 1 year ago

SiobhyB commented 1 year ago

Describe the bug

With Talkback on and after double tapping to open "Playback settings," focus is put on the third setting in the list (“Muted”) instead of the first (“Autoplay”) or the header of the settings menu.

To Reproduce

Steps to reproduce the behavior:

  1. Navigate to the editor for a post containing a VideoPress block, or go through the steps to add the VideoPress block in the editor.
  2. Enable TalkBack.
  3. Open the block's settings and double tap on "Playback settings".
  4. Verify that the third setting in the list (“Muted”) is given the initial focus.

Expected behavior

I would expect TalkBack to focus on the first setting (“Autoplay”) or the header of the settings menu.

Smartphone (please complete the following information):

fluiddot commented 1 year ago

I'm trying to reproduce the issue and I'm not sure if I'm following the steps to reproduce it properly. When I open the block's settings and navigate to "Playback Settings", the initial focus is on the modal itself (I interpret the focus as the element with the blue border). If I navigate the items via TalkBack, they are focused in the expected order (sharing a video capture). @SiobhyB I'm wondering if you could share a video to compare with my findings. Also, I'd appreciate it if you could guide me on how to reproduce it. Thank you very much 🙇 !

https://user-images.githubusercontent.com/14905380/234628364-ed593363-50fd-4db2-ab6c-0499322ef9c7.mp4

SiobhyB commented 1 year ago

@fluiddot, that's interesting, I'm following the same steps as you, except the focus is consistently going straight to the Muted setting for me:

https://user-images.githubusercontent.com/2998162/234632984-87eda9b4-56ad-4716-be55-0ea9666ad34c.mp4

I wonder if you're you testing using a physical device? I haven't tried with a simulator. I'm using a Pixel 6 (Android 13) for testing. @lmischner, were you also using a physical device when you reproduced this issue? It'd be interesting to compare details.

fluiddot commented 1 year ago

I wonder if you're you testing using a physical device? I haven't tried with a simulator. I'm using a Pixel 6 (Android 13) for testing.

Yes, I'm using a physical device (Samsung Galaxy S20 FE 5G - Android 13). Maybe there are differences in TalkBack between device models 🤔. I'll try with a Pixel device emulator.

fluiddot commented 1 year ago

I wonder if you're you testing using a physical device? I haven't tried with a simulator. I'm using a Pixel 6 (Android 13) for testing.

Yes, I'm using a physical device (Samsung Galaxy S20 FE 5G - Android 13). Maybe there are differences in TalkBack between device models 🤔. I'll try with a Pixel device emulator.

I managed to reproduce it with the emulator. Seems either it's only related to Pixel devices or can't be reproduced on Samsung devices 🤔.

fluiddot commented 1 year ago

I played around with the Bottom sheet component and noticed that the issue seems related to sub-sheets, not specific to the VideoPress block or the "Playback settings" section. Probably we haven't bumped into this because it's the first time we have this navigation structure.

https://user-images.githubusercontent.com/14905380/234870092-efaf69f2-7dee-4035-826d-6e68b1a42167.mp4

Note in the video that the item focused on when entering each sub-sheet varies depending on the position of the original button 🙃 .

fluiddot commented 1 year ago

Findings of exploring potential workarounds

1 - Use importantForAccessibility a11y prop

Based on the documentation, this prop can be used in the case of two overlapping UI components with the same parent. Following the findings shared in this comment, I decided to try this solution in case there's any overlap between the screen that renders the InspectorControls and SubSheet, which renders the "Playback Settings".

My idea was to wrap the sub sheet content with a View and enforce Talkback to focus it:

diff --git forkSrcPrefix/projects/packages/videopress/src/client/block-editor/blocks/video/components/playback-panel/index.native.js forkDstPrefix/projects/packages/videopress/src/client/block-editor/blocks/video/components/playback-panel/index.native.js
index d5e6f6e412483d65d16b52dd0a69abed6b145ecc..6d8098a97f1f691b09a977a003a1c3105b7d8a30 100644
--- forkSrcPrefix/projects/packages/videopress/src/client/block-editor/blocks/video/components/playback-panel/index.native.js
+++ forkDstPrefix/projects/packages/videopress/src/client/block-editor/blocks/video/components/playback-panel/index.native.js
@@ -6,7 +6,7 @@ import { PanelBody, ToggleControl, BottomSheet } from '@wordpress/components';
 import { useCallback, useState } from '@wordpress/element';
 import { __ } from '@wordpress/i18n';
 import { Icon, chevronRight } from '@wordpress/icons';
-import { Text } from 'react-native';
+import { Text, View } from 'react-native';

 /**
  * Sidebar Control component.
@@ -54,7 +54,7 @@ export default function PlaybackPanel( { attributes, setAttributes } ) {
            }
            showSheet={ showSubSheet }
        >
-           <>
+           <View importantForAccessibility="yes">
                <BottomSheet.NavBar>
                    <BottomSheet.NavBar.BackButton onPress={ goBack } />
                    <BottomSheet.NavBar.Heading>
@@ -131,7 +131,7 @@ export default function PlaybackPanel( { attributes, setAttributes } ) {
                        ) }
                    />
                </PanelBody>
-           </>
+           </View>
        </BottomSheet.SubSheet>
    );
 }

The solution worked but I realized that when navigating back to the block settings, the focus was now pointing to a wrong item 😭 .

https://user-images.githubusercontent.com/14905380/235112107-f745f41b-0876-4f86-81f5-627860c8a30f.mp4

2- Sending Accessibility Events

After testing option 1, I thought that we might need to notify the focus when navigating between the sub sheet and the block settings. Fortunately, React Native provides a function to trigger this notification to Talkback via AccessibilityInfo.setAccessibilityFocus.

The idea is:

  1. When navigating to the sub sheet, focus on the content.
  2. When navigating back, focus on the navigation button.

The code can be checked in the branch: https://github.com/WordPress/gutenberg/compare/rnmobile/fix/bottomsheet-subsheet-talkback.

It worked in most cases, but still, I noticed that some of the sub sheets (e.g. Playback Bar Color) were not focusing on the expected navigation button when navigating back 😭 😭.

https://user-images.githubusercontent.com/14905380/235113715-fdfed7fc-c3ef-4373-8109-f77e74162a9f.mp4

Next

After trying the above options none have proven to be solid and solve completely the issue. My gut feeling is that the way we use React navigation, plus rendering the sub sheet's content with the Slot/Fill component, might be interfering with how Talkback picks the item to focus on when navigating to screens. Besides, I spotted some issues in the React navigation repository that mentions TalkBack. However, I tested Talkback on a simple project with React Navigation and couldn't reproduce this issue.

As a next step, I'd advocate reviewing and refactoring the navigation inside the bottom sheet component.

fluiddot commented 1 year ago

As a side note, I also managed to reproduce this issue on the Image block when navigating to the "Link To" screen. This reinforces the fact that this issue is not unique to the VideoPress block.

https://user-images.githubusercontent.com/14905380/235116547-b3169399-4194-4bf6-ab7c-7bb2718be12d.mp4

lmischner commented 1 year ago

@fluiddot, that's interesting, I'm following the same steps as you, except the focus is consistently going straight to the Muted setting for me:

device-2023-04-26-165632.mp4 I wonder if you're you testing using a physical device? I haven't tried with a simulator. I'm using a Pixel 6 (Android 13) for testing. @lmischner, were you also using a physical device when you reproduced this issue? It'd be interesting to compare details.

Looks like @fluiddot already determined this was Pixel specific (or, at the least, Samsung devices weren't affected), but wanted to confirm that, yes, I was using a physical Pixel device when I came a cross it.