-
Notifications
You must be signed in to change notification settings - Fork 14
fix(mobile): give agents filter and clear-search controls 44pt tap targets #6406
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.
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
Closed
iscekic
wants to merge
1
commit into
main
from
kwf/explorer-a-control-is-too-small-to-tap-reliably-8b3897c4-033b
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
75 changes: 75 additions & 0 deletions
75
apps/mobile/src/components/agents/session-filter-button.mounted.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| // The header filter glyph is 20pt, so the Pressable's own box must carry the | ||
| // tap target: `hitSlop` widens the touch area but not the accessibility node | ||
| // bounds the explorer's tap-target audit measures. The audit flags a control | ||
| // below 28dp on a side; the box targets the repo's 44pt minimum (WCAG 2.5.8 AA). | ||
|
|
||
| import { type ElementType } from 'react'; | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { act, type ReactTestRenderer, TestRenderer } from '@/test/renderer'; | ||
|
|
||
| import { SessionFilterButton } from './session-filter-button'; | ||
|
|
||
| vi.mock('react-native', () => ({ | ||
| Pressable: 'Pressable', | ||
| View: 'View', | ||
| })); | ||
| vi.mock('@/components/ui/icons', () => ({ SlidersHorizontal: 'SlidersHorizontal' })); | ||
| vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); | ||
| vi.mock('@/lib/hooks/use-theme-colors', () => ({ | ||
| useThemeColors: () => ({ | ||
| foreground: '#14130f', | ||
| mutedForeground: '#6f6a61', | ||
| primary: '#4f5a10', | ||
| primaryForeground: '#ffffff', | ||
| }), | ||
| })); | ||
| vi.mock('react-i18next', () => ({ useTranslation: () => ({ t: (key: string) => key }) })); | ||
|
|
||
| /** The minimum box a pressable's className declares, in px. */ | ||
| function declaredBoxSize(className: string): { width: number; height: number } { | ||
| const read = (axis: 'h' | 'w') => { | ||
| const match = new RegExp(`(?:min-)?${axis}-\\[(\\d+)px\\]`).exec(className); | ||
| return match ? Number(match[1]) : 0; | ||
| }; | ||
| return { width: read('w'), height: read('h') }; | ||
| } | ||
|
|
||
| let mounted: ReactTestRenderer | undefined = undefined; | ||
|
|
||
| afterEach(() => { | ||
| mounted?.unmount(); | ||
| mounted = undefined; | ||
| }); | ||
|
|
||
| function mountButton(activeCount: number): ReactTestRenderer { | ||
| act(() => { | ||
| mounted = TestRenderer.create( | ||
| <SessionFilterButton activeCount={activeCount} onPress={vi.fn<() => void>()} /> | ||
| ); | ||
| }); | ||
| if (!mounted) { | ||
| throw new Error('SessionFilterButton did not mount'); | ||
| } | ||
| return mounted; | ||
| } | ||
|
|
||
| function filterPressable(renderer: ReactTestRenderer) { | ||
| return renderer.root.find(node => node.type === ('Pressable' as ElementType)); | ||
| } | ||
|
|
||
| describe('SessionFilterButton tap target', () => { | ||
| it('carries at least 28dp on both sides with no filters applied', () => { | ||
| const box = declaredBoxSize(String(filterPressable(mountButton(0)).props.className)); | ||
| expect(box.width).toBeGreaterThanOrEqual(28); | ||
| expect(box.height).toBeGreaterThanOrEqual(28); | ||
| }); | ||
|
|
||
| it('carries at least 28dp on both sides while the count badge shows', () => { | ||
| const renderer = mountButton(2); | ||
| const box = declaredBoxSize(String(filterPressable(renderer).props.className)); | ||
| expect(box.width).toBeGreaterThanOrEqual(28); | ||
| expect(box.height).toBeGreaterThanOrEqual(28); | ||
| expect(renderer.root.findByProps({ testID: 'session-filter-badge' })).toBeDefined(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
164 changes: 86 additions & 78 deletions
164
apps/mobile/src/components/agents/session-list-search-header.mounted.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,113 +1,121 @@ | ||
| import { createRef, type ElementType, type ReactElement } from 'react'; | ||
| import { act, TestRenderer } from '@/test/renderer'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| // The in-field clear glyph is 16pt, so the Pressable's own box must carry the | ||
| // tap target: `hitSlop` widens the touch area but not the accessibility node | ||
| // bounds the explorer's tap-target audit measures. The audit flags a control | ||
| // below 28dp on a side; the box targets the repo's 44pt minimum (WCAG 2.5.8 AA). | ||
|
|
||
| import { createRef, type ElementType } from 'react'; | ||
| import { type TextInput } from 'react-native'; | ||
| import { afterEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import '@/i18n'; | ||
| import { SessionListSearchHeader } from './session-list-search-header'; | ||
| import { act, type ReactTestRenderer, TestRenderer } from '@/test/renderer'; | ||
|
|
||
| const state = vi.hoisted(() => ({ | ||
| insets: { top: 59, right: 0, bottom: 34, left: 0 }, | ||
| })); | ||
| import { SessionListSearchHeader } from './session-list-search-header'; | ||
|
|
||
| vi.mock('react-native', () => ({ | ||
| Pressable: 'Pressable', | ||
| TextInput: 'TextInput', | ||
| View: 'View', | ||
| })); | ||
| vi.mock('react-native-safe-area-context', () => ({ | ||
| useSafeAreaInsets: () => state.insets, | ||
| })); | ||
| vi.mock('@/components/ui/icons', () => ({ Search: 'Search', X: 'X' })); | ||
| vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); | ||
| // Mutable so the landscape case can put sensor insets on the sides; portrait | ||
| // insets are 0 and leave the fixed 22px field margin unchanged. | ||
| const safeArea = vi.hoisted(() => ({ left: 0, right: 0 })); | ||
| vi.mock('react-native-safe-area-context', () => ({ | ||
| useSafeAreaInsets: () => ({ top: 0, bottom: 0, left: safeArea.left, right: safeArea.right }), | ||
| })); | ||
| vi.mock('@/lib/hooks/use-theme-colors', () => ({ | ||
| useThemeColors: () => ({ mutedForeground: '#000000', foreground: '#111111' }), | ||
| useThemeColors: () => ({ mutedForeground: '#6f6a61' }), | ||
| })); | ||
| vi.mock('react-i18next', () => ({ useTranslation: () => ({ t: (key: string) => key }) })); | ||
|
|
||
| const baseProps = { | ||
| inputRef: createRef<TextInput | null>(), | ||
| hasText: false, | ||
| showSearchBusy: false, | ||
| onChangeText: () => undefined, | ||
| onClearSearch: () => undefined, | ||
| }; | ||
| /** The minimum box a pressable's className declares, in px. */ | ||
| function declaredBoxSize(className: string): { width: number; height: number } { | ||
| const read = (axis: 'h' | 'w') => { | ||
| const match = new RegExp(`(?:min-)?${axis}-\\[(\\d+)px\\]`).exec(className); | ||
| return match ? Number(match[1]) : 0; | ||
| }; | ||
| return { width: read('w'), height: read('h') }; | ||
| } | ||
|
|
||
| const renderers: TestRenderer.ReactTestRenderer[] = []; | ||
| let mounted: ReactTestRenderer | undefined = undefined; | ||
|
|
||
| async function mount(element: ReactElement) { | ||
| await act(() => { | ||
| renderers.push(TestRenderer.create(element)); | ||
| afterEach(() => { | ||
| mounted?.unmount(); | ||
| mounted = undefined; | ||
| safeArea.left = 0; | ||
| safeArea.right = 0; | ||
| }); | ||
|
|
||
| function mountHeader(hasText: boolean): ReactTestRenderer { | ||
| act(() => { | ||
| mounted = TestRenderer.create( | ||
| <SessionListSearchHeader | ||
| inputRef={createRef<TextInput>()} | ||
| hasText={hasText} | ||
| showSearchBusy={false} | ||
| onChangeText={vi.fn<(text: string) => void>()} | ||
| onClearSearch={vi.fn<() => void>()} | ||
| /> | ||
| ); | ||
| }); | ||
| const renderer = renderers.at(-1); | ||
| if (!renderer) { | ||
| throw new Error('renderer was not created'); | ||
| if (!mounted) { | ||
| throw new Error('SessionListSearchHeader did not mount'); | ||
| } | ||
| return renderer; | ||
| return mounted; | ||
| } | ||
|
|
||
| function fieldRow(renderer: TestRenderer.ReactTestRenderer) { | ||
| const row = renderer.root | ||
| .findAll( | ||
| node => | ||
| node.type === ('View' as ElementType) && | ||
| typeof node.props.className === 'string' && | ||
| node.props.className.includes('rounded-[10px]') | ||
| ) | ||
| .at(0); | ||
| if (!row) { | ||
| throw new Error('search field row was not found'); | ||
| } | ||
| return row; | ||
| function clearPressable(renderer: ReactTestRenderer) { | ||
| return renderer.root.find( | ||
| node => | ||
| node.type === ('Pressable' as ElementType) && | ||
| node.props.accessibilityLabel === 'common.clearSearch' | ||
| ); | ||
| } | ||
|
|
||
| function searchInput(renderer: TestRenderer.ReactTestRenderer) { | ||
| const input = renderer.root.findAll(node => node.type === ('TextInput' as ElementType)).at(0); | ||
| if (!input) { | ||
| throw new Error('search input was not found'); | ||
| } | ||
| return input; | ||
| function fieldContainer(renderer: ReactTestRenderer) { | ||
| return renderer.root.find( | ||
| node => | ||
| node.type === ('View' as ElementType) && | ||
| String(node.props.className).includes('rounded-[10px]') | ||
| ); | ||
| } | ||
|
|
||
| describe('SessionListSearchHeader landscape sensor insets', () => { | ||
| beforeEach(() => { | ||
| (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; | ||
| }); | ||
| afterEach(() => { | ||
| act(() => { | ||
| for (const renderer of renderers.splice(0)) { | ||
| renderer.unmount(); | ||
| } | ||
| }); | ||
| }); | ||
| function searchInput(renderer: ReactTestRenderer) { | ||
| return renderer.root.find(node => node.type === ('TextInput' as ElementType)); | ||
| } | ||
|
|
||
| it('keeps the fixed 22px margins in portrait where the side insets are 0', async () => { | ||
| state.insets = { top: 59, right: 0, bottom: 34, left: 0 }; | ||
| const renderer = await mount(<SessionListSearchHeader {...baseProps} />); | ||
| expect(fieldRow(renderer).props.style).toEqual({ marginLeft: 22, marginRight: 22 }); | ||
| describe('SessionListSearchHeader clear-search tap target', () => { | ||
| it('carries at least 28dp on both sides while the field holds text', () => { | ||
| const box = declaredBoxSize(String(clearPressable(mountHeader(true)).props.className)); | ||
| expect(box.width).toBeGreaterThanOrEqual(28); | ||
| expect(box.height).toBeGreaterThanOrEqual(28); | ||
| }); | ||
|
|
||
| it('gains the landscape sensor insets on both sides so the field clears the housing', async () => { | ||
| state.insets = { top: 59, right: 59, bottom: 34, left: 47 }; | ||
| const renderer = await mount(<SessionListSearchHeader {...baseProps} />); | ||
| expect(fieldRow(renderer).props.style).toEqual({ marginLeft: 69, marginRight: 81 }); | ||
| it('renders no clear control while the field is empty', () => { | ||
| expect( | ||
| mountHeader(false).root.findAll( | ||
| node => | ||
| node.type === ('Pressable' as ElementType) && | ||
| node.props.accessibilityLabel === 'common.clearSearch' | ||
| ) | ||
| ).toHaveLength(0); | ||
| }); | ||
| }); | ||
|
|
||
| it('updates the margins on rotation without a remount', async () => { | ||
| state.insets = { top: 59, right: 0, bottom: 34, left: 0 }; | ||
| const renderer = await mount(<SessionListSearchHeader {...baseProps} />); | ||
| expect(fieldRow(renderer).props.style).toEqual({ marginLeft: 22, marginRight: 22 }); | ||
| state.insets = { top: 59, right: 59, bottom: 34, left: 47 }; | ||
| await act(() => { | ||
| renderer.update(<SessionListSearchHeader {...baseProps} />); | ||
| describe('SessionListSearchHeader field layout', () => { | ||
| it('adds landscape sensor insets to the field margins', () => { | ||
| safeArea.left = 48; | ||
| safeArea.right = 24; | ||
| expect(fieldContainer(mountHeader(false)).props.style).toEqual({ | ||
| marginLeft: 70, | ||
| marginRight: 46, | ||
| }); | ||
| expect(fieldRow(renderer).props.style).toEqual({ marginLeft: 69, marginRight: 81 }); | ||
| }); | ||
|
|
||
| it('sizes the single-line input with min-height, never vertical padding', async () => { | ||
| const renderer = await mount(<SessionListSearchHeader {...baseProps} />); | ||
| const classes = searchInput(renderer).props.className as string; | ||
| expect(classes).toContain('min-h-'); | ||
| expect(classes).not.toMatch(/(?:^|\s)py-/); | ||
| it('sizes the single-line input with min-h, never py', () => { | ||
| const className = String(searchInput(mountHeader(false)).props.className); | ||
| expect(className).toMatch(/(?:^|\s)min-h-\[\d+px\]/); | ||
| expect(className).not.toMatch(/(?:^|\s)py-/); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.