From a3a7ab2cf0bc171cdc07896d9fd7bccffa65fa52 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Sun, 27 Aug 2023 04:42:42 -0700 Subject: [PATCH] Mobile: Fixes #8707: Fix not all dropdown items focusable with VoiceOver (#8714) --- .eslintignore | 1 + .gitignore | 1 + .../app-mobile/components/Dropdown.test.tsx | 56 +++++++++ packages/app-mobile/components/Dropdown.tsx | 17 +-- packages/app-mobile/components/ItemList.js | 110 ------------------ 5 files changed, 68 insertions(+), 117 deletions(-) create mode 100644 packages/app-mobile/components/Dropdown.test.tsx delete mode 100644 packages/app-mobile/components/ItemList.js diff --git a/.eslintignore b/.eslintignore index 40236ae78..36900fb49 100644 --- a/.eslintignore +++ b/.eslintignore @@ -402,6 +402,7 @@ packages/app-mobile/components/ActionButton.js packages/app-mobile/components/BackButtonDialogBox.js packages/app-mobile/components/CameraView.js packages/app-mobile/components/CustomButton.js +packages/app-mobile/components/Dropdown.test.js packages/app-mobile/components/Dropdown.js packages/app-mobile/components/ExtendedWebView.js packages/app-mobile/components/FolderPicker.js diff --git a/.gitignore b/.gitignore index 284ec0c9f..544dc3f20 100644 --- a/.gitignore +++ b/.gitignore @@ -388,6 +388,7 @@ packages/app-mobile/components/ActionButton.js packages/app-mobile/components/BackButtonDialogBox.js packages/app-mobile/components/CameraView.js packages/app-mobile/components/CustomButton.js +packages/app-mobile/components/Dropdown.test.js packages/app-mobile/components/Dropdown.js packages/app-mobile/components/ExtendedWebView.js packages/app-mobile/components/FolderPicker.js diff --git a/packages/app-mobile/components/Dropdown.test.tsx b/packages/app-mobile/components/Dropdown.test.tsx new file mode 100644 index 000000000..e349c52ef --- /dev/null +++ b/packages/app-mobile/components/Dropdown.test.tsx @@ -0,0 +1,56 @@ +import * as React from 'react'; + +import { describe, it, expect, jest } from '@jest/globals'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react-native'; +import '@testing-library/jest-native'; + +import Dropdown, { DropdownListItem } from './Dropdown'; + +describe('Dropdown', () => { + it('should open the dropdown on click', async () => { + const items: DropdownListItem[] = []; + for (let i = 0; i < 400; i++) { + items.push({ label: `Item ${i}`, value: `${i}` }); + } + + const onValueChange = jest.fn(); + + render( + , + ); + + // Should initially not show any other items + expect(screen.queryByText('Item 3')).toBeNull(); + expect(screen.queryByText('Item 4')).toBeNull(); + + const openButton = screen.getByText('Item 1'); + fireEvent.press(openButton); + + // Other items should now be shown + await waitFor(() => { + expect(screen.getByText('Item 3')).not.toBeNull(); + expect(screen.getByText('Item 4')).not.toBeNull(); + }); + + // Pressing one of these items should hide the dropdown + fireEvent.press(screen.getByText('Item 4')); + + // We haven't changed the selectedValue, so Item 301 should no longer be visible + await waitFor(() => { + expect(screen.queryByText('Item 4')).toBeNull(); + }); + + expect(onValueChange).toHaveBeenLastCalledWith('4'); + + // The dropdown should still be clickable + fireEvent.press(screen.getByText('Item 1')); + + await waitFor(() => { + expect(screen.queryByText('Item 2')).not.toBeNull(); + }); + }); +}); diff --git a/packages/app-mobile/components/Dropdown.tsx b/packages/app-mobile/components/Dropdown.tsx index dc8777d71..b91eda951 100644 --- a/packages/app-mobile/components/Dropdown.tsx +++ b/packages/app-mobile/components/Dropdown.tsx @@ -1,8 +1,7 @@ const React = require('react'); -import { TouchableOpacity, TouchableWithoutFeedback, Dimensions, Text, Modal, View, LayoutRectangle, ViewStyle, TextStyle } from 'react-native'; +import { TouchableOpacity, TouchableWithoutFeedback, Dimensions, Text, Modal, View, LayoutRectangle, ViewStyle, TextStyle, FlatList } from 'react-native'; import { Component } from 'react'; import { _ } from '@joplin/lib/locale'; -const { ItemList } = require('./ItemList.js'); type ValueType = string; export interface DropdownListItem { @@ -122,7 +121,7 @@ class Dropdown extends Component { this.setState({ listVisible: false }); }; - const itemRenderer = (item: DropdownListItem) => { + const itemRenderer = ({ item }: { item: DropdownListItem }) => { const key = item.value ? item.value.toString() : '__null'; // The top item ("Move item to notebook...") has a null value. return ( { - ({ + length: itemHeight, + offset: itemHeight * index, + index, + })} /> diff --git a/packages/app-mobile/components/ItemList.js b/packages/app-mobile/components/ItemList.js deleted file mode 100644 index e4cafb819..000000000 --- a/packages/app-mobile/components/ItemList.js +++ /dev/null @@ -1,110 +0,0 @@ -const React = require('react'); -const { View, ScrollView } = require('react-native'); - -class ItemList extends React.Component { - constructor() { - super(); - - this.scrollTop_ = 0; - } - - itemCount(props = null) { - if (props === null) props = this.props; - return this.props.items ? this.props.items.length : this.props.itemComponents.length; - } - - updateStateItemIndexes(props = null, height = null) { - if (props === null) props = this.props; - - if (height === null) { - if (!this.state) return; - height = this.state.height; - } - - const topItemIndex = Math.max(0, Math.floor(this.scrollTop_ / props.itemHeight)); - const visibleItemCount = Math.ceil(height / props.itemHeight); - - let bottomItemIndex = topItemIndex + visibleItemCount - 1; - if (bottomItemIndex >= this.itemCount(props)) bottomItemIndex = this.itemCount(props) - 1; - - this.setState({ - topItemIndex: topItemIndex, - bottomItemIndex: bottomItemIndex, - }); - } - - UNSAFE_componentWillMount() { - this.setState({ - topItemIndex: 0, - bottomItemIndex: 0, - height: 0, - itemHeight: this.props.itemHeight ? this.props.itemHeight : 0, - }); - - this.updateStateItemIndexes(); - } - - UNSAFE_componentWillReceiveProps(newProps) { - if (newProps.itemHeight) { - this.setState({ - itemHeight: newProps.itemHeight, - }); - } - - this.updateStateItemIndexes(newProps); - } - - onScroll(event) { - this.scrollTop_ = Math.floor(event.nativeEvent.contentOffset.y); - this.updateStateItemIndexes(); - } - - onLayout(event) { - this.setState({ height: event.nativeEvent.layout.height }); - this.updateStateItemIndexes(null, event.nativeEvent.layout.height); - } - - render() { - const style = this.props.style ? this.props.style : {}; - - // if (!this.props.itemHeight) throw new Error('itemHeight is required'); - - let itemComps = []; - - if (this.props.items) { - const items = this.props.items; - - const blankItem = function(key, height) { - return ; - }; - - itemComps = [blankItem('top', this.state.topItemIndex * this.props.itemHeight)]; - - for (let i = this.state.topItemIndex; i <= this.state.bottomItemIndex; i++) { - const itemComp = this.props.itemRenderer(items[i]); - itemComps.push(itemComp); - } - - itemComps.push(blankItem('bottom', (items.length - this.state.bottomItemIndex - 1) * this.props.itemHeight)); - } else { - itemComps = this.props.itemComponents; - } - - return ( - { - this.onLayout(event); - }} - style={style} - onScroll={event => { - this.onScroll(event); - }} - > - {itemComps} - - ); - } -} - -module.exports = { ItemList };