Skip to content
This repository has been archived by the owner on Nov 3, 2020. It is now read-only.

Built-in screen reader #966

Merged
merged 1 commit into from Dec 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Expand Up @@ -90,5 +90,6 @@ module.exports = {
'@typescript-eslint/explicit-function-return-type': 'off', // Want to use it, but it requires return types for all built-in React lifecycle methods.
'@typescript-eslint/no-non-null-assertion': 'off',
'@typescript-eslint/no-use-before-define': ['error', { functions: false }],
'import/extensions': 'off'
Copy link
Contributor

Choose a reason for hiding this comment

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

What doe this do for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was seeing errors with this in my local environment. Not sure why. It was complaining that we didn’t use .ts extensions.

},
}
109 changes: 101 additions & 8 deletions src/App.tsx
@@ -1,4 +1,4 @@
import React from 'react'
import React, { useState, useCallback } from 'react'
import { BrowserRouter, Route } from 'react-router-dom'

import 'normalize.css'
Expand All @@ -7,18 +7,111 @@ import './App.css'
import FocusManager from './components/FocusManager'

import AppRoot from './AppRoot'
import {
ScreenReader,
AriaScreenReader,
SpeechSynthesisTextToSpeech,
NullTextToSpeech,
TextToSpeech,
} from './utils/ScreenReader'

/* istanbul ignore next - unsure how to test */
window.oncontextmenu = (e: MouseEvent): void => {
e.preventDefault()
}

const App = () => (
<BrowserRouter>
<FocusManager>
<Route path="/" component={AppRoot} />
</FocusManager>
</BrowserRouter>
)
export interface Props {
tts?: {
enabled: TextToSpeech
disabled: TextToSpeech
}
}

const App = ({
tts = {
enabled: new SpeechSynthesisTextToSpeech(),
disabled: new NullTextToSpeech(),
},
}: Props) => {
const [screenReaderEnabled, setScreenReaderEnabled] = useState(false)
const [screenReader, setScreenReader] = useState<ScreenReader>(
new AriaScreenReader(tts.disabled)
)

/* istanbul ignore next - need to figure out how to test this */
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having flashbacks from trying to test the previous keyboard shortcut (⌘-K) which we never figured out how to do before we eventually removed the feature. cc @ben

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One way to do it is to move these out of App and into something a bit more testable, then maybe have a much smaller untested bit here.

const onKeyPress = useCallback(
(event: React.KeyboardEvent) => {
if (event.key === 'r') {
if (screenReaderEnabled) {
screenReader.onScreenReaderDisabled()
setScreenReader(new AriaScreenReader(tts.disabled))
setScreenReaderEnabled(false)
} else {
const newScreenReader = new AriaScreenReader(tts.enabled)
setScreenReader(newScreenReader)
setScreenReaderEnabled(true)
newScreenReader.onScreenReaderEnabled()
}
}
},
[
screenReader,
setScreenReader,
screenReaderEnabled,
setScreenReaderEnabled,
tts.disabled,
tts.enabled,
]
)

/* istanbul ignore next - need to figure out how to test this */
const onClick = useCallback(
({ target }: React.MouseEvent) => {
if (target) {
const currentPath = window.location.pathname

setImmediate(() => {
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 safe to use setImmediate?

Perhaps we need to add setImmediate to our package.json dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s interesting. It does work, so where is it coming from…?

// Only send `onClick` to the screen reader if the click didn't
// trigger navigation.
if (window.location.pathname === currentPath) {
screenReader.onClick(target)
}
})
}
},
[screenReader]
)

/* istanbul ignore next - need to figure out how to test this */
const onFocus = useCallback(
({ target }: React.FocusEvent) => {
if (target) {
const currentPath = window.location.pathname

setImmediate(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

When referencing window methods we've been explicitly using the window method (window.setTimeout(...)) in order to clearly identify window methods from local methods.

@eventualbuddha - What do you think about this? Perhaps there is an ESLint rule we can add to enforce this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t feel strongly about it. There already is an eslint rule that enforces things like window.location over location. Probably just need to expand that.

// Only send `onFocus` to the screen reader if the focus didn't
// trigger navigation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does onFocus trigger navigation in this codebase? Or is this just for safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for safety.

if (window.location.pathname === currentPath) {
screenReader.onFocus(target)
}
})
}
},
[screenReader]
)

return (
<BrowserRouter>
<FocusManager
screenReader={screenReader}
onKeyPress={onKeyPress}
onClickCapture={onClick}
onFocusCapture={onFocus}
>
<Route path="/" component={AppRoot} />
</FocusManager>
</BrowserRouter>
)
}

export default App
7 changes: 6 additions & 1 deletion src/components/FocusManager.test.tsx
Expand Up @@ -2,8 +2,13 @@ import React from 'react'
import { render } from '../../test/testUtils'

import FocusManager from './FocusManager'
import { AriaScreenReader, NullTextToSpeech } from '../utils/ScreenReader'

it('renders FocusManager', async () => {
const { container } = render(<FocusManager>foo</FocusManager>)
const { container } = render(
<FocusManager screenReader={new AriaScreenReader(new NullTextToSpeech())}>
foo
</FocusManager>
)
expect(container.firstChild).toMatchSnapshot()
})
37 changes: 31 additions & 6 deletions src/components/FocusManager.tsx
@@ -1,6 +1,7 @@
import React from 'react'
import { RouteComponentProps, withRouter } from 'react-router-dom'
import styled from 'styled-components'
import { ScreenReader } from '../utils/ScreenReader'

const StyledFocusManager = styled.div`
height: 100%;
Expand All @@ -9,17 +10,32 @@ const StyledFocusManager = styled.div`
}
`

class FocusManager extends React.Component<RouteComponentProps> {
export interface Props extends RouteComponentProps {
screenReader: ScreenReader
onKeyPress?: React.DOMAttributes<HTMLElement>['onKeyPress']
onClick?: React.DOMAttributes<HTMLElement>['onClick']
onFocus?: React.DOMAttributes<HTMLElement>['onFocus']
onKeyPressCapture?: React.DOMAttributes<HTMLElement>['onKeyPressCapture']
onClickCapture?: React.DOMAttributes<HTMLElement>['onClickCapture']
onFocusCapture?: React.DOMAttributes<HTMLElement>['onFocusCapture']
}

class FocusManager extends React.Component<Props> {
public screen = React.createRef<HTMLDivElement>()

public componentDidMount() {
this.focus()
this.onPageLoad()
}

public componentDidUpdate(prevProps: RouteComponentProps) {
if (this.props.location.pathname !== prevProps.location.pathname) {
this.focus()
this.onPageLoad()
}
}
public focus = () => {

public onPageLoad = () => {
this.props.screenReader.onPageLoad()

// can't seem to find a better way than this, unfortunately.
// the delay of 150 is to handle the case the Next button is selected
// via arrow keys and then clicked. A shorter delay will fail to move
Expand All @@ -28,14 +44,23 @@ class FocusManager extends React.Component<RouteComponentProps> {
window.setTimeout(() => {
const elementToFocus =
document.getElementById('audiofocus') || this.screen.current!

elementToFocus && elementToFocus.focus()
elementToFocus && elementToFocus.click()
}, 150)
}

public render() {
return (
<StyledFocusManager ref={this.screen} tabIndex={-1}>
<StyledFocusManager
ref={this.screen}
tabIndex={-1}
onKeyPress={this.props.onKeyPress}
onClick={this.props.onClick}
onFocus={this.props.onFocus}
onKeyPressCapture={this.props.onKeyPressCapture}
onClickCapture={this.props.onClickCapture}
onFocusCapture={this.props.onFocusCapture}
>
{this.props.children}
</StyledFocusManager>
)
Expand Down
10 changes: 2 additions & 8 deletions src/components/LinkButton.tsx
Expand Up @@ -32,15 +32,9 @@ const LinkButton = (props: Props) => {
if (onPress) {
onPress(event)
} else if (goBack && !to) {
// Delay to avoid passing tap to next screen
window.setTimeout(() => {
history.goBack()
}, 100)
history.goBack()
} else if (to && !goBack) {
// Delay to avoid passing tap to next screen
window.setTimeout(() => {
history.push(to)
}, 100)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These timeouts did not seem to be required anymore. I tried it both with a mouse and with a touchscreen and I never saw clicks "bleed through". I removed them because they were making the screen reader's job much harder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Super! There are at least 7 more places we use window.setTimeout() to avoid passing taps. Would love to figure out how to remove these as well… as they feel like hacks.

history.push(to)
}
}
return (
Expand Down
3 changes: 2 additions & 1 deletion src/components/Modal.tsx
Expand Up @@ -54,7 +54,8 @@ const Modal: React.FC<Props> = ({
document.getElementById('root')! || document.body.firstElementChild
}
ariaHideApp
role="none"
aria-modal
role="alertdialog"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These seemed to be appropriate attribute values for a modal, though nothing in this PR actually requires them.

Copy link
Contributor

Choose a reason for hiding this comment

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

If not required or not useful, wouldn't it be better to delete 'em?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could go either way. It’s not clear to me what the future of our ARIA markup should be. I’m okay removing it if you think we should.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right… we're not really using ARIA for anyone else other than ourselves.

Well, I leave it to you to decide as perhaps you have the most recent experience understanding exactly what each of these does and how we benefit from them.

isOpen={isOpen}
contentLabel={ariaLabel}
portalClassName="modal-portal"
Expand Down
2 changes: 1 addition & 1 deletion src/components/YesNoContest.tsx
Expand Up @@ -361,7 +361,7 @@ export default class YesNoContest extends React.Component<Props> {
content={
<Prose>
{overvoteSelection && (
<p>
<p id="modalaudiofocus">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is carried over from CandidateContest.

Do you want to change your vote to{' '}
<strong>{YES_NO_VOTES[overvoteSelection]}</strong>? To change
your vote, first unselect your vote for{' '}
Expand Down
1 change: 0 additions & 1 deletion src/lib/gamepad.test.tsx
Expand Up @@ -50,7 +50,6 @@ it('gamepad controls work', async () => {

// Go to First Contest
handleGamepadButtonDown('DPadRight')
fireEvent.click(getByText('Start Voting'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was no longer needed because I removed the LinkButton activation delay.

advanceTimers()

// First Contest Page
Expand Down
7 changes: 3 additions & 4 deletions src/lib/gamepad.ts
@@ -1,8 +1,7 @@
import { Button } from 'react-gamepad'
import mod from '../utils/mod'

export const getActiveElement = () =>
document.activeElement! as HTMLInputElement
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing was relying on this being an HTMLInputElement, and it typically won't be, so I just made it an HTMLElement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this was related to legacy usage.

export const getActiveElement = () => document.activeElement! as HTMLElement

function getFocusableElements(): HTMLElement[] {
const tabbableElements = Array.from(
Expand Down Expand Up @@ -41,15 +40,15 @@ function handleArrowDown() {
}

function handleArrowLeft() {
const prevButton = document.getElementById('previous') as HTMLButtonElement
const prevButton = document.getElementById('previous')
/* istanbul ignore else */
if (prevButton) {
prevButton.click()
}
}

function handleArrowRight() {
const nextButton = document.getElementById('next') as HTMLButtonElement
const nextButton = document.getElementById('next')
/* istanbul ignore else */
if (nextButton) {
nextButton.click()
Expand Down
5 changes: 3 additions & 2 deletions src/pages/ContestPage.tsx
@@ -1,4 +1,4 @@
import React, { useContext } from 'react'
import React, { useContext, useRef } from 'react'
import { Redirect, RouteComponentProps } from 'react-router-dom'
import { CandidateVote, OptionalYesNoVote } from '@votingworks/ballot-encoder'

Expand Down Expand Up @@ -35,6 +35,7 @@ const ContestPage = (props: RouteComponentProps<ContestParams>) => {
votes,
} = useContext(BallotContext)

const rootRef = useRef<HTMLElement>()
const currentContestIndex = parseInt(contestNumber, 10)
const contest = contests[currentContestIndex]

Expand All @@ -58,7 +59,7 @@ const ContestPage = (props: RouteComponentProps<ContestParams>) => {
// - confirm intent when navigating away without selecting a candidate

return (
<Screen>
<Screen ref={rootRef as any}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All changes in this file are unnecessary and represent a path I did not end up taking. I was trying to get a reference to the root element of the screen for the purpose of adding/removing event handlers, but I ended up doing that by adding handlers to FocusManager. I'll remove these.

{contest.type === 'candidate' && (
<CandidateContest
aria-live="assertive"
Expand Down
33 changes: 33 additions & 0 deletions src/setupTests.tsx
Expand Up @@ -25,6 +25,39 @@ window.print = jest.fn(() => {

const printMock = mockOf(window.print)

function mockSpeechSynthesis() {
const w = window as {
speechSynthesis: typeof speechSynthesis
SpeechSynthesisUtterance: typeof SpeechSynthesisUtterance
SpeechSynthesisEvent: typeof SpeechSynthesisEvent
}

w.speechSynthesis = makeSpeechSynthesisDouble()
w.SpeechSynthesisUtterance = jest.fn().mockImplementation(text => ({ text }))
w.SpeechSynthesisEvent = jest.fn()
}

function makeSpeechSynthesisDouble(): typeof speechSynthesis {
return {
addEventListener: jest.fn(),
cancel: jest.fn(),
dispatchEvent: jest.fn(),
getVoices: jest.fn(),
onvoiceschanged: jest.fn(),
pause: jest.fn(),
paused: false,
pending: false,
removeEventListener: jest.fn(),
resume: jest.fn(),
speak: jest.fn(),
speaking: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be great if all of this was just part of jsdom, but a search for speechSynthesis in that repo literally returns nothing.

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 lonely on the bleeding edge of A11y. 🤦🏼‍♂️

}
}

beforeEach(() => {
mockSpeechSynthesis()
})

afterEach(() => {
fetchMock.restore()
printMock.mockClear()
Expand Down