Fix table header and body column misalignment #10011

talldan · 2018-09-18T17:03:08Z

Description

Fixes #9779 - table header misalignment issue

Changes:

The issue was caused by thead, tbody and tfoot elements being given the display: table property (SO explanation - https://stackoverflow.com/a/12153220). This styling has now been removed so that the elements have their default display property.
table-layout: fixed; was previously applied on the thead, tbody and tfoot elements, but since they're no longer display: table that was no longer working. This style is now applied to the table element and the table element has been changed from display: block to display: table
The above changes break the overflow-x: auto style that was applied to the table element, so a new wrapping div has been introduced to the editor and front-end rendered output of the block. Overflow is now applied to this div.
Table block deprecation has been introduced since the output of the block has been changed.

How has this been tested?

Column alignment

Paste a table with a thead directly into the editor (e.g. <table><thead><tr><td><strong>Column One</strong></td><td><strong>Column Two</strong></td></tr></thead><tbody><tr><td>one</td><td>two</td></tr><tr><td>three</td><td>four</td></tr></tbody></table>)
Observed in the editor that the table header columns are aligned with the body regardless of whether the 'Fixed width table cells' option is enabled.
Observed in the preview that everything is aligned.

Overflow

Enter long content without spaces or line breaks in a cell to make the table wider than the editor width.
Observed that the user can scroll horizontally in the editor to view all the content of the table
Observed that the user can scroll horizontally in the preview to view the entire table. (To test this, add_theme_support( 'wp-block-styles' ); needs to be added somewhere (e.g. in client-assets.php)).

Fixed width table cells

Toggle fixed width table cells
Ensured that table cells are equal spaced and that long content wraps in both the editor and preview

Deprecation

Create a table block on master branch and save post
Checkout this branch and load post in editor.
Ensured 'This block has been modified externally.' message is not present, and table displayed as expected

Screenshots

Before

After

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.

talldan · 2018-09-18T17:03:50Z

packages/block-library/src/table/style.scss

-	&.has-fixed-layout tbody,
-	&.has-fixed-layout tfoot {
-		display: table;
+	overflow-x: auto;


@jasmussen ah, I think this should be in theme.scss instead of style.scss, is that right?

Fixed-width layout has to work everywhere because it's a user-set option.

But the table doesn't have to be responsive out of the box, no, so okay to remove the width, min-width, and overflow rules and put those in theme.scss 👍 👍

Thanks for taking a look at this 😄

I've moved the overflow rule to theme.scss 👍

jasmussen · 2018-09-19T06:40:16Z

Very cool, thanks so much for working on this. Really really appreciate it.

It seems to be working:

I detected an issue with my own theme as part of this where I broke the files. But that was an issue in my theme.

Awesome work.

- `table` element needs to be `display: table` so that `table-layout: fixed` works correctly. - `thead`, `tbody` and `tfoot` need to use their default display properties, otherwise columns become misaligned between header, body and footer. (#9779) - Remove unecessary width on `thead`, `tbody`, and `tfoot` which is implicitly 100%

…siveness

noisysocks

Looking good! Will test this in a bit 🙂

noisysocks · 2018-09-25T11:01:52Z

packages/block-library/src/table/index.js

-				<Section type="body" rows={ body } />
-				<Section type="foot" rows={ foot } />
-			</table>
+			<div className={ classes }>


It's unfortunate that we have to introduce a wrapper <div>. Are there any alternatives? I wonder if Gutenberg could be less opinionated about this and leave table overflow styling up to the theme?

Sadly no other solutions I could find. Table elements are designed not to overflow and always grow to fit their contents.

@jasmussen what do you think about this suggestion of not worrying about overflowing tables?

I feel like by putting these styles in theme.scss we are already making themes opt in to this, and I also feel like we need a responsive play for any block that comes with Gutenberg. Even by making this opt in we're already watering down that aspect a bit.

If that requires a div to be compatible, I think that's okay, but as is always the case I'm okay with being overruled by @mtias or @karmatosed

Hey @jasmussen & @noisysocks - after a chat with Matias, I've decided to try an alternative where we wrap content instead of overflowing it. The advantage is that this doesn't involve a deprecation and is a much smaller change. I think it'll also be easier to undo if we decide change our minds in the future. There's a new PR here:
#10230

I'll close this one, but it's here in case we need to refer back to it.

noisysocks · 2018-09-25T11:05:24Z

packages/block-library/src/table/index.js

+
+			supports: {
+				align: true,
+			},


In other parts of the codebase we pull the attributes and supports object out into variables so that we can re-use them both in the block definition and in the deprecated object. Might be worth doing this here as a way of simplifying things.

deprecated: [ { attributes, supports, save( { attributes } ) { ... }, }, ],

👍

Given that, I'm guessing that deprecated blocks don't care about newly added attributes? Would they just ignore them? There aren't any in this PR, but I do have some in another PR.

Judging by the source code, that should be safe, so long as the new attribute doesn't cause the output of the old save function to change. (I can't think of an example that would.)

noisysocks · 2018-09-25T11:09:15Z

packages/block-library/src/table/style.scss

-	&.has-fixed-layout thead,
-	&.has-fixed-layout tbody,
-	&.has-fixed-layout tfoot {
-		display: table;


Letting the <table> be a display: table makes sense 🙂

noisysocks · 2018-09-25T13:13:18Z

Everything works great in my testing except for this step:

Observed that the user can scroll horizontally in the preview to view the entire table.

I'm using the default WordPress theme.

edit: I was missing a add_theme_support( 'wp-block-styles' ); 🙂

noisysocks

Works great in my testing!

talldan · 2018-09-25T14:08:22Z

Thanks for the review! Pushed commit f7a8fc3 to make those recommended changes. Will wait on an answer to the thread above before merging.

talldan added the [Type] Bug An existing feature does not function as intended label Sep 18, 2018

talldan added this to the 4.0 milestone Sep 18, 2018

talldan self-assigned this Sep 18, 2018

talldan requested review from jasmussen and ellatrix September 18, 2018 17:03

talldan commented Sep 18, 2018

View reviewed changes

talldan mentioned this pull request Sep 18, 2018

Add table dimension controls #9801

Closed

4 tasks

talldan added 6 commits September 19, 2018 11:11

Add a wrapping div around the rendered table to provide mobile respon…

2eb4696

…siveness

Move has-fixed-layout class to top level element for table block

b5802a4

Update integration tests

29b38e2

Add table block deprecation

b7e03ae

Move overflow rule to theme.scss

eab3703

talldan force-pushed the fix/table-misalignment branch from 3aad4db to eab3703 Compare September 19, 2018 10:23

talldan requested a review from a team September 20, 2018 11:49

noisysocks reviewed Sep 25, 2018

View reviewed changes

noisysocks approved these changes Sep 25, 2018

View reviewed changes

Extract blockAttributes and supports into variables and re-use

f7a8fc3

talldan mentioned this pull request Sep 28, 2018

Fix table header and body column misalignment (responsiveness using word-break: break-all) #10230

Merged

4 tasks

talldan closed this Sep 28, 2018

talldan deleted the fix/table-misalignment branch September 28, 2018 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix table header and body column misalignment #10011

Fix table header and body column misalignment #10011

talldan commented Sep 18, 2018 •

edited

Loading

talldan Sep 18, 2018

jasmussen Sep 19, 2018

talldan Sep 19, 2018

jasmussen commented Sep 19, 2018

noisysocks left a comment

noisysocks Sep 25, 2018

talldan Sep 25, 2018

jasmussen Sep 25, 2018

talldan Sep 28, 2018 •

edited

Loading

noisysocks Sep 25, 2018

talldan Sep 25, 2018

noisysocks Sep 25, 2018

noisysocks Sep 25, 2018

noisysocks commented Sep 25, 2018 •

edited

Loading

noisysocks left a comment

talldan commented Sep 25, 2018

Fix table header and body column misalignment #10011

Fix table header and body column misalignment #10011

Conversation

talldan commented Sep 18, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Sep 19, 2018

noisysocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks commented Sep 25, 2018 • edited Loading

noisysocks left a comment

Choose a reason for hiding this comment

talldan commented Sep 25, 2018

talldan commented Sep 18, 2018 •

edited

Loading

talldan Sep 28, 2018 •

edited

Loading

noisysocks commented Sep 25, 2018 •

edited

Loading