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

[A11Y] Side nav items have no keyboard focus styles #2656

Closed
Tracked by #3360
davwheat opened this issue Mar 3, 2021 · 6 comments · Fixed by #3016
Closed
Tracked by #3360

[A11Y] Side nav items have no keyboard focus styles #2656

davwheat opened this issue Mar 3, 2021 · 6 comments · Fixed by #3016
Assignees
Labels
type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.)

Comments

@davwheat
Copy link
Member

davwheat commented Mar 3, 2021

Bug Report

Current Behavior

Most side nav items are missing keyboard focus-ring styling which makes it almost impossible to tell if they are focused.

gBfIa1

Possible solution

There are a few different approaches that could take place.

.addAccessibilityFocusRing() mixin

A mix-in would allow for re-use throughout core and extensions, providing the most open solution possible.

I was working on implementing this on giffgaff's theme, but it might be better suited to the core repo instead (modified to fit core's styling, of course!)

// Adds a pink focus ring to elements

/** 
 * This mixin allows support for a custom focus
 * selector to be supplied.
 *
 * For example...
 * 
 *? button { .addKeyboardFocusRing(":focus-within") }
 * becomes
 *? button:focus-within { <styles>  }
 *
 * AND
 *
 *? button { .addKeyboardFocusRing(" :focus-within") }
 * becomes
 *? button :focus-within { <styles>  }
 */
.addKeyboardFocusRing(@customFocusSelector) {
  .focusRingStyles() {
    outline: 2px solid @gg-color-light-pink;
  }

  @realFocusSelector: ~"@{customFocusSelector}";

  &@{realFocusSelector} {
    .focusRingStyles();
  }
}

.addKeyboardFocusRing() {
  .focusRingStyles() {
    outline: 2px solid @gg-color-light-pink;
  }

  // We need to declare these separately, otherwise
  // browsers will ignore `:focus-visible` as they
  // don't understand `:-moz-focusring`

  // These are the keyboard-only versions of :focus
  &:-moz-focusring {
    .focusRingStyles();
  }
  &:focus-visible {
    .focusRingStyles();
  }
}

Environment

  • Flarum version: 0.1.0-beta.15 (and dev-master)
@SychO9
Copy link
Member

SychO9 commented Mar 3, 2021

For this specific case, i.e dropdown menu items (yes the sidebar is actually a dropdown component), couldn't we just remove the CSS block that disables the outline on focus ? Browsers should automatically have an outline on focus I believe

.Dropdown-menu>li>a:focus, .Dropdown-menu>li>button:focus {
    outline: none;
}

@davwheat
Copy link
Member Author

davwheat commented Mar 3, 2021

@SychO9 That would work for this specific case, but implementing something like this could be good for establishing a consistent (and themed) a11y outline which could be used on other elements and in extensions.

Take a look at the tags page -- the default outline in Chromium browsers (at least for me) seems a bit weird on that page, with outlines appearing partly behind some elements and in front of others.

Some exts may want to include other elements or custom buttons/elements which also need a focus outline, so this would allow them to use the same outline as core

@SychO9
Copy link
Member

SychO9 commented Mar 3, 2021

Take a look at the tags page -- the default outline in Chromium browsers (at least for me) seems a bit weird on that page, with outlines appearing partly behind some elements and in front of others.

Seems like it's due to the fact that the anchors have absolute positioning, I can't wait until we switch to flexbox/grid (#848)

That would work for this specific case, but implementing something like this could be good for establishing a consistent (and themed) a11y outline which could be used on other elements and in extensions.
...
Some exts may want to include other elements or custom buttons/elements which also need a focus outline, so this would allow them to use the same outline as core

That's a good point, I like the idea of it.

So to sum it up, what we want to do is:

  • Remove the CSS code that undoes the focus effect
  • Create the focus ring mixin (should use a variable for the outline value)
  • Apply it to the dropdown items (and to other elements when tackling other related issues)
  • Apply the hover effects of the dropdown items on focus as well (not sure why this isn't currently done, maybe just forgotten)

Right ?

@davwheat
Copy link
Member Author

davwheat commented Mar 3, 2021

I can't wait until we switch to flexbox/grid (#848)

Oh god yes, it couldn't come any sooner!

That all sounds great!

I have made the mistake of applying outlines on :focus instead of :focus-visible many times, so we need to make sure I don't do it again :P

@davwheat davwheat added the type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.) label Mar 15, 2021
@davwheat davwheat added this to the 0.1 milestone Mar 16, 2021
@askvortsov1 askvortsov1 modified the milestones: 1.0, 1.1 Apr 16, 2021
@SychO9
Copy link
Member

SychO9 commented Apr 29, 2021

Related: #2814

@davwheat
Copy link
Member Author

Moved milestone to PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants