Content structure: Fix Semantics

[marekhrabe]

Description

Fixes #15290

I hope I understood the proposed solution correctly and I've updated the markup from divs into ul>li. This came with an additional need to reset the margin to match the original styling.

After updating to <ul>, the outline showed up after opening the popover as the focus moves there:

The outline of <div> is disabled globally in wp-admin/css/common.min.css. There is no such rule for <ul> but to 100% match the original behavior and looks, I have disabled the outline for this specific usage. Let me know if I should keep the outline enabled so the result looks like the screenshot above.

Title was updated to use <h2>. No visual regression there - it looks the same.

How has this been tested?

• with post with some content
• open up the Content Structure
• confirm updated markup and no visual regression

Screenshots

[gziolo]

I have disabled the outline for this specific usage. Let me know if I should keep the outline enabled so the result looks like the screenshot above.

I don't think that the outline should be disabled. See #15479 which was merged this week by @jasmussen.

[jasmussen]

We need to indicate somehow what has focus, yes, especially when it's set like this. We have some options on visuals, though, and in the pr linked I've leveraged one style.

[afercia]

Left some comments for things that need to be changed.

Re: the redundant role="list" for lists: In a few places we're using it because Safari doesn't expose styled lists as lists. Consequently, VoiceOver doesn't tell anything about the list. From a recent conversation on Twitter with @cookiecrook, seems that Apple doesn't want to change that. Wondering if we should make lists with this (redundant) role a standardized component, as previously mentioned somewhere (can't find where). /Cc @aduth maybe your memory works better than mine.

[marekhrabe]

I believe I have addressed all the feedback now. I also noticed we disable the eslint rule and I used the same thing, including the explanation.

[talldan]

Hey @marekhrabe, this one will need to be rebased for the tests to pass.

There was fix merged to master to resolve an issue introduced by the WordPress 5.2 release.

[aduth]

Hey @marekhrabe, this one will need to be rebased for the tests to pass.

There was fix merged to master to resolve an issue introduced by the WordPress 5.2 release.

A build restart should be sufficient in most cases, as far as I've seen. I'll give it a try.

[gziolo]

Re: the redundant role="list" for lists: In a few places we're using it because Safari doesn't expose styled lists as lists. Consequently, VoiceOver doesn't tell anything about the list. From a recent conversation on Twitter with @cookiecrook, seems that Apple doesn't want to change that. Wondering if we should make lists with this (redundant) role a standardized component, as previously mentioned somewhere (can't find where). /Cc @aduth maybe your memory works better than mine.

It might be who mentioned it. I think it's still a valid proposal to introduce a hihger-level component for Lists which would ensure that all good practices are applied behind the scenes. In general, I don't think that you should be ever using raw HTML tag in Gutenberg outside of save definition of blocks. This would make it so easy to unify both visual aspect and behavior for all types of users.

[aduth]

Wondering if we should make lists with this (redundant) role a standardized component, as previously mentioned somewhere (can't find where). /Cc @aduth maybe your memory works better than mine.

No specifics come to mind, but it's been a recurring point across a number of separate issues that the abstraction of a component affords us a useful tool to guarantee consistency and best-practices across the entire application. A List component could be an example of this.

[marekhrabe]

Travis CI passed - we still need a code review here. @talldan @aduth @gziolo? 😇

Whether we go with List or just updating eslint to recommend the explicit role for lists, I believe both should be done separately on a whole repo scope.

[gziolo]

Code wise everything looks good.

Whether we go with List or just updating eslint to recommend the explicit role for lists, I believe both should be done separately on a whole repo scope.

Yes, this should be tackled separately. We should simply blacklist li from using in code outside of List component and inside every save method of the block. Well, it's not that simple, but I think the best way to move forward in general to let ESLint handle it.

@afercia, I did some testing but I would appreciate your sanity check to confirm that everything looks good. I hope I will gain more confidence over time and I won't have to ping you so often :D

[afercia]

@gziolo looks good to me 👍

• The roles are announced as expected.
• The list items are announced (1 of 4, 2 of 4, etc.) though it's a bit noisy together with the number of Words, Headings, etc.
• The h2 heading is announced and listed correctly, though I'm not sure it's actually useful. Of course, it's listed as last heading in the page, as the popover is rendered at the end of the source.

[gziolo]

Of course, it's listed as last heading in the page, as the popover is rendered at the end of the source.

Yes, it's kind of expected with how popovers are implemented. Isn't there an open issue to bring a similar behavior to popover like to modal, where all other DOM elements outside of popover are marked with some aria attributes which improves that experience?

[afercia]

Not sure there's a specific issue. It was discussed previously and yes maybe we should come to a decision on how to treat the various popovers.

[gziolo]

Thanks @marekhrabe for working on this issue.