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

[VisuallyHidden] Updating Visually Hidden CSS #4641

Merged
merged 5 commits into from
Nov 16, 2021

Conversation

sophschneider
Copy link
Contributor

@sophschneider sophschneider commented Nov 12, 2021

WHY are these changes introduced?

The visually hidden styles in polaris uses top which unnecessarily pushes hidden content to the top. it also uses the deprecated clip style. This PR updates the styles to use the GovUK design system visually hidden styles discussed here. The styles have been tested extensively as seen here. I tested the styles myself on mac Voice Over.

VO demo of old styles: https://videobin.shopify.io/v/1XbjO4
VO demo of new styles: https://videobin.shopify.io/v/bbj2Aj

old styles, note the top attribute, this can be much more drastic if the container is tall:
Screen Shot 2021-11-15 at 10 14 56 AM

new styles:
Screen Shot 2021-11-15 at 10 14 38 AM

WHAT is this pull request doing?

updates styles for visually hidden

Playground code

export function Playground() {
  const headings = ['hello', 'world', 'aria hidden'].map((heading) => {
    if (heading === 'aria hidden') {
      const accessibilityLabel = `I am visually hidden`;

      return (
        <>
          <VisuallyHidden>{accessibilityLabel}</VisuallyHidden>
          <div aria-hidden>I am aria hidden</div>
        </>
      );
    }

    return heading;
  });

  return (
    <Page title="Playground">
      <Card>
        <DataTable
          columnContentTypes={['text', 'text', 'text']}
          headings={headings}
          rows={[]}
          verticalAlign="middle"
        />
      </Card>
    </Page>
  );

🎩 checklist

@sophschneider sophschneider changed the title [Visually Hidden] Added left position 0 to visually hidden css [VisuallyHidden] Fixed left position 0 to visually hidden css Nov 12, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 12, 2021

size-limit report

Path Size
cjs 166.08 KB (0%)
esm 96.64 KB (0%)
esnext 143.53 KB (+0.04% 🔺)
css 34.6 KB (+0.23% 🔺)

@sophschneider sophschneider force-pushed the sophie/visually-hidden-css-bug branch from 34a5bb0 to e8e1e62 Compare November 12, 2021 21:33
Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

Adding some context from Slack to this issue. This is the approach I am most familiar with:

clip-path: inset(50%);
height: 1px;
overflow: hidden;
position: absolute;
white-space: nowrap; 
width: 1px;

top and left values should be removed. clip is depreacted. I would love to know if you have any opinions @sophschneider. I read this great blog post recently diving deep into this problem. I also appreciated the conversation in the HTML5 boilerplate.

@sophschneider sophschneider changed the title [VisuallyHidden] Fixed left position 0 to visually hidden css [VisuallyHidden] Fix for Visually Hidden CSS causing visual scroll issues Nov 12, 2021
@sophschneider
Copy link
Contributor Author

@alex-page read the blog and thread, they were both super interesting and helpful! I'm going to use your snippet and test it on a bunch of browsers and screen readers, I might add in or take out some styles from the threads if I run into the same problems they did (announcing order, weird focus ring, etc.)

@sophschneider sophschneider force-pushed the sophie/visually-hidden-css-bug branch from c9f44bf to 2693175 Compare November 13, 2021 14:46
@sophschneider sophschneider changed the title [VisuallyHidden] Fix for Visually Hidden CSS causing visual scroll issues [VisuallyHidden] Updating Visually Hidden CSS Nov 15, 2021
@sophschneider sophschneider force-pushed the sophie/visually-hidden-css-bug branch from 525dac1 to 291b978 Compare November 15, 2021 15:19
@sophschneider sophschneider force-pushed the sophie/visually-hidden-css-bug branch from d37f6db to 35ff532 Compare November 15, 2021 15:22
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Looks good just make sure to reenable the no-important lint rule and then 🚢

src/styles/shared/_accessibility.scss Show resolved Hide resolved
@sophschneider sophschneider force-pushed the sophie/visually-hidden-css-bug branch from ee691ed to b68bc47 Compare November 16, 2021 00:36
@sophschneider sophschneider merged commit 4d692a1 into main Nov 16, 2021
@sophschneider sophschneider deleted the sophie/visually-hidden-css-bug branch November 16, 2021 02:04
@emma-boardman
Copy link
Contributor

emma-boardman commented Feb 18, 2022

👋 Hi!

We've run into a couple of bugs in online-store-web as a result of this change. In one instance, we were able to create a workaround:

However, the bug also occurs on the ThemesIndex (video included below), and there are too many components using VisuallyHidden to override that CSS on a case-by-case.

Would there be any strong objections to me re-adding top:0 and releasing a v8 fix?

cc @sophschneider, @alex-page , @alexandcote

GitIntegrationSheet bug:

Screen.Recording.2022-02-18.at.10.59.46.mov

@sophschneider
Copy link
Contributor Author

hi @emma-boardman! no objections from me but it is kind of weird that VisuallyHidden is being used for visual styling even if it is accidental, ideally it wouldn't change things visually at all

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.

4 participants