Make toolbar accessible

[MathuraMG]

Trello card

This PR:

• changes the play and stop elements to buttons
• makes the stop button always visible ( to increase accessibility)
• adds title to the iframe

[catarak]

Now that the play/pause buttons are button elements, they have an outline when you click them which doesn't look great. Does adding outline: none affect tabbing in accessibility?

Also, for the css, I prefer to use BEM, as in the class for the buttons should be toolbar__stop-button--selected.

[MathuraMG]

outline:none doesn't affect tabbing, I'll add that

I didn't entirely get the class naming, should toolbar__stop-button--selected be the initial state?

[catarak]

Yes exactly!

[MathuraMG]

I added BEM style to the buttons, however both styles look similar now.
What do you think of merging both to just .toolbar__button and .toolbar__button--selected ?

[catarak]

The BEM philosophy is for each HTML element to have one class (though I find myself breaking this more than occasionally), because CSS specificity rules get annoying fast and are hard to debug. I recommend reading about BEM to understand why it's used. There are lots of other CSS philosophies, like OOCSS, but for me, I find using BEM the easiest.

In the Toolbar component, the buttons should be either .toolbar__<type>-button--selected or .toolbar__<type>-button.

Then, to make all of this work with SCSS, what you can do is in the %toolbar-button placeholder in _placeholders.scss, change it to

%toolbar-button {
   //...existing styles...
  &--selected {
    background-color: $light-button-background-hover-color;
    & g {
        fill: $light-button-hover-color;
    }
  }
}

Then, in _toolbar.scss, the stop button would look like

.toolbar__stop-button {
    @extend %toolbar-button;
    &--selected {
        @extend %toolbar-button;
        @extend %toolbar-button--selected;
    }
}

[MathuraMG]

Thanks for the above, I'll make the changes !

I stand corrected on what I said about outline:none . I came came across a lot of articles that mention that it is quite important, especially on :focus ( for instance - article )
To take care of this, the simple solution would be to add the below in %toolbar-button

&:focus {
        outline: 1px solid #dddddd;
}

But I see how this throws off the aesthetics. As this works well with screen readers right now ( with outline:none; ), what do you think off making this a separate item for later so that we can also address the design?

Edit : All these articles seem pretty old - but this was one suggestion - paciellogroup

[catarak]

I'm okay with figuring out the outline stuff later! For this project I know that there is some design budget so we can just save it for when we need a bunch of design input.

[MathuraMG]

Great, I've added a card to look at this - trello card

[catarak]

this line should be
"toolbar__play-button": !this.props.isPlaying

[MathuraMG]

In the current code, the html works as follows.
when play is selected

<button class="toolbar__play-button toolbar__play-button--selected">
<button class="toolbar__stop-button">

when stop is selected

<button class="toolbar__play-button">
<button class="toolbar__stop-button  toolbar__stop-button--selected">

Based on my findings while reading up on BEM, the standard seems to be that the modifier class is added to the base class instead of replacing it.
BEM Naming

<div class="block block--mod">...</div>
    <div class="block block--size-big
        block--shadow-yes">...</div>

Additionally, it seems like replacing it has a few drawbacks.
link

That's why I have structured the code as is. Please let me know if I'm missing something and we should go for the single class approach.

[catarak]

Cool, my bad, let's leave it that way then. 😄