WIP: A11y improvements #1010

Merged
merged 57 commits into from Sep 6, 2016

Conversation

Projects
None yet
3 participants
@mfairchild365
Member

mfairchild365 commented Jul 18, 2016

NOTE: this PR is under development, do not merge yet. I've created this PR as a place to discuss and coordinate.

This aims to fix the following issues:

  • - [dropdown widget controls (search, share, idm)] Convert the input-label style of buttons to just <button> elements. The css only input-label style has several a11y issues associated with it, including incorrect semantics, bad focus-management, inconsistent outlines, etc. I see little benefit to using the css-only trick here because we will need to use JS anyway (for aria) and the framework requires JS to work anyway.
    • - [MF] update wdn-ui.js to reflect changes
    • - [MF] update includes to reflect changes
    • - [RD] update styles to hide/show containers based on the container's aria-hidden attribute.
  • - [MF] [Navigation controls] replace the input-label style of buttons for the navigation toggle as well. This is slightly different than the dropdown widget controls and will require some extra consideration.

In a future release: Improve tab order of these controls as well. Because they are moved around with css at different widths, tab order is sometimes incorrect.

mfairchild365 added some commits Jul 14, 2016

Move the `wdn-dropdown-widget-content` class to the correct location
This should allow the search box to be closed when the search icon is pressed while in mobile
Improve access to the 'checked' state
use `prop()` instead of `attr()`, because `attr()` doesn't always work.
Convert dropdown widgets to use buttons instead of labels
The css only toggle trick presented a number of accessibility issues, including outlines, focus management, semantics, etc.
Convert css navigation hack to buttons
The CSS input hack had some a11y issues (multiple labels, focus management, roles, etc), this aims to solve them.
Be smarter about figuring out if a control was clicked
If the event target was the button itself, the logic would not be triggered correctly
Improve navigation button
Make sure it has the same contents as the desktop button (more accessible), improve dom order of mobile menu button
wdn/templates_4.1/scripts/wdn-ui.js
@@ -56,7 +56,7 @@ define(['jquery', 'modernizr'], function($, Modernizr) {
//Try to get the control element
if ($control.parent('.'+dropdownButtonClass).length) {
- $control = $control.parent('.'+dropdownButtonClass)[0];
+ $control = $($control.parent('.'+dropdownButtonClass)[0]);

This comment has been minimized.

@kabel

kabel Aug 2, 2016

Contributor

Not sure why you didn't just use the jQuery API .eq or .first

@kabel

kabel Aug 2, 2016

Contributor

Not sure why you didn't just use the jQuery API .eq or .first

skoolbus39 and others added some commits Aug 2, 2016

Remove redundant padding
Already declared via the 'wdn-nav-icon' parametric mixin above
Use `.first()` instead
Its a little easier to read
Update method to toggle IDM options popup
Use aria-pressed='true' to toggle IDM options popup instead of CSS checkbox hack
Update IDM options popup layout
* Adjust popup positioning
* Remove explicit width and add 'white-space: nowrap' so popup uses no more than the space it needs
* Remove fallback hex colors for background and border-bottom (modern browsers support RGBa)
Update button creation
Duplicate buttons for nav and share menu are no longer needed. The desktop and mobile functions have been conflated.
Search toggle and popup layout updates
* Adjust search positioning
* Fix erroneous flex property
* Reduce specificity: #wdn_search_toggle_label does not need #wdn_search declared as a parent
* Remove #wdn_search_toggle, no longer used
* Remove redundant cursor styling (already declared for buttons)
* Adjust search form popup positioning
Search toggle updates (part 2)
* Declare explicit height
* Set padding to zero
* Transform icon to fine tune vertical centering (which requires making it block level element)
* Remove margin-right from search icon
Menu toggle layout updates
* Hide label until it is replaced by JavaScript.
* Show .wdn-icon-menu at all widths
* Remove line-height, redundant cursor styling (already declared for buttons), and adjust padding for shared styling of menu and share icons
* Set margin and padding to zero for menu and share icons psuedo content
* Remove float and width from menu icon
* Move left margin from .wdn-icon-menu to .wdn-nav-toggle
* Add styles for .wdn-nav-toggle: padding, z-index, overflow, line-height, remove border and bg-color (from button)
* Set .wdn-nav-toggle height for mobile and adjust positioning for desktop
* Remove fallback hex color for borders (modern browsers support RGBa)
Navigation state updates
* Remove fallback hex color for border-bottom (modern browsers support RGBa)
* Change .wdn-nav-toggle position to fixed when scrolling
Search toggle state updates
* Adjust position of search when scrolling
* Toggle display of search popup when aria-pressed='true' instead of using the CSS checkbox hack
Share toggle state updates
* Toggle display of share popup when aria-pressed='true' instead of using the CSS checkbox hack
Share toggle layout updates
* Add relative positioning to share element to support absolute positioning of share popup
* Set button height for mobile
* Adjust desktop positioning
* Remove .wdn-share-button styling (element has been removed)
* Remove .wdn-hang-left and .wdn-hang-right styles (MediaHub is currently recreating styles as needed)
* Set padding to zero for .wdn-dropdown-widget-button
* Include nav-icon styling for .wdn-icon-share
* Remove explicit width and add 'white-space: nowrap' so popup uses no more than the space it needs
* Set margin on share options popup
* Remove fallback hex color for background-color (modern browsers support RGBa)
* Update background-color of popup arrow to match popup
* Adjust positioning of popup and arrow
* Nest list styles inside share options
* Remove redundant share toggle styles in #navigation (elements have been removed)
Change plural 'navButtons' to singular
We no longer have separate nav menu toggle buttons for mobile and deesktop
Create 'menuTrigger' variable for reuse
* Create 'menuTrigger' variable for reuse
* Replace navSel variable inside $navBarLabels variable (labels/buttons are now in the wdn-menu-trigger div at all widths)
* Use menuTrigger in $nav.trigger function
Labels to buttons
The labels have been changed to buttons for a11y.
* Change 'navBarLabels' to 'navBarButtons' for more accurate description
* Change '> label > span[class^="wdn-icon"]' to 'button'. Note: the share toggle is not a direct child of the menuTrigger as it is wrapped in a div to include corresponding markup for its share options popup.
Add padding-bottom to buttons at full-width nav
Adding padding-bottom creates a larger click/tap target and prevents the share popup from overlapping the nav
menuTrigger, part 2
Use menuTrigger variable in another function (missed in a previous commit)
Adjust share widget position
Top position to match height of breadcrumbs
Adjust navigation widget position
Top position to match height of breadcrumbs
Merge pull request #1012 from skoolbus39/a11y-rd
Update styles to hide/show containers based on the container's aria-hidden attribute
'navigation',
'search',
- 'socialmediashare',

This comment has been minimized.

@kabel

kabel Aug 30, 2016

Contributor

These are all async and order does not matter. If navigation depends on socialmediashare the latter should be in the former's dependency definition.

@kabel

kabel Aug 30, 2016

Contributor

These are all async and order does not matter. If navigation depends on socialmediashare the latter should be in the former's dependency definition.

This comment has been minimized.

@mfairchild365

mfairchild365 Aug 30, 2016

Member

I thought order does matter because of the for loop with pluginObj.initialize();?

@mfairchild365

mfairchild365 Aug 30, 2016

Member

I thought order does matter because of the for loop with pluginObj.initialize();?

This comment has been minimized.

@kabel

kabel Aug 30, 2016

Contributor

Sure, but you should not depend on that. Dependencies should be explicitly declared and initialization should be entirely independent of the status of another module.

@kabel

kabel Aug 30, 2016

Contributor

Sure, but you should not depend on that. Dependencies should be explicitly declared and initialization should be entirely independent of the status of another module.

@mfairchild365

This comment has been minimized.

Show comment
Hide comment
@mfairchild365

mfairchild365 Aug 30, 2016

Member

@kabel I think this is ready to be reviewed (I know its a lot).

I'm a little worried about the potential case where some sites might not have the newest version of the includes, but DO have the newest assets. I've added some CSS to hide the old labels, but some of the functionality won't work until the includes are updated. Would it be worth it to go a step further and make sure that the functionality works even if the includes are not updated? I'm guessing not.

Do you think this is okay to roll out?

Member

mfairchild365 commented Aug 30, 2016

@kabel I think this is ready to be reviewed (I know its a lot).

I'm a little worried about the potential case where some sites might not have the newest version of the includes, but DO have the newest assets. I've added some CSS to hide the old labels, but some of the functionality won't work until the includes are updated. Would it be worth it to go a step further and make sure that the functionality works even if the includes are not updated? I'm guessing not.

Do you think this is okay to roll out?

@kabel

This comment has been minimized.

Show comment
Hide comment
@kabel

kabel Aug 30, 2016

Contributor

I'd drop this entirely as navigation is now responsible for the loading and initialization. The module is only for loading modules that are not owned by anything else.

I'd drop this entirely as navigation is now responsible for the loading and initialization. The module is only for loading modules that are not owned by anything else.

@@ -252,6 +254,10 @@
}
}
}
+
+ label {
+ display: none; /*BC fix, prevent duplicate nav buttons after change to buttons if includes are not yet updated */

This comment has been minimized.

@kabel

kabel Aug 30, 2016

Contributor

Please use less comments to avoid adding comments to the compressed output.

@kabel

kabel Aug 30, 2016

Contributor

Please use less comments to avoid adding comments to the compressed output.

This comment has been minimized.

@mfairchild365

mfairchild365 Aug 30, 2016

Member

good suggestion!

@mfairchild365

mfairchild365 Aug 30, 2016

Member

good suggestion!

wdn/templates_4.1/scripts/navigation.js
+ $navToggleButton.addClass('wdn-nav-toggle');
+
+ //Handle click events
+ $([$navToggleButton]).each(function(index, $button) {

This comment has been minimized.

@kabel

kabel Aug 30, 2016

Contributor

This seems weird. Why are you calling each? You already have a jQuery object, so you can go straight to on.

@kabel

kabel Aug 30, 2016

Contributor

This seems weird. Why are you calling each? You already have a jQuery object, so you can go straight to on.

This comment has been minimized.

@mfairchild365

mfairchild365 Aug 30, 2016

Member

I think this was a leftover from when we were doing something a little differently. I will improve it.

@mfairchild365

mfairchild365 Aug 30, 2016

Member

I think this was a leftover from when we were doing something a little differently. I will improve it.

wdn/templates_4.1/scripts/wdn-ui.js
- return;
+ //Try to get the control element
+ if ($control.parent('.'+dropdownButtonClass).length) {
+ $control = $control.parent('.'+dropdownButtonClass).first();

This comment has been minimized.

@kabel

kabel Aug 30, 2016

Contributor

Are you sure you're looking for parent (only first parent)? Perhaps you mean parents or closest. parent will return 1 or 0 elements, so no need for first.

@kabel

kabel Aug 30, 2016

Contributor

Are you sure you're looking for parent (only first parent)? Perhaps you mean parents or closest. parent will return 1 or 0 elements, so no need for first.

wdn/templates_4.1/scripts/wdn-ui.js
}
//close all dropdown widgets
- if (!$('.wdn-dropdown-widget-content').find(e.target).length) {
- closeDropDown('.wdn-dropdown-widget-toggle');
+ if (!$('.'+dropdownContentClass).find(e.target).length) {

This comment has been minimized.

@kabel

kabel Aug 30, 2016

Contributor

Where we can, we should reduce the number of times to go to the selector engine with just a class.

@kabel

kabel Aug 30, 2016

Contributor

Where we can, we should reduce the number of times to go to the selector engine with just a class.

@mfairchild365

This comment has been minimized.

Show comment
Hide comment
@mfairchild365

mfairchild365 Aug 31, 2016

Member

@kabel I've added better support for backwards compatibility (if includes haven't been updated yet).

Member

mfairchild365 commented Aug 31, 2016

@kabel I've added better support for backwards compatibility (if includes haven't been updated yet).

@kabel kabel merged commit 833482a into develop Sep 6, 2016

1 check passed

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

@kabel kabel deleted the a11y-improvements branch Feb 7, 2017

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