Fix a11y issue where all pinnies are inaccessible to screenreaders

[marlowpayne]

97 opened against correct base branch for a v2.0.4 release

Opening this PR for visibility and greater discussion about a11y and our plugin architecture

Changes

• Pinny sets aria-hidden = true to its Lockup container, causing it to be unreadable by screenreaders
• This hack of a fix leverages #x-root (through a new pageContentSelector option) to determine page content that must be hidden instead of using the Lockup container. It also defaults back to the Lockup container if said element does not exist on page.
• The hack is further expanded on by changing the default append behaviour. Pinny will now append to document.body instead of the default Lockup container. This ensures that it is kept outside of the Lockup container which becomes aria-hidden if the new pageContentSelector element does not exist on page.

  Outstanding Issues

• This hack was applied for CCL (on a Zepto-compatible codebase), but it affects all Pinnies across all sites
• There's an edge case where if the Pinny uses appendTo to append itself inside #x-root or its Lockup container then it'll be aria hidden
• Lockup does not have a way of having multiple content containers within it, that could be leveraged in situations like this by Pinny (or another plugin)
• Why are Lockup and Shade separate plugins when they're only ever used alongside Pinny??

  How to Test

• Bring in this branch's code for Pinny on a Mobify project
• Test whether Pinnies in the project are a11y compliant

  Post merge actions

• This is for a backfilled Zepto-compatible release, so a new v2.0.4 release will have to be published from this branch (to avoid polluting develop with the legacy Bower components)
• This fix will then need to be patched onto the current code in develop and a new v3 jQuery release prepared from it

[jeffkamo]

@marlowpayne Okay! I can confirm that this fix works! However, there's another issue that must be addressed: focus must be managed by Pinny (at least by default).

Currently, when using a screen reader (I tested with VoiceOver just now) and you "click" the button that opens the modal, focus remains on the button, which means when the user swipes to see the next visible item, it actually doesn't seem to work... it behaves as if nothing is present. However, if you "tap" on the screen where the modal content exists, the screen reader is then able to see it, and the content can be swiped as expected.

So... if left as it is now, Pinny is still extremely unusable to screen readers. At the very least, I recommend the following be implemented as well:

• After clicking the button that opens the modal, the Pinny instance records that DOM element
• After the modal opens and after the rest of the site content is hidden, focus should be sent to the modal content
• When Pinny is closed (whether by close button, tapping out of the modal, etc.) then focus is returned to the afore mentioned DOM element (the open button)

It may be worth looking at these best practices for the typical expected behaviour of a dialog modal.

[jeffkamo]

This is outside of the scope of this PR, but we may also want to consider requiring that all modals have a "close" button or a set of instructions that explain how to escape the modal (also visually hidden).

For example, on one of our projects, Pinny is used for the main navigation and it has no close button, because the sighted user is expected to click out of the modal, or re-click the open button. Because of this, a screen reader becomes permanently trapped in the main nav. Their only escape is to click a link in the nav, or refresh the page (that's probably bad UX).

Ultimately we want to make pinny as fool proof as possible for implementors, and ensure that end users aren't getting horribly screwed over because someone forgot to add an escape button.

[jeffkamo]

Ah, excellent, I see Cole submitted an issue (#98)! Thanks @cole-sanderson!

[marlowpayne]

I have updated the main PR section with these recent developments:

• I have tweaked the pre-existing focus management of the Pinny plugin to function better in more use cases
• I have changed the default element that Pinny appends to to document.body. This is to move it by default outside of its Lockup container, which will be aria hidden if the pageContentSelector option does not exist (default is #x-root, which doesn't exist in our testing sandbox)

@jeffkamo and @cole-sanderson , when you have time could you give this another look?