Skip to content
This repository has been archived by the owner on Apr 19, 2019. It is now read-only.

[new design] Pen for new sidebar #365

Open
ciar4n opened this issue Mar 14, 2018 · 38 comments
Open

[new design] Pen for new sidebar #365

ciar4n opened this issue Mar 14, 2018 · 38 comments

Comments

@ciar4n
Copy link
Contributor

ciar4n commented Mar 14, 2018

As requested by @dgt41 I've created a pen for the new sidebar based on the design by @svom @coolcat-creations https://www.dropbox.com/s/jlgncxdggip22rh/Joomla4-031217-lj-white.pdf?dl=0.

CSS is rewritten from scratch and is standalone (non BS dependent). The intent is to use this to create the custom element.

Sidebar pen for review: https://codepen.io/littlesnippets/pen/4d7312648363235a6b1c9ed9a8df64cf

An a11y review would be appreciated as not at all my forte (@joomla/joomla-accessibility-team-jat, @brianteeman).

@C-Lodder
Copy link
Member

screeny

Slight issue if the secondary nav is heigher than the viewport.

@brianteeman
Copy link
Contributor

brianteeman commented Mar 14, 2018

I am not on the a11y team and you should really ask them to check. All the markup for the menu fails a11y in general see joomla/joomla-cms#19763 but its been 16 days since the promised answer

I am not sure if the html is the proposed markup or just for demo but at a minimum this needs fixing <span class="fa fa-plus"></span>

And you should check the shades of blue used here https://webaim.org/resources/contrastchecker/

@dgrammatiko
Copy link
Contributor

I am trying to figure out how the plus sign/button will work with keyboard. Let me explain:
https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html

the left/right arrow keys are supposed to move you to the previous/next root menu (unless there is a deeper ul). Now we end up with only one li tag and 2 a tags inside. I think this should be a li for the name and the plus sign should be a ul/li/a. That way we can keep the expected menu behaviour for keyboard users. (please keep in mind that complying to A11Y is not just making some tab keys follow the menu, it's far more complicate than that, check the link I posted above)

@brianteeman
Copy link
Contributor

yes i think you are correct - the plus is another level

@ciar4n
Copy link
Contributor Author

ciar4n commented Mar 14, 2018

@C-Lodder Kinda tricky one this. It is due to the 2nd level items been absolute positioned taking them out of the flow of the page. Not sure if there is a CSS only solution.

@brianteeman I've posted to the a11y glip channel

I think this should be a li for the name and the plus sign should be a ul/li/a.

So I guess any 3rd level menu items on this menu will display as an icon inline to the 2nd level items? Icon set by the menu item class name?

@brianteeman
Copy link
Contributor

Please post on a public forum like github and not a private channel. They might actually do something then

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Mar 14, 2018

So I guess any 3rd level menu items on this menu will display as an icon inline to the 2nd level items? Icon set by the menu item class name?

not necessarily, I mean the plus button should still be the a tag, so it's more about playing with classes and some js to get it in the same level. But anyhow I'm neither an expert of A11Y, all I wanted to point out is that menus are extremely complicated in the keyboard behaviour (if we really mean to be accessible and not do what bootstrap is doing with their component tabs 😂)

@dgrammatiko
Copy link
Contributor

@brianteeman there is an issue in their repo: joomla/accessibility#35 so I guess the valuable feedback will be posted there

@ciar4n
Copy link
Contributor Author

ciar4n commented Mar 14, 2018

So I guess the first concern is what markup to use for the child menu items that have a secondary link (plus icon). Currently we have...

<li class="mainnav-child-li" role="menuitem">
  <a class="mainnav-child-item" href="#" target="">Users</a>
  <a href="#" class="mainnav-child-item-secondary">
    <span class="fa fa-plus"></span>
  </a>
</li>

It is suggested this should be...

<li class="mainnav-child-li" role="menuitem">
  <a class="mainnav-child-item" href="#" target="">Users</a>
  <ul>
    <li>
      <a href="#" class="mainnav-child-item-secondary">
        <span class="fa fa-plus"></span>
      </a>
    </li>
  </ul>
</li>

This is very much an a11y question so we can wait a while for some recommendations from the accessibility team. @joomla/joomla-accessibility-team-jat

From a CSS perspective both options can be made work.

@brianteeman
Copy link
Contributor

I thought some more about should the plus be another level. I was thinking it should be because my head was still in j3 mode. In reality "articles" and "new article" are the same level. they are just displayed differently

@zwiastunsw
Copy link
Contributor

Not necessarily. "Articles" is a link to the article list. "New article" (+) is a link to a subpage. In the hierarchy, it is a link to a subpage. But I agree that it is possible to put them on one level.

@ciar4n
Copy link
Contributor Author

ciar4n commented Mar 17, 2018

@brianteeman
Copy link
Contributor

Not sure if this is related to the latest changes or not

chrome_2018-03-17_12-58-20

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Mar 17, 2018

  1. Please delete the attribute aria-hidden="true" from the all ul tags.
    This attribute deletes all nested lists from the DOM and Accessibility tree.

  2. Add content to each line,
    <span class="fa fa-plus"></span>
    for example Add item. And assign class .sr-only (from Bootstrap) to the span tag.
    <span class="fa fa-plus" class="sr-only>Add item</span>

  3. Replace tag a with a button whenever you have
    <span aria-label="Close Menu">×</span>

  4. Add aria-expanded="false" to each list in the submenu (to ul tag). The value of the attribute should change to `"true"' when the submenu receives a focus.

  5. Hide all menu item icons of the screen readers (use aria-hidden="true"), for example:
    <span class="mainnav-parent-icon fa-fw fa fa-users" aria-hidden="true"></span>

@C-Lodder
Copy link
Member

C-Lodder commented Mar 18, 2018

@zwiastunsw - The sr-only class is used for screen readers only. So normally you'd do something like this:

<span class="fa fa-plus" aria-hidden="true"></span>
<span class="sr-only">Add item</span>

This does not make sense at all:

<span class="fa fa-plus" class="sr-only">Add item</span>

as it will display the icon and text on screen readers only.

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Mar 18, 2018

@C-Lodder: What you have written does not really make sense. I did not suggest to use aria-hidden here (in line with icon "plus").
Hide the icons Users, Menu, Component, etc. for screen readers. (use aria-hidden here)
And because blind users don't see the plus icon, show them the label - use class sr-only here (they will hear it).

@brianteeman
Copy link
Contributor

@zwiastunsw thats what you wrote in point 2 above

@zwiastunsw
Copy link
Contributor

I apologise, if that is not clear. I have not written in point 2:
<span class="fa fa-plus" aria-hidden="true"></span> <span class="sr-only">Add item</span>.
I just wrote: <span class="fa fa-plus" class="sr-only>Add item</span>.
You rightly drew attention to this line in your first comments.

@brianteeman
Copy link
Contributor

What it should be is

<span class="fa fa-plus" aria-hidden="true"><span class="sr-only">Add item</span></span>
or

<span class="fa fa-plus" aria-hidden="true" title="Add item"><span class="sr-only">Add item</span></span>

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Mar 18, 2018

Oh, I can understand. My mistake. It should be:
<span class="fa fa-plus"><span class="sr-only>Add item</span></span>.

@brianteeman
Copy link
Contributor

no that doesnt hide the icon. It should be exactly what I wrote

@zwiastunsw
Copy link
Contributor

You are right. But you don't have to hide this icon. Screen readers will not see it.

@brianteeman
Copy link
Contributor

Thats not globaly true. Many screen readers will see it and fontawesome themselves state that you should hide it

@zwiastunsw
Copy link
Contributor

I have to agree with you :)
See: https://fontawesome.com/how-to-use/accessibility - Semantic Icons (Webfont with CSS)

@brianteeman
Copy link
Contributor

I know - I already did all of this for J3 ;)

@brianteeman
Copy link
Contributor

Re

Please delete the attribute aria-hidden="true" from the all ul tags.
This attribute deletes all nested lists from the DOM and Accessibility tree.

Isnt the objective here to hide the list from the dom when the menu item is collapsed. In which case it is correct for the tree to have aria-hidden=true when it is collapsed and set to false when it i expanded

@zwiastunsw
Copy link
Contributor

I agree.

@zwiastunsw
Copy link
Contributor

I will withdraw point 5 from my suggestions for the time being and I have to think about that.
Screen reader users should have immediate access to the menu, not to the toggle switch and the icon bar.

@C-Lodder
Copy link
Member

C-Lodder commented Mar 18, 2018

@brianteeman You've nested the span and added aria-hidden="true" on the outer one. Won't that hide both span tags rather than just the icon?

Thinking semantically here, shouldn't it be done as shown here? #365 (comment)

@brianteeman
Copy link
Contributor

yes - sorry - need more coffee

@ciar4n
Copy link
Contributor Author

ciar4n commented Mar 18, 2018

Pen amended as suggested..

  1. Please delete the attribute aria-hidden="true" from the all ul tags.
    This attribute deletes all nested lists from the DOM and Accessibility tree.

  2. Add content to each line,
    <span class="fa fa-plus"></span>
    for example Add item. And assign class .sr-only (from Bootstrap) to the span tag.
    <span class="fa fa-plus" class="sr-only>Add item</span>

  3. [new design] Pen for new sidebar #365 (comment)

  4. Add aria-expanded="false" to each list in the submenu (to ul tag). The value of the attribute should change to `"true"' when the submenu receives a focus. (@dgt41 if you don't mind I'll leave this bit to you.)

https://codepen.io/littlesnippets/pen/4d7312648363235a6b1c9ed9a8df64cf

@ciar4n
Copy link
Contributor Author

ciar4n commented Mar 18, 2018

#365 (comment) #365 (comment)

This visual issue should be largely amended

@zwiastunsw
Copy link
Contributor

zwiastunsw commented Mar 18, 2018

At this moment it is very good for me. I have access to all items only from the keyboard. Screen reader announces each item to me (NVDA, ChromeVox).
I suggest to add the title attribute, as Brian suggested.
<span class="fa fa-plus" aria-hidden="true" title="Add item"></span>.
Now, needed to modify javascript - provide keyboard support (arrows), change the status attributes of the drop-down items.

@ciar4n
Copy link
Contributor Author

ciar4n commented Mar 23, 2018

Title attribute added as suggested. I believe that finishes the markup.

Thank you @zwiastunsw @brianteeman @dgt41 @C-Lodder for the help on this. Good work 👍

@dgt41 I'll hand this over to you to work your magic

@dgrammatiko
Copy link
Contributor

So here we go: https://codepen.io/dgt41/pen/jzLGaG

ATM there are some changes that need to be done in the HTML (sorry, didn't catch those earlier)

Refer to https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html to check the keyboard functionality and post back what is not working

@ylahav
Copy link

ylahav commented Mar 25, 2018

First, nice implementation.
one remark:
when the sub-menu is opened - the left / right arrow have to close the sub-menu. Now it close it and got to prev / next menu item - and if this menu item has a sub-menu it open it - it is confusing, in my opinion.

@dgrammatiko
Copy link
Contributor

@ylahav the keyboard arrow keys can be switched. What I don't know is if that breaks accessibility as what is defined in the W3C document as expected behaviour is what we're currently doing. For me (JS implementation) will be a simple change in the key values but I'm not the one to decide on the behaviour...

@brianteeman
Copy link
Contributor

100% we should be doing what it says in the documentation and not applying our own ideas

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants