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

Editor crashes with a downcasting error when input contain an empty <tr></tr> #3274

Closed
hponcet opened this issue Jun 28, 2019 · 4 comments · Fixed by #6762
Closed

Editor crashes with a downcasting error when input contain an empty <tr></tr> #3274

hponcet opened this issue Jun 28, 2019 · 4 comments · Fixed by #6762
Assignees
Labels
package:table support:3 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@hponcet
Copy link

hponcet commented Jun 28, 2019

This was fixed if i hard replace the source html by replace(/<tr[^<]*><\/tr>/gm, '<tr><th></th></tr>')

@Mgsy
Copy link
Member

Mgsy commented Jun 28, 2019

Hello, can you provide step-by-step instruction on how to reproduce the issue in the editor?

@jodator
Copy link
Contributor

jodator commented Jun 28, 2019

Hmm this will break the editor, ie in the article manual test:

<table><tr></tr><tr></tr></table>

Error:

article.js:44 TypeError: Cannot read property 'name' of undefined
    at Mapper.getModelLength (mapper.js:422)
    at Mapper._findPositionIn (mapper.js:487)
    at Mapper.on.priority (mapper.js:88)
    at Mapper.fire (emittermixin.js:207)
    at Mapper.toViewPosition (mapper.js:271)
    at DowncastDispatcher.modelViewSplitOnInsert (converters.js:198)
    at DowncastDispatcher.fire (emittermixin.js:207)
    at DowncastDispatcher._testAndFire (downcastdispatcher.js:466)
    at DowncastDispatcher.convertInsert (downcastdispatcher.js:177)
    at DowncastDispatcher.convertChanges (downcastdispatcher.js:136)

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-table Oct 9, 2019
@mlewand mlewand added this to the next milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:table labels Oct 9, 2019
@mlewand
Copy link
Contributor

mlewand commented Apr 6, 2020

The thing is that ideally we should not end up with model that has content like that - this is clearly pointless table markup.

We did put extra effort in iteration 30 to make sure no empty rows are created by our core plugins (ckeditor/ckeditor5-table#277, ckeditor/ckeditor5-table#278). But in the end we decided that we need to fix the source of the issue, rather to make all our algorithms work with flawed table structure.

I mentioned that there will be issues like this popping out with time, and we can see some examples:

  • Table loses geometry after horizontal split and removing column #6439
  • This very issue, although there's unfortunately no repro-case the easiest one I can see is simply pasting content from external sources - we will never be able to guarantee that the content doesn't contain empty rows (think of copying part of other page, pasting from Word, other WYSIWYG editors).

That being said I believe we should change our approach and gracefully handle case with empty row.

@jodator
Copy link
Contributor

jodator commented Apr 7, 2020

I mentioned that there will be issues like this popping out with time, and we can see some examples:

The issues you mentioned comes from, unfortunately, wrong table handling in our commands. The fixes are on the way: #6545 & #6546

However we should make our conversion process resilient and anticipate wrong input data.

@Mgsy Mgsy added the support:1 An issue reported by a commercially licensed client. label Apr 23, 2020
@jodator jodator modified the milestones: next, iteration 32 Apr 30, 2020
@niegowski niegowski self-assigned this May 7, 2020
jodator added a commit that referenced this issue May 12, 2020
Fix (table): Empty table rows are properly handled during conversion and layout post-fixing. Closes #3274.
@lslowikowska lslowikowska added support:3 An issue reported by a commercially licensed client. and removed support:1 An issue reported by a commercially licensed client. labels Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table support:3 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
7 participants