Skip to content
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

Merged
merged 11 commits into from Dec 2, 2019
Merged

Add table block caption #15554

merged 11 commits into from Dec 2, 2019

Conversation

@talldan
Copy link
Contributor

talldan commented May 10, 2019

Related #15283, #6923

Fixes #11589

Description

  • Adds the ability to specify a caption for the table block.

How has this been tested?

  1. Add a table
  2. Add a caption to the table
  3. Preview the post and check that the table has a caption

Screenshots

Side-by-side of image and table captions in the editor
Screen Shot 2019-05-10 at 4 00 23 pm

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented May 10, 2019

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 <RichText> directly within the table to represent the caption, as that component outputs a <div> as its root element.

Instead I've added a normal caption tag to the table and made it visibility: hidden (not sure if making it zero height and overflow hidden would be preferable?). Then I've added a RichText outside the table for editing the caption.

Is there a better way to do this?

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented May 15, 2019

Thinking that adding a rootTagName prop to RichText might be a better solution so that I don't have to add a hack in this PR.

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented May 16, 2019

Well ... I don't know why I didn't think of this before, but the obvious solution is to put the RichText component in the caption element. Not sure why I didn't think of this before.🤦‍♂

However, I tested it out, and that introduces other problems. The HTML spec mentions to put the caption before the tbody in a table, but if the caption is first in the DOM the tab order is wrong for users of the table block.

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.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented May 17, 2019

@talldan I think it could be appropriate here to use the figure element? In general, I'd see this as being implemented almost identical to the image block (though I'm not really a fan of its save implementation with always-figure and div wrappers).

<figure>
    <table><!-- ... --></table>
    <figcaption>My lovely caption</figcaption>
</figure>

See also: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/table#attr-summary

Include the <table> element in a <figure> element and add the description in prose next to it.

Edit: Aha! I didn't realize this was the original implementation. It seems to me like it would be a reasonable approach.

@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented May 18, 2019

Re: the div wrappers problem within the editor:
I'm not sure if it's technically possible, but I'd vote for a contenteditable <caption> element. Is it possible to use that instead of a RichText component?

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 <caption> element.

Hiding the <caption> element seems a bit hacky to me, as it wouldn't be perceived by assistive technologies and as a consequence the table wouldn't have an associated title.

If it's not possible to use a <caption> within the editor, I guess we should just accept the table won't have a 100% correct semantic in the editor.

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".

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented May 23, 2019

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.

@talldan talldan force-pushed the add/table-block-caption branch from 1c315a4 to 95236f4 Jun 10, 2019
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Jun 10, 2019

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)

<table>
    <caption><RichText ... /></caption>
     // ...
</table>

For:

  • Correct table markup

Against:

  • I haven't figured out a way to make the tab order work when using caption-side: bottom;, so this option might be limited to only displaying the caption above the table.
  • VoiceOver didn't seem to know what to do with a contenteditable inside a table caption. It doesn't seem to announce it when editing. That would seem to rule this option out.

Option 2 (RichText outside the table, visibly hidden caption)

<table>
    <caption className="screen-reader-text">{ caption }</caption>
     // ...
</table>
<RichText ... />

Pros:

  • Correct table markup.
  • Could support both caption-side: top; and caption-side: bottom;.

Cons:

  • The user has to leave the context of the table to edit the caption. Though they're at least still within the context of the table block.
  • After altering the content of the RichText, the updated table caption wasn't read by VoiceOver in Chrome (worked fine in Safari). Forcing the element to be replaced in the DOM instead of updated resolved this, but it's a bit of a hack!

Option 3 (figure/figcaption)

<figure>
    <table>
         // ...
    </table>
     <figcaption><RichText /></figcaption>
<figure>

Pros:

  • Could support both caption-side: top; and caption-side: bottom;.

Cons:

  • While this is considered ok, it's not quite how table captions are supposed to be marked up.
  • The caption isn't read by voiceover when interacting with the table.
  • The user has to leave the context of the table to edit the caption. Though they're at least still within the context of the table block.
@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Jun 10, 2019

@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 <caption> element. Some of them focus the editable but don't announce anything. Some don't even focus the editable. Basically only Safari + VoiceOver get it right.

Codepen for testing: https://codepen.io/afercia/full/RzbMvx

The same happens with a native <input> field so this appears to be some unfortunate browsers limitation.

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 aria-label="Table caption" to clarify what it is. The important thing is that in the front end it's rendered as a real <caption> element.

@talldan talldan force-pushed the add/table-block-caption branch from 95236f4 to e27beab Jun 11, 2019
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Jun 11, 2019

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.

@talldan talldan requested review from etoledom and SergioEstevao as code owners Oct 18, 2019
@talldan talldan force-pushed the add/table-block-caption branch from 2d67988 to c99d081 Oct 18, 2019
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Oct 18, 2019

@ellatrix I've brought in the latest changes, and it seems to work. I need to update some tests, but it should be good to re-review soon.

The bad news is the Voiceover issues remain. The main problem is that Voiceover reads "Write caption...", the aria-label for the figcaption instead of the text content ... Not sure why. This is also happening on the image block.

I've decided to remove all the experimentation I did to try to get Voiceover working, and defer that to a later PR.

@talldan talldan force-pushed the add/table-block-caption branch from f43f3cf to 801f6cf Oct 21, 2019
@patrikhuber

This comment has been minimized.

Copy link

patrikhuber commented Nov 1, 2019

Hi! This is great. Any estimates on when we can see this in official WordPress?
Thanks!

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Nov 6, 2019

@talldan Looks good. There's just a failing test left?

Copy link
Member

ellatrix left a comment

Looks good. This should include a fixture for the deprecation(s) though. Why is there a deprecation is the first place?

talldan added 8 commits May 9, 2019
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
@talldan talldan force-pushed the add/table-block-caption branch from 801f6cf to f3ae751 Dec 2, 2019
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Dec 2, 2019

@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 🤦‍♂

@talldan talldan force-pushed the add/table-block-caption branch from 1f4f4af to 6474a17 Dec 2, 2019
@talldan talldan merged commit 4a4bd4a into master Dec 2, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Table block automation moved this from In Progress to Done Dec 2, 2019
@talldan talldan deleted the add/table-block-caption branch Dec 2, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.