WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 3 weeks ago

#47149 closed defect (bug) (fixed)

Tab structure does not use tab semantics

Reported by: anevins Owned by: afercia
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Media Keywords: needs-post-mortem wpcampus-report has-screenshots semantic-buttons has-patch i18n-change
Focuses: accessibility, javascript Cc:
PR Number:

Description

Moved from the WPCampus accessibility report issues on GitHub, see: https://github.com/WordPress/gutenberg/issues/15287

  • Severity:
    • Medium
  • Affected Populations:
    • Blind
    • Low-Vision
    • Motor Impaired
    • Cognitively Impaired
  • Platform(s):
    • Windows - Screen Reader
    • Windows - ZoomText
    • Windows - Dragon
    • Mac - VoiceOver
    • Android - TalkBack
    • iOS - VoiceOver
  • Components affected:
    • Media Dialog

Issue description
In the "Featured Image" modal dialog, two links are used to switch
views (between the "Media Library" and the "Upload Files" panels).
These are coded as links with redundant href attributes ("#"),
which is not semantically valid, and does not convey the
currently-active link to assistive technologies.

The choice of button vs link has a significant effect on user
expectations for assistive technology users: links convey the
expectation that the browser will navigate to a different location or
page, which standard buttons do not. The difference also affects what
keys can be used within assistive technologies to navigate between the
elements, and which page summaries the element appears in (for example,
it won't show up in a list of all form elements on the page).

Semantic markup is vital for assistive technology users, to convey the
structure and relationships between information. When appropriate
semantic markup is not used, it may be more difficult for such users to
understand what the content means, and to comprehend the structure that
the visual presentation conveys.

Issue Code

    <div class="media-router">


        <a href="#" class="media-menu-item active">Upload Files</a>


        <a href="#" class="media-menu-item">Media Library</a>


    </div>

Remediation Guidance
The visual presentation of the content within the "Featured Image"
dialog is a tab panel. Using the [tab panel pattern]
(https://www.w3.org/TR/wai-aria-practices-1.1/#tabpanel) will give the
elements proper roles, and using aria-selected will allow users of
assistive technologies to know which tab is currently selected.

Recommended Code

    <div role="tablist" class="media-router">


        <button role="tab" aria-selected="true" class="media-menu-item active">Upload Files</button>


        <button role="tab" class="media-menu-item">Media Library</button>


    </div>

Relevant standards

Note: This issue may be a duplicate with other existing accessibility-related bugs in this project. This issue comes from the Gutenberg accessibility audit, performed by Tenon and funded by WP Campus. This issue is GUT-58 in Tenon's report

Attachments (29)

2fd289d77bd897652138d753f1420cd8.gif (757.9 KB) - added by audrasjb 6 months ago.
Workaround: add button role and aria-pressed attributes
media-views.min.js (243.0 KB) - added by audrasjb 6 months ago.
Workaround: search "JBA" comments to see the changes from the original file
media menu.png (85.1 KB) - added by afercia 6 months ago.
Media menu items in the desktop and responsive view.
47149.diff (10.6 KB) - added by afercia 5 months ago.
47149.2.diff (11.3 KB) - added by afercia 4 months ago.
47149.3.diff (13.0 KB) - added by afercia 4 months ago.
47149.4.diff (13.1 KB) - added by afercia 4 months ago.
47149.5.diff (19.8 KB) - added by afercia 7 weeks ago.
Media - Title as menu.png (438.8 KB) - added by melchoyce 6 weeks ago.
Media - Media Types menu.png (441.5 KB) - added by melchoyce 6 weeks ago.
screen.png (39.8 KB) - added by karmatosed 6 weeks ago.
Media Tabs.png (527.9 KB) - added by melchoyce 6 weeks ago.
01 sidebar gray bg.png (336.3 KB) - added by afercia 6 weeks ago.
02 longer translations.png (245.9 KB) - added by afercia 6 weeks ago.
media modal mobile.png (311.8 KB) - added by afercia 5 weeks ago.
47149.6.diff (21.5 KB) - added by afercia 5 weeks ago.
47149.7.diff (1.1 KB) - added by afercia 4 weeks ago.
media menu.2.png (80.5 KB) - added by afercia 4 weeks ago.
Menu items examples
47149.8.diff (668 bytes) - added by afercia 3 weeks ago.
47149.8.png (108.2 KB) - added by afercia 3 weeks ago.
New toggle button text
video-details-screen-actions.png (474.6 KB) - added by sabernhardt 3 weeks ago.
Video Details modal with visible "Actions" heading
add-audio-screen-actions.png (22.3 KB) - added by sabernhardt 3 weeks ago.
Add Audio modal with visible "Actions" heading
47149.9.diff (689 bytes) - added by afercia 3 weeks ago.
double-heading.png (2.2 KB) - added by azaozz 3 weeks ago.
example.png (64.8 KB) - added by afercia 3 weeks ago.
Less prominent headings example
finder example.png (159.8 KB) - added by afercia 3 weeks ago.
If this UI was made with HTML, those would be headings and they're less prominent than the items
media-menu-h2-styling.png (1.1 MB) - added by melchoyce 3 weeks ago.
47149 example.png (249.9 KB) - added by afercia 3 weeks ago.
Making the heading less prominent
47149.10.diff (484 bytes) - added by afercia 3 weeks ago.

Change History (121)

#1 @afercia
6 months ago

Comment from the GitHub issue: https://github.com/WordPress/gutenberg/issues/15287#issuecomment-488441147

Aside: I'm wary about using a tab interface when there are just 2 "tabs", as it tends to make keyboard interaction more complicated. Instead, it definitely helps when there are many tabs as it significantly reduces the amount of tab stops. With only 2 tabs, the missing information could be provided by other means.

#2 @afercia
6 months ago

  • Milestone changed from Awaiting Review to 5.3

@audrasjb
6 months ago

Workaround: add button role and aria-pressed attributes

@audrasjb
6 months ago

Workaround: search "JBA" comments to see the changes from the original file

#3 @audrasjb
6 months ago

  • Keywords has-screenshots added; needs-screenshots removed

Hi,

I'd like to propose a first workaround, based on this ressource: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role

I was not able to make a proper fix on this issue but I uploaded a testable JS fix above.

  • add role="button" attribute
  • add aria-pressed="true/false" attribute
  • toggle aria-pressed attribute when needed

Search for //JBA comments to see the changes I've done.
See also the animated GIF file to see the result.

Cheers,
Jb

#4 @audrasjb
6 months ago

  • Owner set to audrasjb
  • Status changed from new to accepted

#5 @afercia
6 months ago

  • Keywords semantic-buttons added

Worth noting this applies also to the menu items in the left sidebar of the media modal (the sidebar appears only in some of the media views). They're all <a href="#"> links, while they should be vertical tabs. However, in the responsive view the menu becomes a dropdown menu (see screenshot on the right), which makes things a bit more difficult to fix.

@afercia
6 months ago

Media menu items in the desktop and responsive view.

#6 @afercia
6 months ago

Previously #32236, which didn't fix the menu items. See tracking ticket #26504.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


5 months ago

#8 @anevins
5 months ago

Steps we need to take:
1) Investigate all instances of this tabs antipattern;
2) Implement the tabs design pattern with keyboard interactions as per the W3C's recommendation. Helpful examples: https://www.w3.org/TR/wai-aria-practices-1.1/examples/tabs/tabs-1/tabs.html
3) Test across all instances of the tabs.

Last edited 5 months ago by afercia (previous) (diff)

#9 @afercia
5 months ago

Note: actually, an ARIA tabbed interface could be built also upon links (<a> tags) if only they were valid URL-fragment links pointing to a valid page fragment, see for example https://inclusive-components.design/tabbed-interfaces/

However, these are just invalid links <a href="#"> thus they need to be changed to buttons, see https://core.trac.wordpress.org/query?keywords=~semantic-buttons

#10 @afercia
5 months ago

  • Owner changed from audrasjb to afercia
  • Status changed from accepted to assigned

I'm looking into this and working on a patch. In the media views, the Menu view generates both the menu on the left sidebar and the horizontal menu (called media-router, not sure why):

http://cldup.com/jR381xmXaL.png

The left menu should be a vertical ARIA tabbed interface as well, so it makes sense to change both menus. Worth reminding it's not just a matter of adding some ARIA roles. ARIA tabs need to implement a specific keyboard interaction pattern, see details in the ARIA Authoring Practices.

I'm almost done with the interaction part. The main problem now is to add the required roles and attributes

Both the menus and the content update "on the fly" and this makes difficult to add IDs referencing the correct views. I'd tend to think this part would need a workaround.

These are the parts that are a bit tricky to implement:

  • Each element with role tab has the property aria-controls referring to its associated tabpanel element.
  • Each element that contains the content panel for a tab has role tabpanel.
  • Each element with role tabpanel has the property aria-labelledby referring to its associated tab element.

@afercia
5 months ago

#11 @afercia
5 months ago

  • Keywords has-patch dev-feedback added; needs-patch removed

47149.diff is a proof of concept. It implements a first part of the ARIA tabbed interface, ie. the keyboard interaction part. As said in the previous comment, there's the need to add roles, attributes, and IDs required for this user interface pattern to work properly. This second part is a bit tricky and needs some exploration.

The basic idea is to extend wp.media.view.FocusManager to make it a tool able to handle special cases of focus management (e.g. arrow keys navigation) and ARIA widgets.

WordCamp Europe 2019 is in a few days, it would be a good opportunity to collaborate at implementing the missing parts :)

Note: in this proof of concept styling is only partially implemented: when testing, please ignore the visual part for now.

Last edited 5 months ago by afercia (previous) (diff)

#12 @afercia
4 months ago

Re: aria-controls, there's an important update. See this quick note by Steve Faulkner:
https://twitter.com/stevefaulkner/status/1138834162562019328

"If you encounter an element on a web page with a defined aria controls relationship, JAWS will no longer say "use JAWSKEY+ALT+M to move to controlled element" by default."

JAWS 2019 release notes: https://support.freedomscientific.com/Downloads/JAWS/JAWSWhatsNew

JAWS was the only known assistive tech to support aria-controls. Not to mention the actual usefulness of aria-controls was previously argued for good reasons.

#13 @afercia
4 months ago

See some initial design feedback on this related ticket comment: https://core.trac.wordpress.org/ticket/47145#comment:9
/Cc @kjellr @melchoyce

#14 @afercia
4 months ago

  • Keywords needs-design-feedback added

@afercia
4 months ago

#15 @afercia
4 months ago

47149.2.diff refreshes the patch and improves styling. Still to do:

  • correct association of the tabs with the tab panels
  • make a decision on how the left menu should behave in the responsive view: currently, it switches to a dropdown but this doesn't fit so well with the ARIA tabs pattern

This ticket was mentioned in Slack in #core-media by afercia. View the logs.


4 months ago

@afercia
4 months ago

#17 @afercia
4 months ago

  • Focuses javascript added
  • Keywords dev-feedback removed

47149.3.diff associates the tabpanel content with the active menu item in the left menu, so that the tabpanel is properly labelled. Also, cleans up things a bit. Still to do:

  • associate the tabpanel with the "router" tabs: "Upload Files" and "Media Library"
  • handle edge cases where the modal view hides the left menu, e.g.: the Image Editor
  • make a decision on how the left menu should behave in the responsive view: currently, it switches to a dropdown but this doesn't fit so well with the ARIA tabs pattern, see screenshot below

http://cldup.com/A28S2RtrFV.png

@afercia
4 months ago

#18 @afercia
4 months ago

In 47149.4.diff

It appears NVDA needs an explicit aria-selected="false" instead of the absence of the attribute, otherwise it will announce all the tabs as "selected".

This ticket was mentioned in Slack in #core-media by karmatosed. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-media by anevins. View the logs.


3 months ago

#21 @anevins
3 months ago

Hey @afercia , just checking you're okay owning this for 5.3? If not, no pressure, we can get this reassigned.

#22 @anevins
3 months ago

Would you also be waiting on design feedback too @afercia ?

#23 @afercia
2 months ago

  • Keywords needs-refresh added; needs-design-feedback removed

I'm progressing slowly on this one, as there are some technical details to solve. Can own it.

Design feedback will be addressed in #47610, if need be. Removing the keyword.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


2 months ago

#25 @joedolson
2 months ago

Thanks for your work on this, Andrea!

I think that your proposed solution should work. Regarding the mobile view, I think that switching the dropdown into a disclosure widget will give the necessary affordance for the interface to make sense.

The fact that the H1 heading is being used to toggle that dropdown will definitely need to change - the control needs to be a clickable element, and the text should be a) constant and b) something that conveys what options are in that tab panel. I'm not honestly certain how to describe that, however - those choices are fairly diverse.

Something like "Media Tools"?

#26 @afercia
2 months ago

the control needs to be a clickable element,

Thanks @joedolson! I guess you meant keyboard operable? :)

#27 @joedolson
2 months ago

Eh. Yeah, that's what I meant. ;) Well, literally, I meant "an element that is natively clickable", but yeah - not stated clearly.

Last edited 2 months ago by joedolson (previous) (diff)

#28 @afercia
7 weeks ago

  • Keywords needs-patch added; has-patch needs-refresh removed

Work is progressing. I'm working on the menu toggle in the responsive view: the H1 heading is now just a heading and I added a button to toggle the menu.

I'd need some feedback on the best option for best visual order / DOM order. The styling is temporary, please focus on the order issue :)

Option 1:

  • button placed in the most intuitive spot from a visual perspective
  • unfortunately, the heading is between the toggle and the menu: in the DOM the menu doesn't immediately follow the toggle

http://cldup.com/Jd_ZdqnxXB.png

Option 2:

  • button placed after the heading
  • DOM order is ideal
  • visually: not great because button and menu are "disconnected"
  • also: the heading text length varies depending on the selected view thus the button "jumps" horizontally when selecting a menu item

http://cldup.com/GGQGx5wHos.png

Option 3:

  • everything moved to the right
  • DOM order is ideal
  • there's no "jump"
  • maybe not great visually
  • could be out of view at high magnification levels (proximity)

http://cldup.com/PaI4pTFCqL.png

Feedback and suggestions for alternative options welcome.

Last edited 7 weeks ago by afercia (previous) (diff)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 weeks ago

#30 @afercia
7 weeks ago

Option 4, with some refactoring of the modal top part:

  • heading and button stacked vertically
  • visual order and DOM order match: heading > button > menu
  • the heading doesn't necessarily need to be "big"
  • of course the font sizes can be adjusted and some more white space added: this is just a quick in-browser editing to illustrate the idea

http://cldup.com/1JgBlRBe86.png

Last edited 7 weeks ago by afercia (previous) (diff)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 weeks ago

#32 @afercia
7 weeks ago

  • Keywords needs-design-feedback added

Discussed during today's accessibility bug scrub. Option 4 was indicated as the preferred option. Needs feedback from the design team though.

This ticket was mentioned in Slack in #design by afercia. View the logs.


7 weeks ago

@afercia
7 weeks ago

#34 @afercia
7 weeks ago

  • Keywords has-patch added; needs-patch removed

Pending design feedback on the menu in the responsive view, 47149.5.diff is a functional patch that implements the ARIA tabs pattern on both the left menu and the Upload/Library menu (called the "router" according to the Media Views conventions).

The patch looks good to me and I'd greatly appreciate some accessibility testing. Some bits in the patch necessarily target DOM elements: maybe it could be improved in a bit more "backbonish" way. If anyone is willing to introduce improvements, that'd be very welcome. I tried to keep the DOM-related methods as much isolated as possible.

This patch is also a good step forward in making the FocusManager a more general and powerful tool to handle focus management in the media views.

To test, you may want to activate the Classic Editor plugin to easily access all the media modal views.

Please test desktop and mobile views on at least these cases:

The "Post" modal dialog:

  • one of the main modal views e.g. "Add Media", "Create Gallery", "Create Audio Playlist", "Featured Image"
  • on these views, test also switching the inner tabs "Upload Files" and "Media Library"
  • test one or more of the sub-views e.g. "Edit Gallery", "Add to Gallery"
  • the "Insert from URL" view is a special case because the "Upload Files" and "Media Library" tabs are hidden there thus the inner tabpanel shouldn't be present, please check

The "Set featured image" modal dialog:

  • on this modal the main tabs are hidden so the main tabpanel shouldn't be present

The modal dialogs that open from the Customizer:

  • e.g. Site Icon or Header Media (depending on the active theme)

The modal dialogs that open from Gutenberg:

  • e.g. the Image block and the Gallery block
  • for example, in the modal that opens from the Gallery block the main tabs have only one tab: this is specific to the Gutenberg implementation and can't be changed. The ARIA pattern works correctly even if it doesn't make much sense for just one tab.

Optionally: any other media modal dialog using a different "frame".

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


7 weeks ago

#38 @karmatosed
7 weeks ago

I think there is something either up with trunk or the patch today, I will paste in what I see running it. I do however, get a white screen without classic editor plugin on trunk. My thinking is that something is interfering with your patch @afercia. I will take a look again tomorrow, however if you could post screenshots for me I can maybe get around any local issues (although not sure it's my install yet).

http://cldup.com/nV0btxKa-g.png

Last edited 7 weeks ago by karmatosed (previous) (diff)

#39 @afercia
7 weeks ago

Thanks @karmatosed. The patch works for me without Classic Editor. From your screenshot I can only see the styles are not loaded. Did you run grunt build? Or hard refreshed the page?

Regardless, design feedback would be appreciated specifically on the button to toggle the menu in the responsive view. See comments 28 and 29 with the 4 options and screenshots.

For accessibility we need the visual order match the source order and ideally this order:

  1. the modal title
  2. the button to toggle menu
  3. the menu

Based on that, option 4 looked the most promising to the accessibility team. I guess it would need some visual adjustment.

#40 @karmatosed
6 weeks ago

Thanks for the context @afercia. I have tested and got it working now, super weird but glad it's working after a fresh svn up, I had done grunt build so assume caught a bad build.

For those following along what I see now is:

http://cldup.com/JQdRv8xEHm.png

Based on that I have a few iterations, I am not sure the side menu one is ideal from usability or a11y but adding incase. For me having menu on a new line does help some visual issues.

http://cldup.com/3Ipo1AYLNS.png

http://cldup.com/kpNB3s7LlZ.png

cc @melchoyce for a second design input here. Likely we do need styling for this but for now focusing on placing as styling can happen after beta 1.

#41 @melchoyce
6 weeks ago

I think we should center the title regardless of the direction we take, but here are two ideas:

  1. The title becomes the menu dropdown as well, since it's the same label as the selected menu item
  2. We update the menu title from "menu" to "media types" to be more explicit about what content you'll find inside the menu.

#42 @afercia
6 weeks ago

@melchoyce thanks for sharing feedback. A few, quick, considerations:

  1. Right now, the title is already the menu toggle: this is exactly what needs to be changed :) There's the need for a UI control (a button) that communicates _what_ it does, e.g.: "(choose) media types". Instead, in the current UI and in the mockup #1 it would communicate the currently selected item.
  2. The second mockup is better for accessibility except for the order. As outlined previously, visual order and source order must match. We need this order:
    • Title
    • Menu toggle
    • The menu itself

This ticket was mentioned in Slack in #design by afercia. View the logs.


6 weeks ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


6 weeks ago

@karmatosed
6 weeks ago

#45 @karmatosed
6 weeks ago

My above attachment is rather large (sorry about that), but I think is a good middle ground to then iterate in beta 1. @melchoyce I would love to see your input on where this could be iterated as hope was ok but I took your work and found that middle point.

@afercia hopefully this can get in for beta 1 and then see that iteration.

@melchoyce
6 weeks ago

#46 @melchoyce
6 weeks ago

Thanks @afercia. Does Media Tabs.png fill all the necessary criteria?

#47 @afercia
6 weeks ago

Thanks @karmatosed. @melchoyce interesting idea. I'm trying it. Two considerations:

  • On intermediate viewport widths the modal sidebar is still visible on the right and has a gray background: the white tabs on the right don't look great with that gray background underneath :(
  • On small viewport widths, there may not be enough room for longer translations. Worth reminding there's a sort of unofficial support for viewport as narrow as 320 pixels.

See screenshots below.

I genuinely like the idea. I can see the symmetry and how the white space helps. However, on the responsive views, this kind of horizontal alignments are always tricky (the 2 horizontally aligned buttons in the customizer come into my mind).

I have no strong preferences. Both the last mockup by @karmatosed and your one solve the order issue. Just let me know what is the design team preference :)

#48 @melchoyce
6 weeks ago

Could we change the tab background color between when "Media Types" is collapsed down into a menu, and when the grey sidebar disappears, so that it matches the sidebar background color?

For translations — can those labels wrap onto multiple lines, or are the heights hard-coded?

#49 @afercia
6 weeks ago

Could we change the tab background color between

Not sure I fully understand but everything is "fluid" there and it's getting a bit complicated :)

For translations — can those labels wrap onto multiple lines, or are the heights hard-coded?

Most of the elements there have fixed heights and are absolutely positioned... almost impossible to allow line wrapping unless we'd want a major refactoring of all the media modal :(

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


5 weeks ago

#51 @afercia
5 weeks ago

  • Type changed from defect (bug) to task (blessed)

The only part missing here is CSS adjustments for the media modal top part, which currently looks like in the screenshot below. Desktop looks like in the previous screenshots on this ticket.

I'd like to propose to commit this for WP 5.3 Beta 2, as the functional / JS part is ready since a few weeks. The small CSS adjustments can be iterated later.

@afercia
5 weeks ago

#52 @afercia
5 weeks ago

47149.6.diff refreshes the patch against latest trunk.

#53 @afercia
5 weeks ago

In 46363:

Accessibility: Media: Use the ARIA tabs pattern for the media modal menus.

The ARIA tabs pattern improves interaction for keyboard and assistive technologies users.
It gives the menu items proper roles, and aria-selected allows users of assistive technologies to know which tab is currently selected.

Props audrasjb, afercia, joedolson, karmatosed, melchoyce.
See #47149.

#54 @afercia
5 weeks ago

In 46375:

Accessibility: Media: Add more headings in the Media Modal.

Headings are the predominant mechanism for screen reader users to find information in a page. They also help all users to better identify the main sections of user interfaces.

  • adds three new headings within the media modal
  • improves plural form translation for "item selected" by using wp.i18n
  • horizontally centers the media modal menu in the responsive view

Props kjellr, karmatosed, melchoyce, afercia.
See #47149.
Fixes #47610.

#55 @mikeschroder
5 weeks ago

Perhaps this is problematic feedback at this stage, but I'm feeling like "Media Types" doesn't communicate to me the options that are inside. Especially with Single image vs Featured Image vs Gallery, which are both the same "Media Type" of Image.

I like that it reduces duplicate headers, but as a user, it's confusing to me what I'd go into "Media Types" to find.

I'll think about alternate wordings or options, too, but wondered if anyone else had thoughts on this, or ways to make it clearer. I worry it'll make the other items much less discoverable.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


5 weeks ago

#57 @afercia
5 weeks ago

Sure. This was noted in https://core.trac.wordpress.org/ticket/47610#comment:38 where a few suggestions were made. It would be only a string change. Suggestions welcome.

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 weeks ago

#59 @afercia
4 weeks ago

  • Keywords needs-design-feedback removed

The only part missing here is CSS adjustments for the media modal top part

This has been addressed in #47610 see starting from comment:35:ticket:47610 and the screenshots there: the toggle button and menu are now centered.

About the copy: suggestions welcome.

@afercia
4 weeks ago

#60 @afercia
4 weeks ago

Noticed an edge case where the ARIA tabs markup is rendered even if there are no visible "menu" and "router" tabs. Learned the state attributes contain information on the two menus and it's possible to additionally check for them to better determine when the ARIA tabs markup needs to be rendered.

47149.7.diff uses this approach to improve the related conditional checks and remove a no longer necessary conditional check.

#61 @afercia
4 weeks ago

In 46438:

Accessibility: Media: Improve the logic to render the ARIA tabs markup after [46363].

Additionally checks for the "menu" and "router" state attributes to better determine when the ARIA tabs pattern markup is appropriate.

See #47149.

This ticket was mentioned in Slack in #core by marybaum. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


4 weeks ago

#64 @mikeschroder
4 weeks ago

We talked about this during Media triage, linked above.

@joemcgill mentioned that this area also gets used for non media related things, like in Shortcake.

Would it be possible to go with "Actions" instead of "Media Types", similar to @melchoyce's recommendation?

#65 @afercia
4 weeks ago

Would it be possible to go with "Actions" instead of "Media Types"

There's already a visually hidden heading "Available actions" placed before the bottom toolbar, see #47610. A toggle button named "Actions" placed in a different section would be a bit confusing.

I'd totally agree this toggle button (and the companion heading from #47610 would need a more generic name as the menu items relate to different actions. There are also "Edit" and "Cancel" actions in the sub views, see attached screenshot for some examples.

Maybe something like "Manage media" or just "Manage"?

Worth also reminding this toggle button appears only in the responsive view, while the heading is visible only on desktop.

I'm not sure I know what "Shortcake" is and what it does in the media views. Not sure other contributors or readers know as well. Can you please add some more details for reference and also for history when this ticket will be read a few years from now?

Regardless, I'm not sure core should base UI improvements on what plugins do in absences of a formal API or formalized UI patterns. :)

@afercia
4 weeks ago

Menu items examples

#66 @azaozz
4 weeks ago

  • Keywords needs-post-mortem added
  • Type changed from task (blessed) to enhancement

Re: process, this ticket was supposed to be a bug fix to change some links into buttons so assistive technology may work better but has turned into an "arbitrary enhancement" for the media modal instead. Being an enhancement, it shouldn't have been committed during beta.

In addition, looking at the above screenshot the "Media Types" label/heading is confusing and degrades user experience (even if changed to "Actions"). What does "media types" have to do with "cancel gallery", and why would the menu need a separate title? The modal already has a title, in this case it's "Create Gallery", that states the purpose a lot better.

Imho this should be reverted.

Last edited 4 weeks ago by azaozz (previous) (diff)

#67 @mikeschroder
3 weeks ago

Sure, @afercia -- here's a link to check out Shortcake.
The screenshots show the relevant use: https://wordpress.org/plugins/shortcode-ui/

#68 follow-up: @afercia
3 weeks ago

  • Type changed from enhancement to defect (bug)

@azaozz thanks for sharing your concerns.

Re: process, this ticket was supposed to be a bug fix to change some links into buttons

Not exactly. The actual point here is that the visual presentation looks like "tabs" but they weren't real tabs, not providing the expected semantics and interaction of the ARIA tabs pattern. Also reported by the WPCampus audit as "medium" severity, ie: not a minor thing. As such, this was a bug.

but has turned into an "arbitrary enhancement" for the media modal instead. Being an enhancement, it shouldn't have been committed during beta.

Being a bug, it can be committed at any time during beta.

Not sure what do you mean by "arbitrary enhancement". The change to ARIA tabs implied other changes including CSS especially for the mobile part.

Please notice the patch for this ticket was ready since weeks. See comment:34. Design feedback was a bit late and I had to wait for their feedback, That's why I opted to commit the patch and change this ticket from "bug" to "task (blessed)" to give design the time for feedback and iterate. See comment:51.

the "Media Types" label/heading is confusing and degrades user experience (even if changed to "Actions"). What does "media types" have to do with "cancel gallery", and why would the menu need a separate title? The modal already has a title, in this case it's "Create Gallery", that states the purpose a lot better.

There are two different things here:

  • the "Media Types" heading pertains to #47610 see there for the good reasons to add more headings within the modal
  • the "Media Types" toggle button that appears in the responsive view to toggle the menu: this is a button and needs to identify the available action. As in "what happens when I click this button". "Create Gallery" (or similar) is not a proper text for this button simply because when you click the button it doesn't create a gallery :) It opens the menu. Also, initially the button text was "Menu": simple and effective. It was changed to "Media Types" after design feedback. see comment:41.

Re: the wording, I'd agree it needs to be improved for both the heading and the button. This was noted on this ticket and also on #47610. The current proposed text are the following ones. Please just let me know which one is preferred, I haven't got a final feedback so far.

  • Settings
  • Actions
  • More...
  • Manage media
  • Manage
  • Menu

For the button, personally I'd lean towards "Menu" because that's what the button does: it opens and closes the menu.
For the heading, it needs to identify what that section of the media modal is about. I'd lean towards "Manage media" but this should be discussed on #47610.

That said, I'd like to change this ticket back to "bug" because it's a bug not an enhancement.

#69 @afercia
3 weeks ago

@mikeschroder thanks for the link :) It took me a while to understand I had to test with the Shortcake plugin and at least the Shortcode UI Example plugin to actually see how it works.

So basically Shortcake reuses the media modal to insert and manage content that is not strictly "media". I'm not sure this is a good idea in the first place as many things in the media views and the media modal UI are specifically designed for media. Headings, visually hidden text, labels, can't take into consideration content different from media. The media views ar not part of a formalized API and, though they're extensible, they're still designed exclusively for media.

That said, as far as I see Shortcake uses two frames: the "add" one and the "edit details" one.

The "add" one adds an "Insert Post Element" menu item and content:

http://cldup.com/XHYZHRURWw.png

This is definitely not only "media". It's used to insert also shortcodes, or a Facebook post, or a Pullquote etc.. To me, it doesn't seem appropriate and contributes to a bad user experience in my opinion. Regardless, I still agree the toggle menu button and the heading need a better text.

Aside: the ARIA tabs pattern works correctly also with the "Insert Post Element" menu item.

In the "edit details" frame it's interesting how Shortcake allows to open an additional modal from the main modal (see screenshot where I shifted one of the modals to reveal the underlying one). Not sure this is good UI and UX:

http://cldup.com/KXqPwsaA-G.png

Notice on the left the inputs and buttons with the new styles look OK.

It is important to note that in the "edit details" frame the heading, toggle button, and menu are hidden with display: none thus they're not a problem. This is because the ARIA tabs implementation and the heading take into consideration the hide-menu state of the frame view.

Lastly, Shortcake uses select2 to replace native <select> elements. The new CSS conflicts a bit with the select2 one and there's a minor visual glitch but select2 is still functional and can be used without any breakage. The minor CSS glitch should be addressed on the Shortcake side.

Worth also noting that it doesn't seem to me Shortcake is so actively maintained. For example, when typing into the "Select Page" field the popup placement is wrong: the instructions popup and the suggestions popup appear completely out of place, also on WordPress 5.2:

Screenshot from WordPress 5.2:

http://cldup.com/lPzSGNzUPp.png

Overall, I don't see serious breakages in Shortcake produced by the admin CSS changes.

Re: improving the button and heading wording: I'll take the responsibility to prepare two patches: on this ticket and on #47610.

@afercia
3 weeks ago

#70 @afercia
3 weeks ago

  • Keywords i18n-change added

47149.8.diff changes the text of the toggle button that appears in the mobile view from "Media Types" to "Menu". This button does only one thing: it opens and closes the menu so the text needs to communicate what the button action is. I'm avoiding to use the verb "toggle" because of #34753.
See screenshot below.

@afercia
3 weeks ago

New toggle button text

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


3 weeks ago

#72 in reply to: ↑ 68 @azaozz
3 weeks ago

Replying to afercia:

Being a bug, it can be committed at any time during beta.

I agree, part of the code committed here is to fix a bug that is clearly described in the ticket description. But later another (larger) part was added which is an enhancement.

Adding enhancements to "bug fix" tickets just to be able to commit them during beta is not good :(

From pure "coding" point of view the facts are that these additional changes were added far too late in the development cycle and there is not enough time to properly test them and ensure they do not bring regressions.

#73 @afercia
3 weeks ago

Sure. I'd just like to note again that a functional patch was ready on 09/18/2019, which is almost one month ago and 5 days before beta 1. I could have committed that patch on the same day because it solved the accessibility bug and was acceptable from a design perspective.

For respect of other teams I asked for design feedback. They asked for some CSS changes. To me, they were not strictly necessary. At this point I'd think it's very important for the future to discuss how to improve the cross-teams collaboration process in order to avoid important bug fixes and improvements are de facto blocked by other teams.

#74 @afercia
3 weeks ago

P.S. can I commit 47149.8.diff which only updates one string?

#75 @audrasjb
3 weeks ago

@afercia 47149.8.diff looks good to me. I'm only wondering about adding contextualisation/translator comments or not.

#76 follow-up: @sabernhardt
3 weeks ago

I agree with the text changes in these two tickets. (I was planning to post this and the images on the other.)

"Actions" makes more sense than "Media Types" for that heading (especially on the Audio and Video screens), and "Selected media actions" makes sense in place of "Available actions" (plus "Menu" would be better for the current "Media Types" button in ticket #47610). Though there may be a more appropriate text string, I struggle to think of one.

@azaozz If I understand your complaint correctly that you mainly do not want that specific heading to be visible in this release, we could edit the text strings and use the "screen-reader-text" class (for now) to improve the experience for assistive technology users. Then we could revisit whether/how the Actions heading should appear in a future release.

(I don't like how much the heading looks like the current tab button text, but I prefer it to all-uppercase text and I would prefer visible headings in general.)

And yes, adding context for translators would be helpful :)

Last edited 3 weeks ago by sabernhardt (previous) (diff)

@sabernhardt
3 weeks ago

Video Details modal with visible "Actions" heading

@sabernhardt
3 weeks ago

Add Audio modal with visible "Actions" heading

#77 @afercia
3 weeks ago

I don't like how much the heading looks like the current tab button text,

I'd agree the styling could make the heading less prominent :) Normal font weight and the default font size used within the media modal could help. The current styling just implements the design feedback provided on #47610. I guess it's a bit late for one more round of design feedback, screenshots, etc. but I wouldn't be opposed to try it.

@afercia
3 weeks ago

#78 @afercia
3 weeks ago

47149.9.diff adds translation context.

#79 @afercia
3 weeks ago

In 46489:

Accessibility: Media: Improve the text of the Media Modal actions toggle button displayed in the responsive view.

See #47149.

@azaozz
3 weeks ago

#80 in reply to: ↑ 76 @azaozz
3 weeks ago

Replying to sabernhardt:

@azaozz If I understand your complaint correctly that you mainly do not want that specific heading to be visible in this release...

Yes, the word "Actions" doesn't seems to improve anything in that context.

Also, why the "double" heading, could be confusing? And why the "main" heading is less important (smaller) than the menu sub-section heading?

#81 @afercia
3 weeks ago

If I'm not wrong, "Add Audio" is used by the Audio widget modal and it's a button, not a heading.

Re: the styling, please ask the design team :)

If it was up to me, I'd make that heading use a normal font weight to make it less prominent. Maybe also a smaller font size. Same for the filters heading.

@afercia
3 weeks ago

Less prominent headings example

@afercia
3 weeks ago

If this UI was made with HTML, those would be headings and they're less prominent than the items

#82 @afercia
3 weeks ago

By the way, the ticket for the headings is #47610 :)

#83 @melchoyce
3 weeks ago

While I don't love the all-caps and would like to see us change them all in the long-run, for now using that particular h2 styling would match the other sidebar h2s, and the lighter color helps de-emphasize it.

#84 @afercia
3 weeks ago

@melchoyce thank you, but as mentioned in the tickets for the headings (this one is about the ARIA tabs), all-apps should be avoided as it a) doesn't help readability b) screen readers may announce it as an abbreviation, speaking out letter by letter.

https://core.trac.wordpress.org/ticket/47610#comment:23

All uppercase should generally be avoided and this is a good opportunity to change it.

#85 @melchoyce
3 weeks ago

b) screen readers may announce it as an abbreviation, speaking out letter by letter.

Does that still happen when something is converted to caps using CSS?

#86 @afercia
3 weeks ago

Does that still happen when something is converted to caps using CSS?

Yes (weird, I know).

#87 @melchoyce
3 weeks ago

Dang, that's annoying.

#88 @afercia
3 weeks ago

Yep, bit annoying.

Worth also reminding the conversation on this Gutenberg issue, where the consensus is to start using sentence case and then iterate / propagate in core: https://github.com/WordPress/gutenberg/issues/16764

#89 @desrosj
3 weeks ago

@afercia RC1 is set to be packaged in a few hours as soon as a few more blockers are cleared up.

Can you please briefly summarize the status? Is there anything else required for this ticket? If not, can this be closed out in favor of follow up tickets?

@azaozz Do you consider an answer to your question a blocker for RC?

#90 @afercia
3 weeks ago

@desrosj thank you for the reminder. I'm very well aware of the deadline :) From a functional perspective, this ticket is complete and can be closed.

On the latest comments, some concerns were expressed about the "prominence" of the new heading in the left sidebar. Happy to implement any design feedback to make it less prominent. Some feedback from the design team would be appreciated, otherwise I guess we can iterate in the next release cycle.

If I can suggest a possible option:

  • not sure why the active items in the left sidebar are black in the first place
  • interactive elements in WordPress are blue, that's a well established convention
  • I'd like to propose to keep the active item blue (and bold)
  • Unbold the new headings

This way, the heading would be less prominent. See attached screenshot.

@afercia
3 weeks ago

Making the heading less prominent

@afercia
3 weeks ago

#91 @afercia
3 weeks ago

47149.10.diff implements the above suggestions. Please just let me know if there's any objection to commit. Thanks.

Last edited 3 weeks ago by afercia (previous) (diff)

#92 @afercia
3 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

Actually, the ticket for the headings is #47620. Will move 47149.10.diff there and close this ticket. Please post any comment related to the headings on #47620. Thank you! :)

Note: See TracTickets for help on using tickets.