[UI Component]: Fix aria hidden by joshbruce · Pull Request #1192 · uswds/uswds

joshbruce · 2016-05-25T22:04:42Z

Fixes #1181 - toggleClass(), within the hide or show functions, causes
double-click requirement - if show-hide functionality is encapsulated,
toggleClass(). Encapsulation would probably reduce code readability.

Fixes #1120

See PR #1160 Fixes #1181 - toggleClass(), within the hide or show functions, causes double-click requirement - if show-hide functionality is encapsulated, toggleClass(). Encapsulation would probably reduce code readability. Fixes #1120

joshbruce · 2016-05-25T22:20:32Z

Adding from #1160:

@msecret, @ducklet, @rogeruiz, @maya

yozlet · 2016-05-25T22:22:20Z

Hey, thanks for this!

I'll leave it to my colleagues to review the rest of the commit, but could you revert the inclusion of Gemfile.lock? Those gems are due for an update but we should do that in a separate commit in case it breaks something.

msecret · 2016-05-25T22:23:05Z

Hi @joshbruce, thanks for the PR. What do you mean by "causes double-click requirement"?

msecret · 2016-05-25T22:23:41Z

  }
 }
+
+.usa-accordion-content-collapsed {


Is there a reason we need the accordion to have it's own class for display none, or should this be put in a general utility?

What about what's done here: https://github.com/frend/frend.co/blob/gh-pages/_components/accordion/accordion.css

http://frend.co/components/accordion/

.usa-accordion-content[aria-hidden="true"] { visibility: hidden; height: 0; }

@msecret - Interesting suggestion. I'm not sure I'm seeing the utility solution that you might be. If we make a mixin for display: none, wouldn't we still need to @include it somewhere that distinguishes the two states?

@maya - Seems an interesting alternative. After looking at the JavaScript for the Frend accordion, it appears, to overcome setting height: 0 by calculating heights for the expanded accordions - might need to test and verify. If so, it would be more JavaScript dependence to make the accordions expand (#1121 & #1122) - unlike the display: block route. See lines 90 through 122.

@joshbruce you're right, there may not be any benefit to the visibility: hidden method, that I could think of. We'll need to do more research. Anyhow, I tried a simple, classless method here: #1196. I'd like to hear what others think.

@maya - Saw that. It's good solution. Incorporated it here. Also tested just doing height: auto, which did work - no JS changes needed. I do like the height solution from the screen-reader perspective - though I'm not sure if visibility: hidden would cause it to be skipped. Which brings up the question: Do we want accordion content to be hidden from screen-readers?

joshbruce · 2016-05-26T00:29:01Z

@msecret - Good question.

When I used the following with hide:

$content.toggleClass('accordion-content accordion-content-collapsed');

And the following to show:

$content.toggleClass('accordion-content-collapsed accordion-content');

It would require hitting the accordion button twice to actually get it to collapse or expand, respectively. Therefore, I may have misinterpreted what toggleClass() really did under the hood. It appears it will either add, or remove, a class if the class is set or not; however, it will not change one class name into another class name.

joshbruce · 2016-05-26T00:34:15Z

@maya and @msecret - Updated PR, with both the simple (#1196) and utility concepts. Can always rollback and update PR, if desired. However, I do appreciate Maya's simple solution.

joshbruce · 2016-05-26T01:35:05Z

@yozlet - Hey. Sorry, I missed your request for the revert on Gemfile.lock. Because I had to start fresh from a new fork - long story - I had to run $ bundle update (tried running just $ npm install - did not seem to work). See my comment on #1128

Ran into this error today when doing
$ gulp website:serve
website:serve jekyll 3.1.3 | Error: kramdown
Not sure why, given that kramdown is not used - unless it is a dependency for red carpet.

And, of course, $ bundle update rewrites the Gemfile.lock.

In PHP (using Composer) we sometimes ignore composer.lock file in .gitignore to try and overcome this. Could do the same thing in Gemfile with specific version numbers instead of just the gem name and .gitignore the .lock file.

If there's something we can do to make a fresh clone (or fork) work without needing $ bundle update - let me know; otherwise, I'm not sure I can revert the file by itself. Or, of course, I could be do something wrong on the fork-clone front.

msecret · 2016-05-26T21:12:58Z

 }

+// Aria hidden helper
+@mixin aria-hidden() {


I think when I said a global utility, I mean a class, such as .hidden. Sorry for the confusion. Although, what might be the benefits of using a mixin like this? It might be a better solution.

@msecret I'd be careful about calling it .hidden, so it doesn't get confused with the hidden attribute. Although that might have been just an example or it might be ok?

Also not sure I'd call the mixin aria-hidden either bc its not the true function of aria-hidden, just a special use case for this situation. Curious to here what are the benefits of using the mixin is too. Are we reusing this somewhere else?

@msecret: @joshbruce explained the benefits of the mixin in #1196:

To allow developers to @include within any class or element; thereby, allowing any element to opt-in to being toggled based on aria-hidden state based on CSS

My question is the same: do we see this as a common pattern/use case?

@msecret & @maya - I think having it as a mixin also establishes a pattern of how to handle display: none related specifically to accessibility considerations - similar to the unstyled-list mixin.

I don't think this particular pattern is used with any other UI component. I think the problem we were trying to solve initially was that aria-hidden shouldn't display: none on elements that use that attribute setting. @maya's simple solution solves this problem and limits its scope to the accordion-content class. This PR extends the flexibility of that implementation, which may be a pre-optimization (capability with no demand from developers, internal nor external)...??

Thanks for the explanation @joshbruce , I think you've convinced me that the mixin is a good way to go. I'd agree with @maya that we should come up with a better name for it though.

@joshbruce can you change the mixin name to accessibly-hidden?

# Conflicts: # Gemfile.lock

yozlet · 2016-05-27T17:11:01Z

@joshbruce I believe git checkout 35bc35b Gemfile.lock will revert the file to the previous version, but it'll need another commit. It's weird that bundle install didn't work, though! Can you try it again after doing the revert? (And whether it works or not, I'd much prefer we leave Gemfile.lock as the old version for this PR, but we can update it in another one.)

joshbruce · 2016-05-29T15:26:07Z

@yozlet - Done. Thanks for the instructions there. It appears there is a deletion from what in the current project: "bundled with 1.12.3"

@maya - @mixin name changed.

maya · 2016-05-29T17:12:18Z

@joshbruce accessibly-hidden please, that says accessibility-.

joshbruce · 2016-05-29T17:48:16Z

@maya - Sorry. Thought that was a typo. :)

yozlet · 2016-05-31T03:18:19Z

@joshbruce: Thank you! Nearly done - go_script needs to come out too (it was pulled out in the main repo 5 days ago). Sorry, it's a moving target!

ducklet · 2016-05-31T18:55:20Z

@maya @joshbruce sorry I've been mia; I'm starting to feel like the wheels came off the wagon a bit with the aria-hidden/display none rules... why are we tying display: none into aria-hidden at all? Having both is redundant. Imho it would make more sense to simply create a class with rule display: none.

I'm also a bit confused by the request to use class name accessibly-hidden. Maybe there's another logic that's escaping me, but content with display: none is inherently not accessible, to anyone.

I understand the impetus behind not simply removing the display: none rule from aria-hidden, but I fear we may be dragging this out and adding unnecessary complexity to the accordion fix. Could we consider just a "use this to hide something until it's exposed" class, be it specific to the accordion or more versatile for other potential future usages?

Thanks for your consideration and esp @joshbruce your great work and persistence in making this fix happen! I am able to override the rule for the time being but putting !important in our code to override !important feels icky :)

joshbruce · 2016-05-31T23:45:22Z

@yozlet - Did a copy-paste from staging as of today.

@ducklet - I asked a similar question in the comments and have decided to leave it to the judgement of the Standards team. I believe this particular concept is the notion of an assistive technology consideration driving sighted user's experience. So, instead of display: none (sighted experience) being the driving force for assistive technologies, we allow assistive technology state aria-hidden=true to drive the visual experience. This would help adopters use this accessibility-first pattern more easily.

I believe the choice for naming is based on the fact that it is directly tied to the aria-hidden=true state; therefore, directly related to an accessibility consideration - not just display: none.

From looking at the line notes - the additional class was the original solution provided - the concern there was creating "yet another class" for adopters to use and remember. Though this fix does not necessarily remove the learning it - it does keep the class list one smaller than the other solution - while allowing adopters to add the @mixin to an element they so choose; therefore, adding display: none to any element with aria-hidden=true.

I agree that !important is icky - and I believe the Standards has attempted to remove its use altogether.

Cheers! Looking forward to seeing how this goes. :)

maya · 2016-05-31T23:50:01Z

@ducklet What @joshbruce stated is correct. The mixin allows us to hide things dynamically based on the aria-hidden state -- it's only to be used for specific components/use cases, not for everything.

maya · 2016-05-31T23:50:25Z

