Fix table block cell selection #16653
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
talldan
requested review from
aduth,
ajitbohra,
gziolo,
jorgefilipecosta,
nerrad,
ntwb,
Soean and
youknowriad
as code owners
talldan
force-pushed
the
fix/table-block-cell-selection
branch
from
33e5ad2 to
41f8824
Compare
ellatrix
reviewed
Jul 24, 2019
| @@ -377,15 +377,24 @@ export class TableEdit extends Component { | |||
| }; | |||
|
|
|||
| const cellClasses = classnames( { 'is-selected': isSelected } ); | |||
| const richTextClassName = 'wp-block-table__cell-content'; | |||
There was a problem hiding this comment.
It could be, it's an existing class name though, not something new, so I don't see a big reason to change it.
ellatrix
requested changes
Jul 24, 2019
| // 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(); |
There was a problem hiding this comment.
I removed this line, and the e2e test still passes.
There was a problem hiding this comment.
Thanks for spotting that, the selector used in the test was wrong, now fixed.
talldan
force-pushed
the
fix/table-block-cell-selection
branch
from
41f8824 to
0e6326d
Compare
|
This one should be good for re-review. |
ellatrix
reviewed
Jul 31, 2019
| await insertBlock( 'Table' ); | ||
|
|
||
| // Create the table. | ||
| const createButton = await page.$x( createButtonSelector ); |
There was a problem hiding this comment.
Also here, could the utility function clickButton be used?
ellatrix
reviewed
Jul 31, 2019
| // 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.` |
There was a problem hiding this comment.
You could also use line breaks here. Press Enter or type \n.
ellatrix
reviewed
Jul 31, 2019
|
|
||
| // 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.' ); |
There was a problem hiding this comment.
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. :)
ellatrix
approved these changes
Jul 31, 2019
talldan
force-pushed
the
fix/table-block-cell-selection
branch
from
0e6326d to
c451522
Compare
5 tasks
gziolo
pushed a commit
that referenced
this pull request
Aug 29, 2019
* 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
pushed a commit
that referenced
this pull request
Aug 29, 2019
* 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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: