Skip to content
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

Scroll to label or legend when linked from error summary #1056

Merged
merged 3 commits into from
Nov 14, 2018

Conversation

36degrees
Copy link
Contributor

@36degrees 36degrees commented Nov 5, 2018

By default, the browser will scroll the target into view. Because our labels or legends appear above the input, this means the user will be presented with an input without any context, as the label or legend will be off the top of the screen.

screen shot 2018-11-14 at 12 51 49

Manually handling the click event, focussing the element and scrolling the question into view solves this.

screen shot 2018-11-14 at 12 52 51

To do

  • Test in all supported browsers

    • Internet Explorer 8 (Windows)
    • Internet Explorer 9 (Windows)
    • Internet Explorer 10 (Windows)
    • Internet Explorer 11 (Windows)
    • Edge (latest 2 versions) (Windows)
    • Google Chrome (latest 2 versions) (Windows)
    • Mozilla Firefox (latest 2 versions) (Windows)
    • Safari 9+ (macOS)
    • Google Chrome (latest 2 versions) (macOS)
    • Mozilla Firefox (latest 2 versions) (macOS)
    • Safari 9.3+ (iOS)
    • Google Chrome (latest 2 versions) (iOS)
    • Google Chrome (latest 2 versions) (Android)
    • Samsung Internet (latest 2 versions) (Android)
  • Test in assistive technologies

    • JAWS + Internet Explorer 11
    • ZoomText + Internet Explorer 11
    • Dragon NaturallySpeaking + Internet Explorer 11
    • NVDA + Firefox
    • VoiceOver (iOS)
    • VoiceOver (macOS)

https://trello.com/c/s9wGbLZW/1454-scroll-to-show-the-label-or-legend-when-linking-from-the-error-summary

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1056 November 5, 2018 13:57 Inactive
@36degrees 36degrees force-pushed the scroll-to-label-on-error-summary-click branch from 2ad6d99 to 93d4a9d Compare November 5, 2018 14:02
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1056 November 5, 2018 14:02 Inactive
NickColley
NickColley previously approved these changes Nov 5, 2018
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Needs a change log entry, and pending testing, but this approach is a lot nicer than I thought it'd have to be!

src/components/error-summary/error-summary.js Outdated Show resolved Hide resolved
src/components/error-summary/error-summary.js Outdated Show resolved Hide resolved
@36degrees 36degrees force-pushed the scroll-to-label-on-error-summary-click branch from 93d4a9d to 852f017 Compare November 5, 2018 14:53
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1056 November 5, 2018 14:53 Inactive

describe.each(inputTypes)('when linking to %s', async (_, inputId, legendOrLabelSelector) => {
beforeAll(async () => {
await page.goto(`${baseUrl}/examples/error-summary`, { waitUntil: 'load' })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not super-keen on hooking into the review app's examples, but couldn't think of a better way of doing this at the minute. This is something I'd definitely to revisit in the future.

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 is an issue with all our examples like this.

@kr8n3r kr8n3r added this to the [NEXT] milestone Nov 6, 2018
@kr8n3r
Copy link

kr8n3r commented Nov 7, 2018

IE11 + Jaws announces for example:
"Conditional? "text input edit" "Problem with input type of text"

@kr8n3r
Copy link

kr8n3r commented Nov 7, 2018

Zoom Text + IE11 doesn't move up to label on
Checkboxes and Conditional revealing content

aria-describedby annoucements are good
image
image

@kr8n3r
Copy link

kr8n3r commented Nov 7, 2018

Samsung Internet on Android scrolls to Conditional reveal label, opens keyboard and scrolls just down a little. Others, including text input are ok.
image

@36degrees 36degrees force-pushed the scroll-to-label-on-error-summary-click branch from 852f017 to 4c4e8a5 Compare November 7, 2018 16:12
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1056 November 7, 2018 16:12 Inactive
@36degrees 36degrees force-pushed the scroll-to-label-on-error-summary-click branch from 4c4e8a5 to af475d6 Compare November 13, 2018 15:32
@36degrees
Copy link
Contributor Author

There is still 'jumpy' scrolling in IE8 and IE9, where clicking the link takes you first to the input and then very quickly 'back up' to the legend or label:

nov-13-2018 15-33-21

However, I've been able to fix this in IE10 and above 🎉

Whilst it's not ideal, I think it is acceptable for IE8 and IE9 to do this.

Alternatively, we could choose to do nothing in browsers that don't support history.pushState, which would mean in IE8 and IE9 you'd end up jumping to the input with the legend or label off the top of the screen.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1056 November 13, 2018 15:37 Inactive
// in Edge 17).
if (window.history.pushState) {
window.history.pushState(null, null, '#' + inputId)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

One alternative might be to avoid setting a hash at all, I'm not sure what the user need is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, it's just maintaining the existing behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

@NickColley Using the hash means you can press "back" and be jumped up to the top of the page again. This is the current behaviour, I've seen people do this—it's pretty useful.

@36degrees For older browsers, could you use a timeout to allow scrollIntoView() to complete before changing the hash? It likely won't scroll again as the input is already in view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colinrotherham good call, but unfortunately changing the hash does cause IE8 and IE9 to scroll the target input to the top of the viewport, even if it is already in the viewport.

Copy link
Contributor

Choose a reason for hiding this comment

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

Boo shame! Nevermind. I've tried your branch locally and the jump is acceptable, the GIF makes it look worse than it is.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1056 November 14, 2018 10:50 Inactive
@36degrees
Copy link
Contributor Author

36degrees commented Nov 14, 2018

It looks like this change also improves things for NVDA users:

Text Input

Before: "Edit, has autocomplete"
After: "Label for input, edit, has autocomplete, problem with input, blank"

Textarea

Before: "Edit, multiline"
After: "Label for textarea, edit, multiline, problem with input, blank"

Select

Before: "Combobox, collapsed, option one."
After: "Label for select, Combobox, option one, collapsed, problem with select"

Select

Before: "Spin button, edit"
After: "Label for date input, grouping, problem with date input. Day, spin button, edit, blank"

File Upload

Before: "Browse, no file selected"
After: "Browse, button"

Radios

Before: "Radio button, not checked"
After: "Legend for radios, grouping, problem with radios. Label for first radio button, radio button not checked, 1 of 2"

Checkboxes

Before: "Checkbox, not checked"
After: "Legend for checkboxes, grouping. Label for first checkbox, checkbox not checked."

Single checkbox

Before: "Checkbox, not checked"
After: "Label for single checkbox, checkbox not checked, problem with single checkbox"

By default, the browser will scroll the target into view. Because our labels or legends appear above the input, this means the user will be presented with an input without any context, as the label or legend will be off the top of the screen.

Manually handling the click event, focussing the element and scrolling the question into view solves this.

This also results in the label and/or legend being announced correctly in NVDA (as tested in 2018.3.2) - without this only the field type is announced. For example:

For a Text Input:

Before: "Edit, has autocomplete"
After: "Label for input, edit, has autocomplete, problem with input, blank"

For a Textarea:

Before: "Edit, multiline"
After: "Label for textarea, edit, multiline, problem with input, blank"

For a Select:

Before: "Combobox, collapsed, option one."
After: "Label for select, Combobox, option one, collapsed, problem with select"

For a Select:

Before: "Spin button, edit"
After: "Label for date input, grouping, problem with date input. Day, spin button, edit, blank"

For a File Upload:

Before: "Browse, no file selected"
After: "Browse, button"

For a Radios:

Before: "Radio button, not checked"
After: "Legend for radios, grouping, problem with radios. Label for first radio button, radio button not checked, 1 of 2"

For a Checkboxes:

Before: "Checkbox, not checked"
After: "Legend for checkboxes, grouping. Label for first checkbox, checkbox not checked."

For a Single checkbox:

Before: "Checkbox, not checked"
After: "Label for single checkbox, checkbox not checked, problem with single checkbox"
Make it clearer when testing with assistive technologies exactly what is being read out.
@36degrees
Copy link
Contributor Author

I'm happy that this is good to review now 👍

36degrees added a commit to alphagov/govuk-design-system that referenced this pull request Nov 14, 2018
This introduces guidance to help service teams link from the error summary to the corresponding questions in an accessible way.

Our approach mirrors the recommended approach from WebAIM [1], linking to the input in all cases.

This does mean that the input would natively be focussed at the top of the viewport, which means the associated label or legend would not be visible to the user. There is a corresponding change to Frontend [2] to manually manage the focus and scroll state, allowing us to target and focus the input whilst scrolling to the label or legend.

To understand how to do this in an accessible way, we carried out research using a simplified test case [3] and the assistive technologies we aim to support:

## Voiceover + Safari (macOS El Capitan)

The behaviour differs depending on whether you use Enter or Ctrl-Opt-Space to navigate.

When navigating using Enter:

- linking to the label or legend generally resulted in the focus moving but nothing being announced.
- linking to the input generally worked well, but the error message (associated using aria-describedby) was not announced for radios or checkboxes.

When navigating using Ctrl-Opt-Space:
- linking to a label worked but it was not clear how to interact with the related form contrl (the label would be read followed by "You are currently on a text element, inside of web content. To exit this web area, press Ctrl-Option-Shift-Up Arrow.")
- linking to the input worked OK, but the associated legend and error message were not read out for inputs within a fieldset

Overall, linking to the input worked better.

## Voiceover + Safari (iOS 11)

There doesn't seem to be any way to get Voiceover on iOS to do anything useful, regardless of where you link to. For everything except selects and file inputs (!?) linking to the input results in the focus and VO cursor moving, but nothing being announced. Linking to the label or legend caused Voiceover to repeat itself, and neither the focus nor the VO cursor moved at all.

There is no good option. ¯\_(ツ)_/¯

## TalkBack + Google Chrome (Android)

Linking to the input worked OK, but the associated legend and error message were not read out for inputs within a fieldset.

Linking to labels resulted in the page scrolling but nothing being announced. Linking to legends resulted in the legend and error message being read, but with no clear way to navigate to the corresponding control.

Overall, linking to the input worked better.

## NVDA + Firefox

Linking to the input only described the input type, without reading any of the associated context (label, legend, error message).

Linking to the label resulted in the label being read out, preceded by 'clickable'.

Overall, linking to the label worked better. However, the manual focus state management introduced in alphagov/govuk-frontend#1056 means that linking to the input now works really well, announcing all expected context.

## JAWS2018 + IE11

Linking to the label resulted in the focus shifting down but then immediately returning to the top of the page and enters reading mode.

Linking to the input generally worked well, but the error message was not read in some cases (text areas, and error messages associated with fieldsets)

Overall, linking to the input worked better.

## Dragon NaturallySpeaking

Clicking links using Naturally Speaking's commands worked as expected in all cases.

[1]: https://webaim.org/techniques/formvalidation/
[2]: alphagov/govuk-frontend#1056
[3]: https://36degrees.github.io/linking-to-form-inputs/
@NickColley
Copy link
Contributor

Glad to see this get done, the NVDA improvements are awesome.

@NickColley
Copy link
Contributor

Needs a changelog entry

@36degrees 36degrees merged commit 4b3c081 into master Nov 14, 2018
@36degrees 36degrees deleted the scroll-to-label-on-error-summary-click branch November 15, 2018 08:47
@36degrees
Copy link
Contributor Author

36degrees commented Nov 16, 2018

@colinrotherham
Copy link
Contributor

Linking to this ticket alphagov/govuk-design-system-backlog#155 so it can be documented how the error summary links to each form control by ID (not a legend/label).

I've found a mix of behaviours across our services (some linking to empty links <a href="#something"></a>) which we need to correct. This work is fab.

@36degrees
Copy link
Contributor Author

@colinrotherham there's a PR open on the Design System which adds guidance around this to the Error Summary – if you fancy checking it out, it'd be great to get feedback from someone outside the team.

alphagov/govuk-design-system#634

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants