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

Fix skiplink target so it doesn't take up space and cause overflow #415

Merged
merged 5 commits into from
May 22, 2019

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented May 1, 2019

READY FOR REVIEW

Summary

  • The skiplink target was using the .su-sr-only-text class which uses the Bourbon @hide-text mixin which takes up space and causes overflow. This PR creates new class .su-skiplinks__target which uses the @hide-visually mixin instead which has the style code we need. The skip link target now has proper styles (same as current best practice).
  • I didn't just use the .su-sr-only-element class on the skip link target directly because .su-sr-only-element unhides on focus.
  • Add documentation for .su-sr-only-text - it's only good for cases where e.g., hide the SR text in the site-search component magnifying glass button/icon.

Needed By (Date)

  • ASAP

Urgency

  • Pretty urgent

Steps to Test

  1. Pull this branch and run npm run build.
  2. Look at the skiplink target in the skiplinks component. It should now now take up any spaces.

Affected Projects or Products

  • Decanter

Associated Issues and/or People

…the @hide-visually mixin instead of @hide-text (causes overflow and takes up space)
@yvonnetangsu yvonnetangsu requested review from josephgknox and JBCSU and removed request for josephgknox May 1, 2019 22:59
@@ -19,7 +19,7 @@
// <a href="#main-content" class="su-skiplinks">Skip to main content</a>
// </header>
// <main>
// <h2 id="main-content" class="su-sr-only-text" tabindex="-1">Main Content</h2>
// <h2 id="main-content" class="su-skiplinks__target" tabindex="-1">Main Content</h2>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need a new class? Why can't we just fix .su-sr-only-text?

If we do need a new class, I'd prefer to not use a name that implies you can only use the class on one item, e.g. skiplinks. I would expect the class to be useful for anything that needs to be hidden, e.g. the supplemental text in Read more links. Is there some reason the skip link target need to be treated specially?

Copy link
Member Author

@yvonnetangsu yvonnetangsu May 3, 2019

Choose a reason for hiding this comment

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

.su-sr-only-text uses the Bourbon mixin @hide-text which is good for some use cases, eg, for the SR only text in site search magnifying glass icon so I want to preserve that.

We currently have 2 existing SR helper classes (the above mentioned .su-sr-only-text, and .su-sr-only-element) and neither of them function the way we want the skiplink target to function. One way we could make this solution more general would be to rethink .su-sr-only-element. Currently, it unhides when it's on-focus. I don't believe any components in Decanter uses it, but this class might be used by some other sites that use Decanter? This might be something that needs a discussion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you saying your implementation, @hide-visually, wouldn't work for the search icon?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I believe that's what I tried initially and it didn't work.

Copy link
Collaborator

@JBCSU JBCSU May 3, 2019

Choose a reason for hiding this comment

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

Ok, so we have a few issues here:

Apparently the point of .su-sr-only-text is to hide just the textual content of an element, but render everything else, e.g. background, border, padding, .... .su-sr-only-element is to hide the entire element, including background, border, padding, .... Applying .su-sr-only-element to the search icon hides the icon as well as the helper text, because the icon is a background image for the helper text.

In testing this, I found the first issue, which is that the search icon's helper text is never read aloud by voice over - the aria-label="Search" is read, regardless of what text is provided for the <button>. So that helper text is completely ignored, at least by voice over - we can remove it entirely and therefore we can remove .su-sr-only-text from the button.

But back to skip links:

If the target of the skip link is, as we recommend, an <h2> whose sole purpose is to announce to screen-readers "Main content", then we want to hide the entire element. So .su-sr-only-element seems appropriate. So now we need to decide what should happen when focus lands on the sr-only-element.

I propose that .su-sr-only-element does not reveal on focus, and that we add a variant, .su-sr-only-element--reveal-on-focus, that does.

That said, I think the target of the skip link, which is the aforementioned <h2>, actually should reveal on focus. When I'm testing keyboard nav and I hit the skip link, I'm left wondering where my focus went. I have to keep tabbing to figure out where I'm at. I think if the <h2>Main Content</h2> revealed itself it would be as useful to keyboard navigators as it is to screen readers. Jiatyan agrees that that would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm curious about the aria-label/button text issue also. If Level Access can confirm that it is ok to remove the button text entirely then we can certainly just do that.

I'm open to the idea of skip link targets reveals itself on focus. I also agree that our default .su-sr-only-element should not reveal on focus and I like the idea of a variant that does reveal itself on focus.

Other Decanter people - any thoughts? 😃

Copy link
Member

@sherakama sherakama May 8, 2019

Choose a reason for hiding this comment

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

Might as well use the bourbon mixins for this. As the target is its own element I would suggest changing the name to su-skiplinks-target instead of the BEM child as it really isn't nested. If you want to reveal the element on focus you can use the unhide paramater as suggested in the bourbon documentation.

/// @example scss
   .su-skiplinks-target {
     @include hide-visually;

     &:active,
     &:focus {
       @include hide-visually("unhide");
     }
   }

Copy link
Member

Choose a reason for hiding this comment

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

As this is pretty much all new it doesn't break compatibility and we can safely merge and go on.

@JBCSU JBCSU requested review from josephgknox and sherakama May 3, 2019 22:30
@sherakama
Copy link
Member

Original issue here: #414

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

As the target is its own element I would suggest changing the name to su-skiplinks-target instead of the BEM child as it really isn't nested.

Other than that this looks good to me.

@yvonnetangsu
Copy link
Member Author

As the target is its own element I would suggest changing the name to su-skiplinks-target instead of the BEM child as it really isn't nested.

Other than that this looks good to me.

Thanks for the review, @sherakama ! I had a discussion with JB last week and she's not keen on the idea of adding a new class name so instead she'd rather we tweak the default .su-sr-only-element class and remove the unhide on focus behavior, and adding a variant to .su-sr-only-element class that does unhide on focus. @JBCSU - comments? Would that work, @sherakama?

JBCSU and others added 2 commits May 22, 2019 11:00
@yvonnetangsu
Copy link
Member Author

yvonnetangsu commented May 22, 2019

@sherakama , @JBCSU , I removed the su-skiplinks__target class, used the su-sr-only-element class instead and changed the su-sr-only-element helper class so it doesn't unhide on focus. I didn't put unhide on focus in the skiplink target - I remember @JBCSU , Scott and I had a discussion and Scott preferred that skiplink targets do not unhide on focus, since that's the conventional thing out there for skiplinks and he didn't want to confuse keyboard users. Any thoughts, @sherakama and @JBCSU ?

JBCSU added 2 commits May 22, 2019 11:50
…4-sr-only-text

* '414-sr-only-text' of github.com:SU-SWS/decanter:
  remove su-skiplinks__target class and use su-sr-only-element; remove unhide behavior from su-sr-only-element helper
Copy link
Collaborator

@JBCSU JBCSU left a comment

Choose a reason for hiding this comment

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

Great, thanks!

I'm noticing that the styleguide itself doesn't have a skip link. We should fix that, but not in this PR. This PR is GTG.

@yvonnetangsu yvonnetangsu merged commit 4f25559 into master May 22, 2019
@yvonnetangsu yvonnetangsu deleted the 414-sr-only-text branch May 22, 2019 20:12
yvonnetangsu added a commit that referenced this pull request Jun 11, 2019
* master: (44 commits)
  Set up color maps for Decanter and refactor main nav colors to refer to color maps (#420)
  Move PR template (#416)
  Fix skiplink target so it doesn't take up space and cause overflow (#415)
  Release 5.0.1 (#417)
  Fix CSS grid gaps not displaying on Edge issue and other minor grid related issues (#413)
  More link fixup (#388)
  forgot to rebuild .js after final lint fixes (#387)
  add openNav, closeNav, openSubnav, closeSubnav events (#379)
  Fix main nav color variants and work with new linting rules (#380)
  add favicon to style guide (#377)
  Adjust linters. (#370)
  update readme.md to match Scott's intro on homepage.md (#371)
  Update README.md (#369)
  move the images we need for the styleguide's homepage into kss-assets (#368)
  Update README.md (#367)
  Update UPGRADE.md (#363)
  package.locl.json file is back (#366)
  Added more to changelog (#364)
  Update homepage.md (#362)
  90 basic webpack (#334)
  ...

# Conflicts:
#	core/scss/components/index.scss
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants