Skip to content

[4.0] [WIP] Basic attributes for a tree structure in media manager#24038

Merged
wilsonge merged 1 commit intojoomla:4.0-devfrom
wilsonge:media-manager-tree
May 1, 2019
Merged

[4.0] [WIP] Basic attributes for a tree structure in media manager#24038
wilsonge merged 1 commit intojoomla:4.0-devfrom
wilsonge:media-manager-tree

Conversation

@wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Feb 27, 2019

This is a PR working on trying to implement at a basic level a tree view hierachy for the media manager to improve a11y as part of the issues raised in joomla/accessibility#58

It also fixes the tree structure which was left broken after adding RTL support (note I have tested this PR in persian and it seems ok to me - it's not totally pixel perfect but more than usable)

https://www.w3.org/TR/wai-aria-practices/examples/treeview/treeview-1/treeview-1b.html

What is in the scope of this

This is not trying to implement keyboard support buttons - just add the required attributes - I deliberately want to leave that for someone else to do so they can get a feel of how Vue.JS works based on the keyboard navigation I did here 544c9c6

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Feb 27, 2019
@zwiastunsw
Copy link
Contributor

zwiastunsw commented Feb 27, 2019

@wilsonge :

  1. Tab key: moves the focus to the first object in the tree view or moves the focus from the object in the tree view to the next object on the page.
  2. About tabindex: See: 5.6.1 Managing Focus Within Components Using a Roving tabindex
  3. aria-labelledby should refer to the ID of the header of this section
<div class="media-disk-name">Local</div>

This should be a headline, e.g. <h3 id="TreeViewPurposeID">Local</h3>
And then:

<ul class="media-tree" aria-labelledby="TreeViewPurposeID" ....>

Is this a sufficient explanation for you?

@wilsonge wilsonge changed the title [4.0] [WIP] Basic attributes for a tree structure in media manager [4.0][a11y][WIP] Basic attributes for a tree structure in media manager Feb 27, 2019
@wilsonge
Copy link
Contributor Author

OK So @laoneo @dneukirchen I need some advice here. Unfortunately to make the tree work for the a11y requirements I need the drive to be part of the element list I think else things don't work. Is there any reason the drives can't be part of the list or in non-local adapters is there going to be cases where this is a bad idea?

@wilsonge
Copy link
Contributor Author

wilsonge commented Apr 2, 2019

@laoneo @dneukirchen bump

@dneukirchen
Copy link
Contributor

dneukirchen commented Apr 3, 2019

I need the drive to be part of the element list

What do you mean with that?

@brianteeman
Copy link
Contributor

image
image

I am assuming that @wilsonge is referring to media-disk-name
I would probably add media-drive-name

They are currently disconnected from the list media-tree
They should be part of the tree

@wilsonge
Copy link
Contributor Author

wilsonge commented Apr 3, 2019

I need the drive to be part of the element list

I mean the media drive names (currently in the class media-drive-name) need to be part of the ul tree for a11y

@dneukirchen
Copy link
Contributor

dneukirchen commented Apr 4, 2019

I guess this needs some restructuring and some changes in tree and tree-item components, but i dont see a case where this should be a problem.
I can offer to test it with non local adapters once the changes are implemented.

@ghost ghost changed the title [4.0][a11y][WIP] Basic attributes for a tree structure in media manager [4.0] [a11y] [WIP] Basic attributes for a tree structure in media manager Apr 4, 2019
@wilsonge
Copy link
Contributor Author

wilsonge commented Apr 4, 2019

Perfect. Thanks

@ghost ghost added J4 Issue a11y Accessibility labels Apr 5, 2019
@ghost ghost removed the J4 Issue label Apr 13, 2019
@wilsonge wilsonge force-pushed the media-manager-tree branch 3 times, most recently from 27bfa47 to dc497b9 Compare April 15, 2019 21:52
@wilsonge
Copy link
Contributor Author

wilsonge commented Apr 15, 2019

OK So I've fixed the html structure. The tabindex stuff is implemented to the point of the active page item gets a tabindex of 0 and the rest -1. As I said I'm going to leave the keyboard navigation for someone else to take a stab at. @dneukirchen can you take a look as well to ensure the cloud plugins are still happy here

@wilsonge wilsonge marked this pull request as ready for review April 15, 2019 23:38
@wilsonge wilsonge force-pushed the media-manager-tree branch from dc497b9 to eac3fee Compare April 15, 2019 23:39
@dneukirchen
Copy link
Contributor

tested successfully.
it works (beside the fact that the dropbox plugin is out of date and was not autoloaded correctly)

html looks like

    <div class="media-sidebar col-md-2 d-none d-md-block">
        <div class="media-disk"><h2 id="disk-1" class="media-disk-name">Dropbox</h2>
            <div class="media-drive">
                <ul role="tree" aria-labelledby="disk-1" class="media-tree">
                    <li role="treeitem" aria-level="1" aria-setsize="0" aria-posinset="1" tabindex="0"
                        class="active media-tree-item media-drive-name"><a><span
                            class="item-name">Your Dropbox</span></a>
                        <ul role="group" class="media-tree">
                            <li role="treeitem" aria-level="2" aria-setsize="1" aria-posinset="0" tabindex="-1"
                                class="media-tree-item"><a><span class="item-icon"><span class="fa fa-folder-o"></span></span>
                                <span class="item-name">dwdwdw</span></a> <!----></li>
                        </ul>
                    </li>
                </ul>
            </div>
        </div>
        <div class="media-disk"><h2 id="disk-2" class="media-disk-name">Local</h2>
            <div class="media-drive">
                <ul role="tree" aria-labelledby="disk-2" class="media-tree">
                    <li role="treeitem" aria-level="1" aria-setsize="0" aria-posinset="1" tabindex="-1"
                        class="media-tree-item media-drive-name"><a><span class="item-name">images</span></a>
                        <ul role="group" class="media-tree">
                            <li role="treeitem" aria-level="2" aria-setsize="3" aria-posinset="0" tabindex="-1"
                                class="media-tree-item"><a><span class="item-icon"><span class="fa fa-folder-o"></span></span>
                                <span class="item-name">banners</span></a> <!----></li>
                            <li role="treeitem" aria-level="2" aria-setsize="3" aria-posinset="1" tabindex="-1"
                                class="media-tree-item"><a><span class="item-icon"><span class="fa fa-folder-o"></span></span>
                                <span class="item-name">headers</span></a> <!----></li>
                            <li role="treeitem" aria-level="2" aria-setsize="3" aria-posinset="2" tabindex="-1"
                                class="media-tree-item"><a><span class="item-icon"><span class="fa fa-folder-o"></span></span>
                                <span class="item-name">sampledata</span></a> <!----></li>
                        </ul>
                    </li>
                </ul>
            </div>
        </div>
    </div>

screenshot:

Bildschirmfoto 2019-04-16 um 08 38 04

@brianteeman
Copy link
Contributor

Looks like you have a lot of unnecessary aria attributes in there. I will check more closely later but you shouldnt need aria-setsize and aria-posint

https://developers.google.com/web/fundamentals/accessibility/semantics-aria/aria-labels-and-relationships#aria-posinset_aria-setsize

When the size of a set cannot be determined by the elements present in the DOM — such as when lazy rendering is used to avoid having all of a large list in the DOM at once — aria-setsize can specify the actual set size, and aria-posinset can specify the element's position in the set.

@wilsonge
Copy link
Contributor Author

wilsonge commented Apr 16, 2019

https://www.w3.org/TR/wai-aria-practices/examples/treeview/treeview-1/treeview-1b.html

I deliberately put them in because of the above link which says

The code in this example explicitly declares values for aria-setsize, aria-posinset and aria-level, which overrides browser computation of values for these properties. The ARIA 1.0 specification for these properties states that browsers can, but are not required to, compute these values.

@brianteeman
Copy link
Contributor

Are you overriding the " browser computation of values for these properties."

@wilsonge
Copy link
Contributor Author

wilsonge commented Apr 16, 2019

https://www.w3.org/TR/wai-aria-practices/examples/treeview/treeview-1/treeview-1a.html

The ARIA 1.0 specification for these properties states that browsers can, but are not required to, compute their values. So, some browser and assistive technology combinations may not compute or report correct position and level information if it is not explicitly declared.

And i've struggled to find docs on what browsers do/don't support this. so seemed better safe than sorry

But no I'm not overriding anything

@brianteeman
Copy link
Contributor

so seemed better safe than sorry

No problem I guess - @zwiastunsw will know better than me

@zwiastunsw
Copy link
Contributor

Unfortunately, I don't know any better.
In my opinion, these attributes are not necessary. But their use will not cause confusion.
I asked for the opinion of an expert in ARIA. I should have received an answer in one day.

@brianteeman
Copy link
Contributor

Its a bit old but reading through the post and examples it seems more up to date that the example you followed http://accessibleculture.org/articles/2013/02/not-so-simple-aria-tree-views-and-screen-readers/

I checked http://www.w3.org/TR/wai-aria-practices/examples/treeview/treeview-1/treeview-1a.html which does not use posinset etc and it works correctly in FF, Edge, Chrome on windows

The only reason that it might not work would be in a scenario where the entire tree is not in the dom at page load but was added dynamically on an event. That is not the case here.

@zwiastunsw
Copy link
Contributor

I asked for the opinion of an expert in ARIA. I should have received an answer in one day.

Only after the holidays will I receive an answer.

@ghost ghost changed the title [4.0] [a11y] [WIP] Basic attributes for a tree structure in media manager [4.0] [WIP] Basic attributes for a tree structure in media manager Apr 19, 2019
@wilsonge wilsonge added the J4 Media Manager Enhanced Features label Apr 21, 2019
@wilsonge wilsonge force-pushed the media-manager-tree branch from eac3fee to ea8e928 Compare April 23, 2019 11:33
@wilsonge
Copy link
Contributor Author

@zwiastunsw Any updates from your contact. If we don't hear anything soon then I'd rather merge this in. I can start some work on the keyboard support and then we can easily come back and delete these extra attributes at a later point

@wilsonge wilsonge merged commit 83e970b into joomla:4.0-dev May 1, 2019
@wilsonge wilsonge deleted the media-manager-tree branch May 1, 2019 11:33
@wilsonge
Copy link
Contributor Author

wilsonge commented May 1, 2019

Merging per above. More than happy to come back to this if/when we hear something back

@wilsonge wilsonge added this to the Joomla 4.0 milestone May 1, 2019
@zwiastunsw
Copy link
Contributor

zwiastunsw commented Jun 13, 2019

  1. The attributes posinset and setsize are not necessary. But they do not interfere. Chrome, FF, and NVDA work well without these attributes. But there may be other browsers that need these attributes.
  2. The more important aria-expanded attribute is missing in the expanding branches
  3. The <a> tags (without href attribute) are also unnecessary.

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

Labels

a11y Accessibility J4 Media Manager Enhanced Features NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants