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

Fix table block cell selection #16653

Merged
merged 3 commits into from Jul 31, 2019

Conversation

@talldan
Copy link
Contributor

commented Jul 18, 2019

Description

Fixes #14904

Fixes an issue where table block cells are difficult to select in some situations.

This issue particularly occurs when a cell is alongside another that has a lot of text content, and also particularly noticable when Fixed Width Cells is enabled.

How has this been tested?

Added an e2e test

Manual testing

  1. Create a table block with 1 row, 2 columns
  2. Toggle on Fixed Width Cells
  3. Type a lot of text into the first cell so that it wraps onto multiple lines
  4. Try clicking at the very top or bottom of the second cell

Expected Result (in this branch)
The cell is selected

Actual Result (in current master)
The cell is not selected

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.

@talldan talldan self-assigned this Jul 18, 2019

@talldan talldan force-pushed the fix/table-block-cell-selection branch from 33e5ad2 to 41f8824 Jul 18, 2019

@talldan talldan added this to In Progress in Table block Jul 19, 2019

@talldan talldan moved this from In Progress to In in Table block Jul 19, 2019

@@ -377,15 +377,24 @@ export class TableEdit extends Component {
};

const cellClasses = classnames( { 'is-selected': isSelected } );
const richTextClassName = 'wp-block-table__cell-content';

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 24, 2019

Member

Could this just be rich-text?

This comment has been minimized.

Copy link
@talldan

talldan Jul 30, 2019

Author Contributor

It could be, it's an existing class name though, not something new, so I don't see a big reason to change it.

// user may click inside a cell, but outside of the RichText, resulting in nothing happening.
const richTextElement = event && event.target && event.target.querySelector( `.${ richTextClassName }` );
if ( richTextElement ) {
richTextElement.focus();

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 24, 2019

Member

I removed this line, and the e2e test still passes.

This comment has been minimized.

Copy link
@talldan

talldan Jul 30, 2019

Author Contributor

Thanks for spotting that, the selector used in the test was wrong, now fixed.

@talldan talldan force-pushed the fix/table-block-cell-selection branch from 41f8824 to 0e6326d Jul 30, 2019

@talldan talldan requested a review from ellatrix Jul 31, 2019

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

This one should be good for re-review.

await insertBlock( 'Table' );

// Create the table.
const createButton = await page.$x( createButtonSelector );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

Also here, could the utility function clickButton be used?

// Add lots of text to the first cell.
await page.click( '.wp-block-table__cell-content' );
await page.keyboard.type(
`Some long text that will wrap onto multiple lines.`

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

You could also use line breaks here. Press Enter or type \n.


// Click in the top left corner of the second cell and type some text.
await page.mouse.click( secondCellX, secondCellY );
await page.keyboard.type( 'This content is in the second cell.' );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

I usually try to avoid long strings of text where one single character may be enough to test. Not sure if that actually makes much difference though. :)

@talldan talldan force-pushed the fix/table-block-cell-selection branch from 0e6326d to c451522 Jul 31, 2019

@talldan talldan merged commit 3d65eda into master Jul 31, 2019

1 of 4 checks passed

Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

Table block automation moved this from In Progress to Done Jul 31, 2019

@talldan talldan deleted the fix/table-block-cell-selection branch Jul 31, 2019

@talldan talldan added this to the Gutenberg 6.3 milestone Aug 1, 2019

@talldan talldan referenced this pull request Aug 2, 2019
5 of 5 tasks complete
gziolo added a commit that referenced this pull request Aug 29, 2019
Fix table block cell selection (#16653)
* Forward focus to child RichText when a table cell is selected

* Add e2e tests

* Update e2e test to ensure cell is clicked instead of the rich text
gziolo added a commit that referenced this pull request Aug 29, 2019
Fix table block cell selection (#16653)
* Forward focus to child RichText when a table cell is selected

* Add e2e tests

* Update e2e test to ensure cell is clicked instead of the rich text
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.