Add summary prop to PanelBody, and use it to create more accessible pre-publish panel sections. #25170
Conversation
ZebulanStanphill
added
[Focus] Accessibility (a11y)
ZebulanStanphill
requested review from
afercia,
ItsJonQ,
ellatrix and
epiqueras
ZebulanStanphill
requested review from
ajitbohra,
chrisvanpatten,
Soean,
talldan and
youknowriad
as code owners
|
Size Change: +165 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
ZebulanStanphill
force-pushed
the
add/panel-body-summary-prop
branch
from
071597c
to
d775c41
Compare
ZebulanStanphill
force-pushed
the
add/panel-body-summary-prop
branch
2 times, most recently
from
dcff01e
to
2a4456f
Compare
|
Cool, thanks @ZebulanStanphill ! This would finally solve #470 by splitting the button action, which needs to be communicated by the button text (accessible name in the accessibility tree) by the state, which needs to be communicated separately. Totally in favor of this approach 🎉 Tested it on both Windows and macOS. On Windows Firefox + NVDA it works like a charm:
On macOS, for some reason Safari and VoiceOver do not announce the description 🙁 I think it's some weirdness with VoiceOver. To be sure it's not something related to some JS or CSS, I made a reduced test case on this codepen: https://codepen.io/afercia/full/bGpvGjK. The examples there have slight variations in the HTML and for some of them the description gets announced, for others it doesn't. Sometimes some screen readers are very sensible to what they consider "unexpected" markup. There are a few options to solve this issue with their pros and cons. In general, I'd recommend to keep the markup as simple as possible. Other minor notes: Not sure the section and header elements are really necessary. These panels are basically accordions and a list of headings with their panels should be enough. Also, I'm not sure assistive technologies communicate in any way the semantics of section and header.
Hm, I'd say only the button should have a click event. Making a non-interactive element interactive would require to assign it a role (button?), replicate the expected interaction, add a focus style, etc. And at that point there would be two nested interactive elements. If the purpose it to extend the clickable area on the description, maybe there are CSS tricks to do that or maybe we shouldn't do it in the first place :)
I was initially concerned about the disappearing of the description. Preferably, it should stay visible. If design prefers to hide it when the panel opens, after all users have the full info available within the panels. Lastly, I'm not sure this pattern to build the strings is good for translations: Ideally, there shouldn't be "variable parts" within a string. I do realize it isn't possible to build specific strings for all the possible cases (especially for the publish date/time). Maybe worth considering to move out the variable part from the strings and use only |
ZebulanStanphill
force-pushed
the
add/panel-body-summary-prop
branch
from
b108034
to
a781ba2
Compare
|
Interesting that on VoiceOver, the description gets announced when its inside the I'm not really sure what to do here. Putting the description inside the As for the whole I've altered the component to keep the summary visible when the section is expanded. I've also adjusted the pre-publish section summary strings as you suggested. I wasn't really sure what to put in the translator comments. So please check those out and let me know what you think.
|
Yeah... When it comes to screen readers it's like going back in time 15 years and have to struggle again with browser quirks everywhere... screen readers are similar to those old browsers. They have lots of quirks, sometimes triggered by the way browsers expose information, sometimes it's just the screen reader. Yep the description wouldn't be great inside the h2. I could live with the visibility one, but the publish date/time would be terrible. Also, this is a generic reusable component to it needs to suit many cases.
It seems it doesn't. But it also doesn't have any benefit? Semantics is not an abstract thing that "works out of the box". And it's not for humans. It's only perceived by software and when software don't communicate a specific semantic structure to users maybe that semantics is redundant? :) |
|
I would have thought the (Actually, come to think of it, why is So what do we do about the VoiceOver thing? Which is better: the current design, or having the description inside the heading? |
|
I'd lean towards keeping the markup simple, for now.
🤷 Yep. Actually, these are accordions. Maybe they should be named for what they are 🙂 In the future we may want to consider to implement some missing bits of the expected interaction, namely: arrows navigation through the accordion headers. See https://www.w3.org/TR/wai-aria-practices-1.1/#accordion
Unfortunately, the description inside the heading would be incorrect. I gave it another round of testing and updated the codepen. Couldn't find a clean solution though. 😞 To me, this seems a Chromium bug. Chrome and Opera (on macOS) show the same buggy behavior. I can't think of a workaround off the top of my head. Asked around on Twitter, maybe someone in the hope someone else can help. |
|
Noting that VoiceOver users can always explore the content by arrowing (Control Option Right or Left arrow keys) so the state is available for them, in a way. Clearly, it would be great to have the description be announced also when they Tab through the buttons. |
ZebulanStanphill
force-pushed
the
add/panel-body-summary-prop
branch
2 times, most recently
from
bc1a0f1
to
648d3fc
Compare
ZebulanStanphill
force-pushed
the
add/panel-body-summary-prop
branch
from
fd90c9a
to
a22a178
Compare
I've reverted to plain |
|
I had some feedback while this is open. These are the changes I would recommend making, but not sure how many of these are truly feasible for this PR and maybe they just need to be dealt with in the future.
Sorry if all of this is out of the scope of this PR. I do like the changes here and it does make the panel much more accessible. Those items above are really just dreams from me I think, but it would make it a lot better functioning in my opinion. Thoughts? |
|
@alexstine thanks for testing and for your feedback. I definitely agree that modal dialogs and well-implemented disclosure widgets are way more understandable and easy to use. To clarify a bit: right now, visually this looks like a Sidebar. That is: a column that slides in into the page from the right. We're going to discuss this Sidebar pattern more in depth in the next weekly accessibility meetings as we think it's far from ideal. To provide more in-depth feedback on the Sidebar pattern, I'd recommend to start from this new issue about the Inserter sidebar: #24975 A couple things:
I'd like this feedback from @alexstine to be well considered by the designers involved in the Gutenberg project as it well illustrates the struggles users have with some design patterns. Cc'ing @mapk |
|
@alexstine I agree with all of your suggestions, but it's certainly outside this particular PR's scope. If I have the spare time, I might try implementing your ideas in a separate PR later. (Or if anyone wants to go ahead and jump on that themselves, feel free to do so.) |
ZebulanStanphill
removed
the
Needs Accessibility Feedback
|
Hi @ZebulanStanphill 👋 Thanks for working on this. I have some design feedback on the placement of the summary prop and the overall focus area. Right now the summary prop feels very disconnected from the panel heading and in general feels out of place with the rest of the other panels that don't show a summary prop. We can work around that if we align the summary closer to the heading, something like this:
Notice how the label is closer to the heading and the toggle arrow on the right is center aligned. As for focus, it also currently feels very disconnected and it's weird that only half of the panel collapsed area is clickable. I think it's important we make the whole panel collapsed area focusable:
I undertand this PR is aimed for 5.6, I hope these suggestion can be ready on time 👍 Again, thank you for working on this. |
|
@enriquesanchez I think I can do that first thing. As for the second thing, however...
I have no clue how to do this. If anyone has any suggestions, please let me know. Remember, the summary must not be inside the |
@ZebulanStanphill 🤔 Maybe we can keep it out of the |
|
@enriquesanchez Yeah, that's what I was thinking, but I can't figure out a way to actually pull it off. A problem I keep running into is that the button needs to take up the entire click area of the header without changing the height of it. I get the feeling that any solution is going to feel really hacky and convoluted. Is it really worth it to have the summary be clickable? I'm starting to think not. Maybe we could find some other way to adjust the styles to make it look nicer instead of trying to make something look/act like it's part of a button without actually being part of it. Maybe the expand/collapse icon would feel less awkward if it was placed next to the button text instead of all the way to the right. |
|
@ZebulanStanphill I took a stab and I think I got it working, here's a POC in Codepen: https://codepen.io/enriquesanchez/pen/qBNqvzy What I'm doing is set Do you think we can try this for this PR? |
|
@enriquesanchez In my experiments, I did something similar. The unsolved problem is that the positioning/sizing doesn't account for the title or summary taking up multiple lines. I can't think of a way to set the height/positioning properly using just CSS, and I don't want to resort to using JS. |
|
@ZebulanStanphill Gotcha, I gave this another try. Here's another attempt that covers the case when the summary is multiple lines and the container needs to grow vertically: https://codepen.io/enriquesanchez/pen/yLJXddr The button is still focusable/clickable and covers the full width and height of the panel container. |
|
@enriquesanchez It looks like your demo doesn't work right on Chrome:
It looks fine on Firefox:
However, in both cases, while it does resize based on the number of lines the summary takes up, it still doesn't account for the section title taking up multiple lines. |
Yep, absolute positioning should be avoided also thinking at text-only zoom. There are CSS tricks to achieve the desired layout, for example by using CSS pseudo elements but they're sort of hacks anyways. I'd suggest to avoid hackish solutions. First, I'd consider a design solution that fits with the desired feature. Function should come first, visuals later. What is needed here is:
|
|
Having thought about it for a while, I really don't see any way to get this PR working both visually and functionally with the current design. Because of this dead end, I'm closing this PR. I think something like this might make for better UI: The Either way, such changes are different enough to warrant a new PR, and I encourage anyone interested in this idea to pursue it, because I currently do not have the spare time to do so. Hopefully, the discussion in #39082 will produce a UI pattern that works in both the Document Settings sidebar and pre-publish panel. |
Description
Giving buttons titles like "Visibility: Public" is not ideal for accessibility. See #470 (comment). But currently, we have a few disclosure widgets in the prepublish panel that use this anti-pattern.
This PR tries to solve that by showing the same info in a different way: it introduces a new
summaryprop to<PanelBody>that let's you show a summary of the panel's contents when the panel is closed, without messing up the label of the actual button to open the section.This PR also improves the semantics ofI have reverted to simply using<PanelBody>so it uses a<section>as its root element, and a<header>to wrap both the clickable header and the summary below. While this was not strictly necessary, I figured that since I had to alter the markup anyway (by adding a wrapping element to contain both the title and new summary), I might as well improve the semantics while I'm at it.<div>s to keep the changes in this PR smaller. We can always come change this is a future PR.The PR also currently alters two of the pre-publish panel sections as a demonstration. This can be cut out before merge and discussed in a separate PR to refine the specific text, or it can be kept at merge if desired.
I need feedback to ensure the current implementation is as accessible as it should be. Note that I have intentionally not made the summary clickable to open the section. Allowing it to be clicked may complicate the semantics and undo the whole point of trying to more accessible. I am not sure what works in an a11y context here, and would appreciate any feedback.
Would moving theonClickhandler to the<header>element be okay, or would that break a11y?I am also not certain if the summary should disappear when the section is expanded, or whether it should always be visible. I currently make it disappear, but I am again uncertain how this affects a11y.The summary is now always visible.Checklist: