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

Ensure that a selection exists before attempting to execCommand #2143

Merged

Conversation

BoardJames
Copy link

@BoardJames BoardJames commented Aug 2, 2017

In normal TinyMCE the table toolbar is not shown unless a table is selected. For the table block we want to always show the toolbar even when the editor does not have a table selected so we have to check if a selection is present and supply a default selection when one is not available.

@codecov
Copy link

codecov bot commented Aug 2, 2017

Codecov Report

Merging #2143 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2143      +/-   ##
==========================================
- Coverage   24.23%   24.21%   -0.03%     
==========================================
  Files         142      142              
  Lines        4460     4465       +5     
  Branches      756      758       +2     
==========================================
  Hits         1081     1081              
- Misses       2854     2857       +3     
- Partials      525      527       +2
Impacted Files Coverage Δ
blocks/library/table/table-block.js 8% <0%> (-2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a59869...afa1302. Read the comment docs.

function isTableSelected( editor ) {
return editor.dom.getParent(
editor.selection.getStart( true ),
'table'
Copy link
Member

Choose a reason for hiding this comment

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

Do we know in this case that the table ancestor here is from this table block, and not another table on the page?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. DomUtils.getParent can take a root node as the third parameter so I'll set it to the editor container.

Copy link
Author

Choose a reason for hiding this comment

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

Turned out that it needed to be the parent of the editor body otherwise it would not consider the table. Otherwise I think this issue is fixed.

@nylen nylen force-pushed the fix/1751-ensure-table-selection-before-execCommand branch from 764d5ef to 9b48343 Compare August 4, 2017 15:04
@nylen
Copy link
Member

nylen commented Aug 4, 2017

Thanks for the fix. It works well for me.

I added the table back to the demo content (#1806).

This PR should close a few other issues; I've edited them into the original post. Let's wait until after today's release to merge it though, so we get some testing ahead of a release.

@nylen nylen added this to the Beta 0.8.0 milestone Aug 4, 2017
…github.com:WordPress/gutenberg into fix/1751-ensure-table-selection-before-execCommand
@BoardJames BoardJames merged commit 0275c43 into master Aug 7, 2017
@BoardJames BoardJames deleted the fix/1751-ensure-table-selection-before-execCommand branch August 7, 2017 00:30
@anna-harrison anna-harrison moved this from In Progress to Done in Ephox Team Aug 7, 2017
@anna-harrison anna-harrison moved this from Done to Logged in JIRA in Ephox Team Aug 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Ephox Team
Logged in JIRA
Development

Successfully merging this pull request may close these issues.

None yet

4 participants