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.

Sign up for GitHub

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

Jump to bottom

Add rudimentary support for table block cell scope attributes #16154

Merged
merged 6 commits into from
Jul 4, 2019
Merged

Add rudimentary support for table block cell scope attributes #16154

merged 6 commits into from
Jul 4, 2019

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jun 13, 2019 •
edited
Loading

Description

Related #15283

Currently, if a user tries to paste table html that includes the scope attribute on table cells, this content is stripped out by Gutenberg.

As mentioned in #15283, the scope attributes are import for users of accessibility software as they help inform the context of a table cell.

While this PR doesn't expose any UI capability for editing this attribute (apart from the HTML editor), it at least ensures gutenberg doesn't worsen the accessibility of tables pasted into it or edited manually.

How has this been tested?

Updated full-content tests to include this attribute.

Manual testing:
Pasting

  1. Paste a simple html table into the editor
<table><thead><tr><th scope="col">test</th></tr></thead><tbody><tr><td>test</td></tr></tbody></table>
  1. Inspect the resultant table block in the editor using the dev tools
  2. Observe that the th element correctly retains the scope attribute
  3. Preview the post
  4. Inspect the resultant table in the post using the dev tools
  5. Observe that the th element correctly retains the scope attribute

Editing HTML

  1. Create a table block in the editor
  2. Toggle on the heading row in the sidebar
  3. Edit the block as HTML and add a scope="col" attribute to the first th element
  4. Switch back to editing visually
  5. Inspect the resultant table block in the editor using the dev tools
  6. Observe that the th element correctly retains the scope attribute
  7. Preview the post
  8. Inspect the resultant table in the post using the dev tools
  9. Observe that the th element correctly retains the scope attribute

Screenshots

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 talldan added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Block] Table Affects the Table Block Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Jun 13, 2019
@talldan talldan self-assigned this Jun 13, 2019
@talldan talldan marked this pull request as ready for review June 14, 2019 08:39
@talldan talldan mentioned this pull request Jun 14, 2019
Closed
@talldan talldan force-pushed the add/scope-attribute-support-for-table-block branch from a935166 to 2805d1e Compare June 27, 2019 14:52
jorgefilipecosta
jorgefilipecosta reviewed Jun 27, 2019
View reviewed changes
packages/block-library/src/table/transforms.js
@@ -10,6 +10,7 @@ const tableContentPasteSchema = {
th: {
allowEmpty: true,
children: getPhrasingContentSchema(),
attributes: [ 'scope' ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://www.w3schools.com/tags/att_td_scope.asp it seems scope attribute can also be used on td elements. Should we also add attributes: [ 'scope' ], to td? Currently when we paste a table with a scope on td elements the scope gets removed if we manually add a scope in the code editor it is persisted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @jorgefilipecosta. It does say on that page The <td> scope attribute is not supported in HTML5. I think it might be better not to encourage future use of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I should make sure the attribute can't be output on tds in both the save and edit functions as well 🤔.

I'll add another commit, should be easy to do.

packages/e2e-tests/fixtures/blocks/core__table.html Outdated
<table class="wp-block-table"><thead><tr><th>Version</th><th>Musician</th><th>Date</th></tr></thead><tbody><tr><td><a href="https://wordpress.org/news/2003/05/wordpress-now-available/">.70</a></td><td>No musician chosen.</td><td>May 27, 2003</td></tr><tr><td><a href="https://wordpress.org/news/2004/01/wordpress-10/">1.0</a></td><td>Miles Davis</td><td>January 3, 2004</td></tr><tr><td>Lots of versions skipped, see <a href="https://codex.wordpress.org/WordPress_Versions">the full list</a></td><td>&hellip;</td><td>&hellip;</td></tr><tr><td><a href="https://wordpress.org/news/2015/12/clifford/">4.4</a></td><td>Clifford Brown</td><td>December 8, 2015</td></tr><tr><td><a href="https://wordpress.org/news/2016/04/coleman/">4.5</a></td><td>Coleman Hawkins</td><td>April 12, 2016</td></tr><tr><td><a href="https://wordpress.org/news/2016/08/pepper/">4.6</a></td><td>Pepper Adams</td><td>August 16, 2016</td></tr><tr><td><a href="https://wordpress.org/news/2016/12/vaughan/">4.7</a></td><td>Sarah Vaughan</td><td>December 6, 2016</td></tr></tbody></table>
<!-- /wp:core/table -->
<!-- wp:table -->
<table class="wp-block-table"><thead><tr><th scope="col">Version</th><th scope="col">Musician</th><th scope="col">Date</th></tr></thead><tbody><tr><td><a href="https://wordpress.org/news/2003/05/wordpress-now-available/">.70</a></td><td>No musician chosen.</td><td>May 27, 2003</td></tr><tr><td><a href="https://wordpress.org/news/2004/01/wordpress-10/">1.0</a></td><td>Miles Davis</td><td>January 3, 2004</td></tr><tr><td>Lots of versions skipped, see <a href="https://codex.wordpress.org/WordPress_Versions">the full list</a></td><td>…</td><td>…</td></tr><tr><td><a href="https://wordpress.org/news/2015/12/clifford/">4.4</a></td><td>Clifford Brown</td><td>December 8, 2015</td></tr><tr><td><a href="https://wordpress.org/news/2016/04/coleman/">4.5</a></td><td>Coleman Hawkins</td><td>April 12, 2016</td></tr><tr><td><a href="https://wordpress.org/news/2016/08/pepper/">4.6</a></td><td>Pepper Adams</td><td>August 16, 2016</td></tr><tr><td><a href="https://wordpress.org/news/2016/12/vaughan/">4.7</a></td><td>Sarah Vaughan</td><td>December 6, 2016</td></tr></tbody></table>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, the scope is not added, I feel by default the test fixture should also not contain scope attributes. What if we add a dedicated fixture to test table scopes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I've moved the test into its own fixture.

@talldan
Copy link
Contributor Author

talldan commented Jul 1, 2019

Managed to get the tests to pass (realised I had to clear both the master and PR cache in travis), so hopefully this one is good to go.

ellatrix
ellatrix approved these changes Jul 4, 2019
View reviewed changes
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

@ellatrix ellatrix merged commit 367480b into master Jul 4, 2019
@ellatrix ellatrix deleted the add/scope-attribute-support-for-table-block branch July 4, 2019 22:00
@youknowriad youknowriad added this to the Gutenberg 6.1 milestone Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgefilipecosta jorgefilipecosta jorgefilipecosta left review comments

@ellatrix ellatrix ellatrix approved these changes

@aduth aduth Awaiting requested review from aduth

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@gziolo gziolo Awaiting requested review from gziolo

@nerrad nerrad Awaiting requested review from nerrad

@notnownikki notnownikki Awaiting requested review from notnownikki

@ntwb ntwb Awaiting requested review from ntwb

@Soean Soean Awaiting requested review from Soean

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@talldan talldan

Labels
[Block] Table Affects the Table Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 6.1
Development

Successfully merging this pull request may close these issues.
4 participants
@talldan @ellatrix @jorgefilipecosta @youknowriad