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): Support cells without children #8942

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Conversation

mat128
Copy link
Contributor

@mat128 mat128 commented Jan 27, 2021

Previously, it was possible for table cells to be entirely empty.
However, a recent change introduced in 93dbbf9 changed the evaluation order for table cell children.

This led to calling hasAnyAttribute on a null reference, which is not supported.

Move isSingleParagraph check prior to hasAnyAttribute. isSingleParagraph is correctly calculated because it relies on the childCount for the table cell.

Fixes #8941
Fixes #8917

Previously, it was possible for table cells to be entirely empty.
However, a recent change introduced in 93dbbf9 changed the evaluation order for table cell children.

This led to calling hasAnyAttribute on a null reference, which is not supported.

Move isSingleParagraph check prior to hasAnyAttribute. isSingleParagraph is correctly calculated because it relies on the childCount for the table cell.

Fixes ckeditor#8941
Fixes ckeditor#8917
@testinfected
Copy link

+1

@bsmilling
Copy link

+1, would be nice if this was included in the next version.

@oleq
Copy link
Member

oleq commented Feb 1, 2021

Hi, @mat128

Thank you for the contribution, your PR totally makes sense. There's one thing missing, though: a unit test for the change. Could you provide it for us to avoid future regressions? Thanks.

@oleq oleq self-requested a review February 1, 2021 12:44
@wenneberg6
Copy link

+1 this bug halted us from upgrading to v. 25

@mat128
Copy link
Contributor Author

mat128 commented Feb 2, 2021

@oleq Thank you for your feedback. I tried writing a unit test but could never trigger the problem.

It appears empty cells have a   added when invoking utility functions for data/model conversion.

it( 'should support empty cells', () => {
	setModelData( model, modelTable( [ [ '' ] ] ) );
	assertEqualMarkup( editor.getData(),
		'<figure class="table">' +
			'<table>' +
				'<tbody>' +
					'<tr><td>&nbsp;</td></tr>' +
				'</tbody>' +
			'</table>' +
		'</figure>'
	);
} );

I also tried setting model data directly, to no avail:

setModelData(model, '<table><tableRow><tableCell></tableCell></tableRow></table>');

Could point me on the right track?

Thank you

@dkrahn
Copy link
Contributor

dkrahn commented Feb 4, 2021

Probably also fixes #8979

@oleq
Copy link
Member

oleq commented Feb 8, 2021

@mat128 Don't worry, we took it over and finished this task in #9004. Thanks again!

@mat128
Copy link
Contributor Author

mat128 commented Feb 8, 2021

Thanks for the quick turnaround! Good job =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants