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

Remove tabindex from main element #534

Merged
merged 2 commits into from Sep 19, 2017

Conversation

Projects
None yet
4 participants
@fofr
Contributor

fofr commented Jul 21, 2017

Reverts #225
See: alphagov/govuk_template#321

Putting tabindex on the <main> element causes different problems:

  • Some apps will display the browser's default focus styles around the main element
  • When clicking anywhere in the page focus will return back to the top

Consider a user interacting with an input field who clicks away from it to remove
the focus style. Should they then hit tab they will be taken to the top of the page.

The linked to browser bug was fixed in Apr. Testing on Safari 10.1.1 it works with VoiceOver on desktop. Testing on iOS 10.2.1 it works in mobile Safari with VoiceOver on.

More context around the issue:
twbs/bootstrap#20732

Example problem

GIF showing focus styles and tab order problem on Registers:

focus-order-issue-tabindex-main

Remove tabindex from main element
Reverts #225

See: alphagov/govuk_template#321

Putting tabindex on the `<main>` element causes different problems:

* Some apps will display the browser's default focus styles around the
main element
* When clicking anywhere in the page focus will return back to the top
Consider a user interacting with an input field who clicks away from it
to remove the focus style. Should they then hit tab they will be taken
to the top of the page.

@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-534 Jul 21, 2017 Inactive

@selfthinker

This comment has been minimized.

Show comment
Hide comment
@selfthinker

selfthinker Jul 21, 2017

Member

It looks like all the browser vendors have fixed the bug that was the reason for this. But it would be good to have a list of all the browsers this was tested in.

Member

selfthinker commented Jul 21, 2017

It looks like all the browser vendors have fixed the bug that was the reason for this. But it would be good to have a list of all the browsers this was tested in.

@fofr fofr changed the title from [Discuss] Remove tabindex from main element to Remove tabindex from main element Jul 24, 2017

@gemmaleigh

This comment has been minimized.

Show comment
Hide comment
@gemmaleigh

gemmaleigh Jul 25, 2017

Contributor

I commented over on GOV.UK template - this was added as a fix to ensure the target of the skiplink was focussed. If this is no longer an issue - I'm happy for this code to be deleted.

Contributor

gemmaleigh commented Jul 25, 2017

I commented over on GOV.UK template - this was added as a fix to ensure the target of the skiplink was focussed. If this is no longer an issue - I'm happy for this code to be deleted.

@fofr

This comment has been minimized.

Show comment
Hide comment
@fofr

fofr Jul 27, 2017

Contributor

@gemmaleigh As we don't control the HTML of pages, should we leave in the outline rule within layout.scss?

Contributor

fofr commented Jul 27, 2017

@gemmaleigh As we don't control the HTML of pages, should we leave in the outline rule within layout.scss?

@gemmaleigh

This comment has been minimized.

Show comment
Hide comment
@gemmaleigh

gemmaleigh Jul 31, 2017

Contributor

@fofr can we leave it in for now and add a comment that the tabindex attribute is no longer required, then we can remove it in a later release?

Contributor

gemmaleigh commented Jul 31, 2017

@fofr can we leave it in for now and add a comment that the tabindex attribute is no longer required, then we can remove it in a later release?

@nickcolley

This comment has been minimized.

Show comment
Hide comment
@nickcolley

nickcolley Sep 18, 2017

Contributor

@gemmaleigh 👍 @fofr can you update please? This seems fairly important would be good to get a new release out.

Contributor

nickcolley commented Sep 18, 2017

@gemmaleigh 👍 @fofr can you update please? This seems fairly important would be good to get a new release out.

Keep outline rule on #content but deprecate
We no longer recommend using tabindex on #content but need to keep the
styles in for apps that haven’t removed the attribute.
@fofr

This comment has been minimized.

Show comment
Hide comment
@fofr

fofr Sep 18, 2017

Contributor

@gemmaleigh @nickcolley outline kept in but deprecated in 5bdc062

Contributor

fofr commented Sep 18, 2017

@gemmaleigh @nickcolley outline kept in but deprecated in 5bdc062

@gemmaleigh

This comment has been minimized.

Show comment
Hide comment
@gemmaleigh

gemmaleigh Sep 19, 2017

Contributor

thanks @fofr 👍

Contributor

gemmaleigh commented Sep 19, 2017

thanks @fofr 👍

@gemmaleigh gemmaleigh merged commit 8216538 into master Sep 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gemmaleigh gemmaleigh deleted the revert-skiplink-tabindex branch Sep 19, 2017

@selfthinker

This comment has been minimized.

Show comment
Hide comment
@selfthinker

selfthinker Sep 19, 2017

Member

And no-one has checked in which browsers it actually works? I would think that's vital.

Member

selfthinker commented Sep 19, 2017

And no-one has checked in which browsers it actually works? I would think that's vital.

@fofr

This comment has been minimized.

Show comment
Hide comment
@fofr

fofr Sep 19, 2017

Contributor

@selfthinker IMO the bug this introduced was worse than the one it fixed. Which makes it less vital.

Tested on:

  • Safari 10.1.1 it works with VoiceOver on MacOS Sierra
  • iOS 10.2.1 and 10.3.3 it works in mobile Safari with VoiceOver on

These were the browsers the bug fix appeared to target.

Contributor

fofr commented Sep 19, 2017

@selfthinker IMO the bug this introduced was worse than the one it fixed. Which makes it less vital.

Tested on:

  • Safari 10.1.1 it works with VoiceOver on MacOS Sierra
  • iOS 10.2.1 and 10.3.3 it works in mobile Safari with VoiceOver on

These were the browsers the bug fix appeared to target.

@36degrees 36degrees referenced this pull request Oct 3, 2017

Merged

Release v3.1.2 #563

@fofr fofr referenced this pull request Aug 9, 2018

Open

Skip link #66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment