Skip to content

LG-7817 Ensure that screen reader maintains user's place on the page. #7487

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

Closed
eileen-nava wants to merge 10 commits into from
Closed

LG-7817 Ensure that screen reader maintains user's place on the page. #7487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2e1d735
Voiceover reads search results but not location step info text
eileen-nava Dec 8, 2022
5c17ab6
screenreader follows hierarchy of page but reads back twice
eileen-nava Dec 8, 2022
de5c2f5
refactor to use aria role
eileen-nava Dec 8, 2022
4a53fc8
Merge branch 'main' into em/7817-screen-reader-order
allthesignals Dec 9, 2022
4f7fe44
Remove conditional render; add aria information
allthesignals Dec 9, 2022
2fb5558
changelog: Upcoming Features, In-Person Proofing, Add in aria configu…
allthesignals Dec 9, 2022
98fdf24
Fix lint
allthesignals Dec 9, 2022
9c2d88f
prevent screenreader from reading back 2x on backbutton
eileen-nava Dec 13, 2022
a9db214
Merge branch 'main' into em/7817-screen-reader
eileen-nava Dec 14, 2022
5ddc7fa
fix jsx tests
eileen-nava Dec 14, 2022
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
8 changes: 4 additions & 4 deletions app/javascript/packages/document-capture/components/back-button.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ import { act } from 'react-dom/test-utils';
import BackButton from './back-button';

const getButtonHtml = () =>
`<button type="button" class="usa-button usa-button--unstyled">\u2039 ${t(
`<button type="button" class="usa-button usa-button--unstyled"><span aria-hidden="true">\u2039 </span><span>${t(
'forms.buttons.back',
)}</button>`;
)}</span></button>`;
const getBorderedButtonHtml = () =>
`<div class="margin-top-5 padding-top-2 border-top border-primary-light"><button type="button" class="usa-button usa-button--unstyled">\u2039 ${t(
`<div class="margin-top-5 padding-top-2 border-top border-primary-light"><button type="button" class="usa-button usa-button--unstyled"><span aria-hidden="true">\u2039 </span><span>${t(
'forms.buttons.back',
)}</button></div>`;
)}</span></button></div>`;

const useOnClickTest = () => {
let wasClicked = false;
Expand Down
3 changes: 2 additions & 1 deletion app/javascript/packages/document-capture/components/back-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ interface BackLinkProps extends ComponentProps<typeof Button> {
function BackButton({ includeBorder = false, ...props }: BackLinkProps) {
const button = (
<Button isUnstyled {...props}>
&#x2039; {t('forms.buttons.back')}
<span aria-hidden="true">&#x2039; </span>
<span>{t('forms.buttons.back')}</span>
</Button>
);
if (includeBorder) {
Expand Down
18 changes: 10 additions & 8 deletions ...cript/packages/document-capture/components/in-person-location-post-office-search-step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist
const [isLoadingComplete, setIsLoadingComplete] = useState(false);
const { setSubmitEventMetadata } = useContext(AnalyticsContext);
const { arcgisSearchEnabled } = useContext(InPersonContext);

// ref allows us to avoid a memory leak
const mountedRef = useRef(false);

Expand Down Expand Up @@ -201,13 +200,16 @@ function InPersonLocationPostOfficeSearchStep({ onChange, toPreviousStep, regist
return (
<>
<PageHeading>{t('in_person_proofing.headings.po_search.location')}</PageHeading>
<p>{t('in_person_proofing.body.location.po_search.po_search_about')}</p>
{arcgisSearchEnabled && (
<AddressSearch onAddressFound={handleFoundAddress} registerField={registerField} />
)}
<p>{t('in_person_proofing.body.location.location_step_about')}</p>
{locationsContent}
<BackButton onClick={toPreviousStep} />
<div aria-live="polite">
Copy link
Copy Markdown
Contributor

@aduth aduth Dec 14, 2022

Choose a reason for hiding this comment

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

Can you explain why we're making this a live region? I'm not sure I follow from the ticket or pull request description how this is related to screen reader hierarchy.

Copy link
Copy Markdown
Contributor

@aduth aduth Dec 14, 2022

Choose a reason for hiding this comment

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

Oh, I see some of the references in the ticket to an earlier Slack thread. In my mind, I might expect there to be some announcement of a change in the page content, but more brief in terms of "Page updated" or "X results found", not announcing all of the new page content as live content.

{arcgisSearchEnabled && (
<AddressSearch onAddressFound={handleFoundAddress} registerField={registerField} />
)}
</div>
<div role="status">
Copy link
Copy Markdown
Contributor

@aduth aduth Dec 14, 2022

Choose a reason for hiding this comment

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

This would make the region a live region, meaning I'd expect a screen reader to read the entire contents of this region at the first opportunity. Is that expected? Since there's quite a lot of location content, would we want to let the user navigate this themselves, since it's not really a "status" text?

Copy link
Copy Markdown
Contributor Author

@eileen-nava eileen-nava Dec 14, 2022

Choose a reason for hiding this comment

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

Yes, that was the screen reader behavior that I expected. I had interpreted these portions of the ticket  

I want the screen reader to read out the correct order of the search page experience

and

Flow of the reader should follow the hierarchy of the page

to mean that the screen reader should read the about text and then the locations text.

You’re right that it is a lot of content, and I don’t personally use a screen reader, so I could be missing something. I will check in with product/design about your concerns and then get back to you.

Copy link
Copy Markdown
Contributor Author

@eileen-nava eileen-nava Dec 15, 2022

Choose a reason for hiding this comment

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

We already verbally talked about this in a meeting yesterday. However, for posterity’s sake, I wanted to note that I checked in with design/product yesterday and there is a new ticket for the updated screen reader implementation.

<p>{t('in_person_proofing.body.location.po_search.po_search_about')}</p>
{locationsContent}
<BackButton onClick={toPreviousStep} />
</div>
</>
);
}
Expand Down