-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add table block caption #15554
Add table block caption #15554
Conversation
It'd be great to get some accessibility feedback on this PR, especially on the editor implementation of the caption. Initially I thought this would be easy, but the tricky part is that the html table element has only a very specific set of elements it allows as children. I wasn't able to use a Instead I've added a normal caption tag to the table and made it Is there a better way to do this? |
Thinking that adding a |
Well ... I don't know why I didn't think of this before, but the obvious solution is to put the However, I tested it out, and that introduces other problems. The HTML spec mentions to put the The tbody also overlaps the caption's inline toolbar, and adjusting the z-index doesn't seem to resolve this. 🤔 Perhaps the original solution was the better one. |
@talldan I think it could be appropriate here to use the <figure>
<table><!-- ... --></table>
<figcaption>My lovely caption</figcaption>
</figure> See also: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/table#attr-summary
Edit: Aha! I didn't realize this was the original implementation. It seems to me like it would be a reasonable approach. |
Re: the div wrappers problem within the editor: Also, while HTML allows to omit the caption element when the table is wrapped in a figure with a figcaption, I'm not sure about actual support from assistive technologies. I'd recommend to use the Hiding the If it's not possible to use a The default placement of the caption should be at the top. That's the default for table captions. The placement at the bottom could be implemented as a style variation. Worth noting a table caption is something different from what generally Gutenberg means for "captions". Therefore, I'd also propose to rename this field to "Table caption". |
Many thanks for the feedback, @aduth and @afercia. I haven't had much time lately to develop this further. I'll do some thinking on this, and try to come up with a way forward. It sounds like I also need to do some testing to understand the difference between how caption and figcaption might work in assistive tech. |
1c315a4
to
95236f4
Compare
Have been doing some initial testing in VoiceOver of the various options for the table caption implementation. (cc: @afercia, would be interested in your thoughts on this). I've implemented option 2 described below on this branch if anyone wants to test it. Option 1 (RichText inside the caption element)
For:
Against:
Option 2 (RichText outside the table, visibly hidden caption)
Pros:
Cons:
Option 3 (figure/figcaption)
Pros:
Cons:
|
@talldan thanks for working on this. Right, I tested also on Windows and seems most of the browser / screen reader combinations have troubles with the editable within the Codepen for testing: https://codepen.io/afercia/full/RzbMvx The same happens with a native Will ping around to see if others have faced the same issue. For now, I don't have suggestions other than using a div element with an |
95236f4
to
e27beab
Compare
Thanks @afercia. Hopefully what I've done so far is the best compromise. The contenteditable has the aria label 'Write caption ...' at the moment, which matches the image block. |
f43f3cf
to
801f6cf
Compare
Hi! This is great. Any estimates on when we can see this in official WordPress? |
@talldan Looks good. There's just a failing test left? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. This should include a fixture for the deprecation(s) though. Why is there a deprecation is the first place?
Try using a figcaption for the table caption Revert "Try using a figcaption for the table caption" Switch back to a standard table caption This reverts commit b5f00dd. Add comment explaining location of RichText Ensure table cell is deselected when navigating from a table cell to the caption Styling tweaks Add an inline toolbar to the caption to match the image block implementation Adjust margin to match image block Add e2e test for the table block caption Refine caption accessibility. Use a div for the caption content Update caption to use a figcaption element with aria-labelledby Update table block deprecation so that it has its own attribute definition Update existing fixtures and add a new fixture for table captions Minor refinements Fix hasCaption logic Update snapshots Update comment with latest understanding of issue Use client id to generate non-clashing caption element id Update block-transforms fixture Update test to use a regular expression
801f6cf
to
f3ae751
Compare
@ellatrix Thanks for the follow-up review. There's a very minor fix in this PR that required the deprecation. I think it's best to move that over to another PR so that it can be reviewed separately. edit: #18861 is the fix that I've moved out of this PR. Turns out it didn't even require a deprecation 🤦♂ |
1f4f4af
to
6474a17
Compare
At some point there was consensus the best option was:
See #15554 (comment) Also, see #15554 (comment)
Maybe I'm missing something but seems to me there's no |
Hey Dan @talldan Could something similar be used for inline alignment of image captions? #12997 (comment) For at the moment the caption begins on the left bottom of the image. It would be great to have inline captions to choose where to have the caption. Left - center - right. Thanks! |
I want to go back to @afercia and their comment. Table captions should be using the native |
Related #15283, #6923
Fixes #11589
Description
How has this been tested?
Screenshots
Side-by-side of image and table captions in the editor
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: