Data Views list layout: revise for improved text truncation #65376
Conversation
stokesman
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -106 B (-0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
| id={ generateItemWrapperCompositeId( idPrefix ) } | ||
| aria-pressed={ isSelected } | ||
| aria-labelledby={ labelId } | ||
| aria-describedby={ descriptionId } | ||
| className="dataviews-view-list__item" | ||
| onClick={ () => onSelect( item ) } | ||
| /> |
There was a problem hiding this comment.
I'm a bit worried about having a button with no contents, is there any way we could move all cell content (except for the actions buttons) inside it?
There was a problem hiding this comment.
Yeah, this is the bit I wondered most about but hoped the existing aria-labeledby and aria-describedby makes it okay.
move all cell content (except for the actions buttons) inside it
The actions have to be a sibling of the title for the truncation to "just work". So both would have to stay outside the main button, but then they’d not be able to impact the layout of other content. Not to say that can’t be made to work, but then the title is outside the button anyway.
There was a problem hiding this comment.
I did some testing with VoiceOver and it announces the same content as on trunk when navigating the rows and gridcells here. I suppose to be totally sure testing with other screen readers is needed but I expect they all do well as support for aria-labelledby ought to be good.
There was a problem hiding this comment.
@afercia maybe you could advise if this change is OK in terms of accessibility, or if it has unwanted consequences?
There was a problem hiding this comment.
@ciampo Technically, it's not that different from an icon-button where the SVG icon is aria-hidden so that the button is 'empty'. However, it's always preferable to not use ARIA when possible. We shouldn't assume all assistive technologies understand ARIA, for example with some combination of browsers and speech recognition software. Is it possible to use visually hidden content instead of aria-label?
Anyways, I'm not sure this is the only problem with the list view implementation and the dataviews in general as an in-depth accessibility audit has still to happen and that's something I can't do alone.
noisysocks
added
the
Backport to WP 6.7 Beta/RC
|
This looks good to me from the design perspective 👍 |
|
I've asked for some a11y-specific feedback. If I have time, I'd also like to try an alternative approach, since there are a couple of additional things about this use of |
|
Whel investigating for #65376 (comment) I noticed that the Edit and Action buttons have a label and description with the same text. This produces a double announcementfor screen reader eusers as both the label and description are announced:
I think but I'm not sure this is caused by the ariakit tooltip that automatically adds a description. As mentioned in other issues, the tooltip should not be used for the description and should only be used or the accessible name (the label). |
There was a problem hiding this comment.
Thanks for the PR! This works great in my testing. I noticed something that could potentially be addressed in a follow-up, though:
- Hovering on the list item doesn't show the pointer cursor anymore.
- Since we truncate the text if it's too long, should we also show the full title when it's being hovered over? For instance, a tooltip or a
titleattribute (that's often considered inaccessible though).
|
@kevin940726, Thanks for testing and reviewing.
Thanks for spotting that! It seems to warrant fixing here so added a commit that should take care of it.
Perhaps, it seems like enough text is visible to identify the entity so I’m not convinced it’s too important. Yet, if we think it’s helpful I'm not opposed. I believe the full title will be available to screen readers so maybe a As for landing this, @ciampo had mentioned he’d like to try an alternative #65376 (comment) so we may wish to wait for that. Yet he did mention additional improvements (a larger scope) so maybe it’s more of a follow up 🤔. |
Please don't introduce any |
There was a problem hiding this comment.
I've taken this for a spin as well. As @kevin940726 notes it's working well for me.
Hovering on the list item doesn't show the pointer cursor anymore.
Thanks for spotting that! It seems to warrant fixing here so added a commit that should take care of it.
With the latest commit, the pointer cursor now appears when hovering the list items.
It does seem like a follow up consideration.
Agreed.
As this is a bug fix, there's still a small window where we can hold off on landing this while the alternative approach is explored.
If the scope of the alternative does prove too great, I don't think there is a lot of harm in merging this one and iterating from there. Just my two cents of course 🙂
|
Excuses for the delay in reviewing / proposing an alternate approach, WP release times can be hectic 😅
Agreed, let's merge this approach, and I can iterate on it anyway. Thank you for waiting on me, though ❤️ |
stokesman
force-pushed
the
fix/data-views-list-primary-field-truncation
branch
from
04da22b
to
9601c78
Compare
|
I rebased on trunk to see if helps some of those e2e tests to run and enabled auto-merge. Thanks everyone for testing and reviewing! |
|
Flaky tests detected in 59e8b7d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11241830129
|
|
Thanks to an e2e test failure I realized we’d overlooked a difference. The primary field may contain a link and the changes here were making it so it was behind the main button and couldn’t be interacted with. Here’s a comparison to help illustrate:
The latest commit has it working again. I‘d prefer if there was a nicer way the component could know if the field is interactive and should be in front. It can’t unconditionally put it in front because when not interactive it creates a dead spot where clicking won’t activate the main button. |
If that link is part of a composite item, that could be problematic in itself. It would be like rendering a link inside a button, which poses semantics and accessibility issues |
|
I don't know how feasible this is but ideally in List layout the primary field should not render as a link. |
|
Is the "primary field as a link" something that can already happen on |
Good to hear, I suspected that was maybe unintended for the list view.
Yes, and on trunk it’s basically a link inside a button since the composite item has Still at the least it sounds like |
I see. Although I imagine it would be weird to have a link in between items of a composite widgets. IMO we should consider removing the link altogether — where does it even link to?
I think we should try to prevent inaccessible / non-semantic scenarios from happening. The primary field being a link could potentially be problematic also in other layouts (ie. grid). If we really need to have some sort links, we could consider adding it to the dropdown menu? |
|
FWIW, I think using links is better for accessibility if we take client side routing into consideration. There are a number of advantages over using buttons:
That said, it's just my two cents and seems unrelated to this PR 😅. Do you think we can merge this for 6.7 and address follow-ups in other issues? |
|
If we think that this PR is an improvement over what's on Separately (and not for the 6.7 release) — I still think that this part of the UI needs a rewrite. If we want to retain the link, we should make the whole composite item a link, so that there's no "link inside button" nesting. I'll see if I can come up with a good solution in the upcoming weeks |
|
A detail to explore separately for sure, but perhaps records should be able to provide a It could potentially be abstracted further with an |
I don’t see a regression but I'm also not sure there’s much urgency to this or if it’s truly qualified for a backport to 6.7 because I’m not certain the hardcoded colors thing wasn’t already present in 6.6. EDIT: I just checked on playground with 6.6 and the list view didn’t have the hardcoded colors (the list layout was different back then) so I guess this does qualify. Aside: props to all involved here. You are some smart cookies ❤️. |
|
Hey folks, just catching up here. Is this still something we want to get into 6.7, or punt to 6.8? It seems like that's still an open discussion. RC1 is taking place later today. |
|
It would be good to include this in 6.7 if possible as it fixes a visual bug. |
I agree, but this PR will need to be merged asap to get it in. cc @getdave @kevin940726 |
|
I couldn’t see a reason not to. I’d been waiting to give a chance for folks to find one. |
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
* Remove background-color and box-shadow from actions * Shorten primary field’s text on hover or selected state * Data Views list layout: revise for improved text truncation * Let primary field be full with in resting state * Remove seemingly errant border radius on item wrapper * Unnest actions gridcells * Fix cursor style of list items * Update test selector * Revert "Update test selector" This reverts commit 59e8b7d. * Put primary field in front if it has interactive elements --------- Co-authored-by: colorful-tones <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: afercia <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: ndiego <[email protected]>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 1ea0edd |
Awesome, thanks @stokesman! |
What?
A revision of markup and style.
Why?
To fix #65215.
The list layout design is tricky to implement—particularly, its hover/focus/selected states. The current layout trick relies on an opaque layer to further "truncate" the title text while hovered/selected/focused. Right now, there doesn’t seem to be a way to avoid the hardcoded colors while retaining the current layout trick.
The approach in this PR avoids hardcoded colors. Besides that, I think it’s nicer that the ellipsis remain visible in all states.
How?
Makes the actions (
__item-actions) a sibling of the title (__primary_field) so when the latter appears, the former truncates as much as needed. This is permitted by making the item’s primary button absolutely positioned and moving its contents out to avoid buttons or othergridcellroles from nesting.Testing Instructions
Inspect/scrutinize all interactions with the list view in all contexts (viewing templates/pages/patterns) and with all "View options" combinations of available properties visible/hidden
Testing Instructions for Keyboard
Screenshots or screencast
Overlaid before and after to verify preserved metrics
before-after-overlay.mov