These changes look good to me. Thanks again for your help, @joshbruce! 🎉

ducklet · 2016-06-01T19:36:44Z

@maya this is merged but just something to think about moving forward, the problem with the abstract concepts we're now discussing is that bottom line, aria-hidden would never be used with display: none. WebAim cautions that mis-using ARIA roles can be as bad or worse than not using them, and ignoring the fundamentals breeds bad practices. I don't mean to harp but I do feel strongly that attaching our own interests to something like this is a slippery slope.

[UI Component]: Fix aria hidden

3b521c6

See PR #1160 Fixes #1181 - toggleClass(), within the hide or show functions, causes double-click requirement - if show-hide functionality is encapsulated, toggleClass(). Encapsulation would probably reduce code readability. Fixes #1120

joshbruce mentioned this pull request May 25, 2016

[Website]: Code preview no longer working #1193

Closed

msecret reviewed May 25, 2016
View reviewed changes

joshbruce mentioned this pull request May 26, 2016

[UI Component]: Fix aria hidden (simple) #1196

Closed

Combine simple + utility

4f14ee5

msecret reviewed May 26, 2016
View reviewed changes

Josh Bruce added 3 commits May 27, 2016 09:38

Correct typo

c5cbfea

Remove collapsed from code sample generation

f72c599

Merge remote-tracking branch '18F/staging' into fix-aria-hidden

3e11efe

# Conflicts: # Gemfile.lock

Revert Gemfile.lock & rename mixin

00346e8

Fix typo on mixin

81fd594

maya added the [Status] In progress label May 31, 2016

maya added the [Status] Revisions needed label May 31, 2016

Remove go_script from Gemfile.lock

47d6b2d

maya added [Status] Review needed and removed [Status] Revisions needed labels May 31, 2016

maya merged commit fb35539 into uswds:staging May 31, 2016

maya removed [Status] In progress labels May 31, 2016

joshbruce mentioned this pull request Jun 2, 2016

aria-hidden="true" should not have display: none !important #1120

Closed

joshbruce mentioned this pull request Jun 10, 2016

[ Website ]: Not all accordions work without JavaScript #1245

Closed

Uh oh!

Conversation

joshbruce commented May 25, 2016

Uh oh!

joshbruce commented May 25, 2016

Uh oh!

yozlet commented May 25, 2016

Uh oh!

msecret commented May 25, 2016

Uh oh!

msecret May 25, 2016

Choose a reason for hiding this comment

Uh oh!

maya May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshbruce May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maya May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshbruce May 26, 2016

Choose a reason for hiding this comment

Uh oh!

joshbruce commented May 26, 2016

Uh oh!

joshbruce commented May 26, 2016

Uh oh!

joshbruce commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msecret May 26, 2016

Choose a reason for hiding this comment

Uh oh!

maya May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maya May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshbruce May 27, 2016

Choose a reason for hiding this comment

Uh oh!

msecret May 27, 2016

Choose a reason for hiding this comment

Uh oh!

maya May 27, 2016

Choose a reason for hiding this comment

Uh oh!

yozlet commented May 27, 2016

Uh oh!

joshbruce commented May 29, 2016

Uh oh!

maya commented May 29, 2016

Uh oh!

joshbruce commented May 29, 2016

Uh oh!

yozlet commented May 31, 2016

Uh oh!

ducklet commented May 31, 2016

Uh oh!

joshbruce commented May 31, 2016

Uh oh!

maya commented May 31, 2016

Uh oh!

maya commented May 31, 2016

Uh oh!

ducklet commented Jun 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

maya May 25, 2016 •

edited

Loading

joshbruce May 25, 2016 •

edited

Loading

maya May 26, 2016 •

edited

Loading

joshbruce commented May 26, 2016 •

edited

Loading

maya May 26, 2016 •

edited

Loading

maya May 26, 2016 •

edited

Loading