Add button accessibility #28

RalucaNicola · 2018-06-09T14:34:39Z

@pshihn here's a PR for making the wired-button accessible. Can you have a look? This was way more difficult than I thought 😁

I ran into several issues because I didn't really understand how to use lit-element:

not sure where the accessibility attributes should actually go, I added them to the div in the shadow dom. I noticed that if I add them directly on the wired-button element they wouldn't have any effect.
not sure how to get the innerHTML of the wired-button element to set it as aria-label, so I added it in the _didRender method, because there I had access to the innerHTML.
is it ok if I add the keydown event listener in the constructor?

Thank you!

pshihn · 2018-06-09T21:03:02Z

I've added a wiki page with details on setting up the dev environment. That would be useful: https://github.com/wiredjs/wired-elements/wiki/Dev-Environment

pshihn · 2018-06-09T21:08:22Z

packages/wired-button/wired-button.js

+        event.preventDefault();
+        this.click();
+      }
+    });
  }


Instead of constructor, this should be done in connectedCallback method when using web components. I think connectedCallback method is already defined in this class. This code can move there.

pshihn · 2018-06-09T21:19:24Z

packages/wired-button/wired-button.js

@@ -80,12 +85,16 @@ export class WiredButton extends LitElement {
      }
    </style>
    <slot></slot>
-    <div class="overlay">
+    <div class="overlay" tabIndex="${this._getIndex()}" role="button">
      <svg id="svg"></svg>


I think the correct node to apply the 'role' here would be 'this'. In all wired-elememts, overlay SVG node is usually purely aesthetic. Another alternative would be we encapsulate the inside another span and add roles to the span. In this case, i think the root node is probably the best option.

So, in the connectedCallback, you can call this.setAttribute('role', 'button')

Tab index should also be set on 'this'.
It should probably be set in the _didRender method because that would be called when disable changes as well.
You also need to consider that the user may have overwritten the tabIndex

this.tabIndex = this.disabled ? -1 : (this.tabIndex || 0);

Somehow I can't manage to make it work if I set the tabIndex on the shadow host. I was reading about it here and it should work without problems: https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex but I can't manage to set the focus on the wired-button when using the tab key. I'll look a bit more into this. For now I changed the code in the _didRender method and I've set it on this. But if you try out the example.html file it's not really working when using tab.

ok, I asked a colleague and it comes from setting delegateFocus to true. In the wired-button component there are no focusable elements, so it won't get the focus when setting tabIndex on the wired-button. @pshihn is there a need for setting it to true? Otherwise I'll set it to false.

Seems like it doesn't work for me even when delegateFocus is false.

Figured it out. you need to set the delegateFocus to false
and when setting the tabIndex, get the default value from the attribute rather than proeprty

this.tabIndex = this.disabled ? -1 : (this.getAttribute('tabindex') || 0);

pshihn · 2018-06-09T21:25:06Z

packages/wired-button/wired-button.js

@@ -122,6 +131,9 @@ export class WiredButton extends LitElement {
      (wired.line(svg, s.width + (i * 2), s.height + (i * 2), s.width + (i * 2), i * 2)).style.opacity = (75 - (i * 10)) / 100;
    }
    this.classList.remove('pending');
+
+    const overlay = this.shadowRoot.querySelector('.overlay');
+    overlay.setAttribute('aria-label', this.innerHTML);
  }


As per my previous comments, in the case of button, the correct node would be this. (Probably not the case for other controls.)
Again in connectedCallback you scan set the 'aria-label' property.

- add keyboard events in connectedCallback - set role and aria-label in connectedCallback - set tabIndex in _didRender method

RalucaNicola · 2018-06-11T12:59:14Z

@pshihn great, thanks for helping me set up the dev environment, it works now 👍 I have a small issue with tabIndex, I wrote a comment on it here, I'm still trying to figure out what's going on.

pshihn

I added a comment about tabIndex. Once that change is made, this should be good to go.

pshihn · 2018-06-13T07:20:43Z

packages/wired-button/wired-button.js

@@ -80,12 +85,16 @@ export class WiredButton extends LitElement {
      }
    </style>
    <slot></slot>
-    <div class="overlay">
+    <div class="overlay" tabIndex="${this._getIndex()}" role="button">
      <svg id="svg"></svg>


Seems like it doesn't work for me even when delegateFocus is false.

pshihn · 2018-06-13T07:34:05Z

packages/wired-button/wired-button.js

@@ -80,12 +85,16 @@ export class WiredButton extends LitElement {
      }
    </style>
    <slot></slot>
-    <div class="overlay">
+    <div class="overlay" tabIndex="${this._getIndex()}" role="button">
      <svg id="svg"></svg>


Figured it out. you need to set the delegateFocus to false
and when setting the tabIndex, get the default value from the attribute rather than proeprty

this.tabIndex = this.disabled ? -1 : (this.getAttribute('tabindex') || 0);

RalucaNicola · 2018-06-13T19:42:35Z

done @pshihn :) I also removed the outline: none in the css, so that focus is visible.

I tried it out with the example by setting different tabindex, disabling the button and played around with voiceover.

pshihn · 2018-06-13T19:45:19Z

Thanks a lot @RalucaNicola I learned something about accessibility going through this with you.

RalucaNicola · 2018-06-13T21:05:41Z

thanks a lot for your help, I hope I can do the next components all by myself :)

Add button accessibility

c01b51d

pshihn requested changes Jun 9, 2018

View reviewed changes

RalucaNicola added 2 commits June 11, 2018 13:47

Merge remote-tracking branch 'upstream/master'

ba0336a

Refactor accessibility code for button:

a8c0381

- add keyboard events in connectedCallback - set role and aria-label in connectedCallback - set tabIndex in _didRender method

pshihn requested changes Jun 13, 2018

View reviewed changes

RalucaNicola added 2 commits June 13, 2018 21:35

Allow focus on wired-button

8178e0f

Fix casing for tabindex attribute

e6b1141

pshihn merged commit da4f150 into rough-stuff:master Jun 13, 2018

pshihn mentioned this pull request Jun 27, 2018

Accessibility for components #13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add button accessibility #28

Add button accessibility #28

RalucaNicola commented Jun 9, 2018

pshihn commented Jun 9, 2018

pshihn Jun 9, 2018

pshihn Jun 9, 2018

pshihn Jun 9, 2018

RalucaNicola Jun 11, 2018

RalucaNicola Jun 11, 2018 •

edited

pshihn Jun 13, 2018

pshihn Jun 13, 2018

pshihn Jun 9, 2018

RalucaNicola commented Jun 11, 2018 •

edited

pshihn left a comment

pshihn Jun 13, 2018

pshihn Jun 13, 2018

RalucaNicola commented Jun 13, 2018 •

edited

pshihn commented Jun 13, 2018

RalucaNicola commented Jun 13, 2018

Add button accessibility #28

Add button accessibility #28

Conversation

RalucaNicola commented Jun 9, 2018

pshihn commented Jun 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalucaNicola Jun 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalucaNicola commented Jun 11, 2018 • edited

pshihn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalucaNicola commented Jun 13, 2018 • edited

pshihn commented Jun 13, 2018

RalucaNicola commented Jun 13, 2018

RalucaNicola Jun 11, 2018 •

edited

RalucaNicola commented Jun 11, 2018 •

edited

RalucaNicola commented Jun 13, 2018 •

edited