Loading nested tables with border-bottom style throws JavaScript error

[jswiderski]

📝 Provide detailed reproduction steps (if any)

1. Start editor with the following content -

<table>
<tbody>
<tr>
<td> 

<table>
<tbody>
<tr>
<td style="border-bottom: 0 solid #fff;">

</td>
</tr>
</tbody>
</table>

</td>
</tr>
</tbody>
</table>

✔️ Expected result

Data gets loaded into editor.

❌ Actual result

Java Script error is being thrown -

TypeError: Cannot read property 'getAttribute' of undefined
    at setAttributeOnItem (writer.js:1487)
    at Writer.setAttribute (writer.js:380)
    at UpcastDispatcher.<anonymous> (tableproperties.js:73)
    at UpcastDispatcher.fire (emittermixin.js:209)
    at UpcastDispatcher._convertItem (upcastdispatcher.js:249)
    at UpcastDispatcher._convertChildren (upcastdispatcher.js:282)
    at UpcastDispatcher.<anonymous> (upcasthelpers.js:446)
    at UpcastDispatcher.fire (emittermixin.js:209)
    at UpcastDispatcher._convertItem (upcastdispatcher.js:249)
    at UpcastDispatcher._convertChildren (upcastdispatcher.js:282)

📃 Other details

Even after enabling nested tables #3232 (comment) the error is still thrown.

• Browser: Any
• OS: Any
• CKEditor version: 23.1.0
• Installed CKEditor plugins: N/A

---

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

[Reinmar]

FYI: The default schema config does not support nested tables at the moment, but it can be overridden: #3232 (comment)

[maxbarnas]

The problem happens when the schema allows for a given item (in this example a border-* style) and the upcasting is done via a custom converter. 

Pre-prepared converters (e.g. engine/conversion/upcasthelpers.attributeToAttribute()) are not prone to this because they iterate over items in range - if there are no items, there is nothing to iterate over. The custom converter table/converters/tableproperties.upcastBorderStyles() just passes the range and assumes there is something inside.

Adding a check around here for available items will solve the issue. The question is: do we want to do something more to prevent this type of problems? Can we?

[Reinmar]

Lets check if I got it right. That converter listens to element:viewElementName so I suppose to element:td and element:th. How's that possible that there's no model tableCell element in data.modelRange? Is it because that td/th was rejected upon element conversion? 

If yes, then it's definitely this converter that's incorrect. It cannot assume that the thing was indeed converted.

[Reinmar]

So, yeah, if modelRange does not hold a tableCell this converter (and potentially other similar converters in that feature) should abort.

[Reinmar]

Turns out that the problem was introduced when we were fixing #6177.

The fix for that issue is incorrect. The TC was as follows – try to load the following HTML

<table style="border:1px solid red">
  <tr>
    <td>parent:00</td>
    <td>
      <table style="border:1px solid green"><tr><td>child:00</td></tr></table>
    </td>
  </tr>
</table>

The error was thrown when trying to convert the border style on the inner table. In https://github.com/ckeditor/ckeditor5/blob/d1b79f1/packages/ckeditor5-table/src/converters/tableproperties.js#L40 we listen to element:table and prior to adding https://github.com/ckeditor/ckeditor5/blob/d1b79f1/packages/ckeditor5-table/src/converters/tableproperties.js#L63-L67, modelRange was null because the converter for the element:table event that should create the model <table> element aborts due to schema (nested tables are not allowed). This is completely fine, and we should also abort trying to convert the border style.

However, instead, we added these couple of lines which do convertChildren() forcing conversion of the <tr>, <td> and text nodes inside. <tr> and <td> are rejected (nested tables are not allowed), but text nodes get converted into paragraph or text nodes. And we end up with this:

Yup, table styles get applied to the content of a table cell.

Tests written like this pass: ckeditor/ckeditor5-table@a754898#diff-7d05be5c29068846363432b7459306703d51e40f853e3fc4191d38c3b6acb5a4R227-R229 and we release this patch :D

The solution here will be to revert these changes and simply abort conversion when modelRange is null because it's null only when content was rejected.