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

Make tables accessible at high zoom levels #16324

Merged
merged 13 commits into from Jul 9, 2019

Conversation

tellthemachines
Copy link
Contributor

@tellthemachines tellthemachines commented Jun 27, 2019

Description

Partially fixes #15346

In order for wide tables to be scrollable at small breakpoints and/or high zoom levels, I have added a <div> wrapper to the table block markup, setting its overflow to auto.

How has this been tested?

Tested at high zoom levels on Chrome, Firefox, Safari (Mac) and IE (Windows 7 and 10).

Screenshots

Screen Shot 2019-06-27 at 5 25 15 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

@tellthemachines tellthemachines added [a11y] Zooming [Type] Bug Accessibility (a11y) Needs Accessibility Feedback Needs Design Feedback labels Jun 27, 2019
@talldan
Copy link
Contributor

talldan commented Jun 27, 2019

I know similar ideas to this have been previously explored (cc @jasmussen @mtias) without much success, and we'd really wanted to avoid the wrapper, but it does seem like it might be the only option.

Just summarising some of the positives and negatives of the wrapping div:

Pros:

  • Solves accessibility/usability issues at a higher zoom level.
  • Reduces text wrapping.
  • Allows users to create bigger, more complex tables.

Cons:

  • The markup produced by the block's save function isn't as clean.
  • The change in markup might affect themes?
  • Encourages use of big tables.
  • The scroll area obscures table content.

Looking at some other products:

  • Google Docs: Wraps text avoiding a scrollable area (but that makes sense for paper documents)
  • Dropbox Paper: Uses a scrollable area like this PR

// Fixed layout toggle
&.has-fixed-layout {
.has-fixed-layout {
Copy link
Contributor

@talldan talldan Jun 27, 2019

Choose a reason for hiding this comment

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

Seeing some inconsistencies between the editor and the post when it comes to the fixed layout. The editor seems to wrap long text, but when viewing the post text isn't wrapped.

Copy link
Contributor Author

@tellthemachines tellthemachines Jul 1, 2019

Choose a reason for hiding this comment

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

Fixed 😅

align: true,
};

const deprecated = [
Copy link
Contributor

@talldan talldan Jun 27, 2019

Choose a reason for hiding this comment

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

Would be great to add a test fixture for this deprecation. There are some examples of them in the folder with the fixtures (some have __deprecated in the filename):
https://github.com/WordPress/gutenberg/tree/master/packages/e2e-tests/fixtures/blocks

It should just be a matter of grabbing the block HTML from master (including the comment delimeters) and pasting it into a new html file, then regenerating the fixtures. If the deprecation works it'll output a valid block.

Copy link
Contributor Author

@tellthemachines tellthemachines Jul 1, 2019

Choose a reason for hiding this comment

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

Done, thanks for the reminder!

@tellthemachines tellthemachines force-pushed the fix/Content-reflow-on-page-zoom branch from b28525d to d909d6a Compare Jul 1, 2019
@tellthemachines
Copy link
Contributor Author

tellthemachines commented Jul 1, 2019

Re some of @talldan 's pros and cons:

  • The change in markup might affect themes?
  • Encourages use of big tables.
  • The scroll area obscures table content.
  1. I checked a few themes (arguably a next to useless endeavour given the amount out there...) and didn't find any problems. But it'll only take someone going table.wp-block-table for this to go bang. Is there any procedure around changes that can potentially affect themes?
  2. But big tables are good! Haha I'm not sure we should be prescriptive about user content though 🤔
  3. The scrollbar should be a clear enough indicator that there's more content to be seen by scrolling. Or am I missing something?

Overall, I'm still for making this change as in my opinion the usability benefits outweigh the costs. But look forward to hearing further discussion points 🙂

@tellthemachines tellthemachines requested a review from talldan Jul 2, 2019
<Section type="body" rows={ body } />
<Section type="foot" rows={ foot } />
</table>
<div className={ className }>
Copy link
Member

@ellatrix ellatrix Jul 4, 2019

Choose a reason for hiding this comment

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

How about changing this to <figure>, so the markup is clean, we can allow a caption, and it's the same as all our other media/figure blocks? Cc @talldan.

Copy link
Contributor Author

@tellthemachines tellthemachines Jul 5, 2019

Choose a reason for hiding this comment

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

Done!

@ellatrix
Copy link
Member

ellatrix commented Jul 4, 2019

Hi @tellthemachines!

I don't really mind the changes here, it makes sense to support big tables and it looks good after a quick test. If we need a wrapping element, I think it should be figure, as we'd add that regardless at some point.

@tellthemachines tellthemachines requested a review from ellatrix Jul 5, 2019
*/
import { RichText, getColorClassName } from '@wordpress/block-editor';

const blockAttributes = {
Copy link
Member

@ellatrix ellatrix Jul 5, 2019

Choose a reason for hiding this comment

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

We're only adding a wrapper element right? Can't we share this attribute definition between files?

Copy link
Contributor Author

@tellthemachines tellthemachines Jul 9, 2019

Choose a reason for hiding this comment

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

Yes, thanks for pointing that out. Done!


td,
th {
word-break: break-all;
Copy link
Member

@ellatrix ellatrix Jul 5, 2019

Choose a reason for hiding this comment

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

Why is this needed? Does it deserve a comment?

Copy link
Contributor Author

@tellthemachines tellthemachines Jul 9, 2019

Choose a reason for hiding this comment

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

Assuming we don't want tables to horizontally scroll when they are aligned, we need table content to break. Added comment.

Copy link
Member

@ellatrix ellatrix left a comment

Looks good. Feedback on above comments would be nice. Looks like e2e and unit tests also need to be updated.

@tellthemachines tellthemachines force-pushed the fix/Content-reflow-on-page-zoom branch from e09783f to f8d31dc Compare Jul 8, 2019
@tellthemachines tellthemachines merged commit 105cb47 into master Jul 9, 2019
2 checks passed
@tellthemachines tellthemachines deleted the fix/Content-reflow-on-page-zoom branch Jul 9, 2019
@afercia
Copy link
Contributor

afercia commented Jul 9, 2019

@tellthemachines thanks for working on this, works great! 🙂

A while ago, we've implemented a similar solution at Yoast, with some additional visual improvements. I'm not saying those are necessarily good for Gutenberg, but I'd like to hear your thoughts and have some design feedback /Cc @mapk. Conversation can continue also on closed issues/PRs 🙂 and if the proposed improvements get some consensus I'd like to open a follow-up issue.

Basically, in our implementation we're trying to solve a few issues:

  • most operating systems don't display scrollbars by default: users may not understand the table is scrollable so we added a visual hint (hidden from assistive technologies with aria-hidden="true")
  • added a box-shadow on the right as additional visual hint
  • added some bottom padding to the table wrapper so that the horizontal scrollbar doesn't hide the table content

All these three things kick in only when there's not enough space to display the table entirely so there's some width/viewport JS calculation running behind the scenes.

Screenshots:

The table while scrolling horizontally and the scrollbar becomes visible:

Screenshot 2019-07-09 at 14 25 04

When there's enough space, all the visual indicators are not displayed:

Screenshot 2019-07-09 at 14 26 02

Any thoughts more than welcome!

@@ -17,25 +28,30 @@
// The table element needs to be kept as display table
// for table features to work reliably.
display: table;

Copy link
Contributor

@afercia afercia Jul 9, 2019

Choose a reason for hiding this comment

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

❤️

@talldan
Copy link
Contributor

talldan commented Jul 19, 2019

@afercia Just doing some table block triage, and there is an issue covering your suggestion:
#8401

@afercia
Copy link
Contributor

afercia commented Jul 19, 2019

@talldan thanks, will comment there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[a11y] Zooming Accessibility (a11y) [Block] Table Needs Accessibility Feedback Needs Design Feedback [Type] Bug
Projects
No open projects
Table block
  
Done
Development

Successfully merging this pull request may close these issues.

5 participants