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

Table properties crashes in some pasting scenarios #6177

Closed
mlewand opened this issue Feb 3, 2020 · 6 comments · Fixed by ckeditor/ckeditor5-table#274
Closed

Table properties crashes in some pasting scenarios #6177

mlewand opened this issue Feb 3, 2020 · 6 comments · Fixed by ckeditor/ckeditor5-table#274
Assignees
Labels
package:table type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@mlewand
Copy link
Contributor

mlewand commented Feb 3, 2020

📝 Provide detailed reproduction steps (if any)

  1. Open table properties manual test.
  2. Open a CKEditor Wikipedia page in another tab.
  3. Select the entire article (can be full page select, or just the inner part of article, doesn't matter).
  4. Copy it.
  5. Paste it into the editor.

✔️ Expected result

Content is pasted.

❌ Actual result

Exception is being thrown (Chrome):

tableproperties.js:63 Uncaught TypeError: Cannot read property 'getItems' of null
    at UpcastDispatcher.<anonymous> (tableproperties.js:63)
    at UpcastDispatcher.fire (emittermixin.js:209)
    at UpcastDispatcher._convertItem (upcastdispatcher.js:239)
    at UpcastDispatcher._convertChildren (upcastdispatcher.js:270)
    at UpcastDispatcher.<anonymous> (upcasthelpers.js:590)
    at UpcastDispatcher.fire (emittermixin.js:209)
    at UpcastDispatcher._convertItem (upcastdispatcher.js:239)
    at UpcastDispatcher._convertChildren (upcastdispatcher.js:270)
    at UpcastDispatcher.<anonymous> (upcasttable.js:117)
    at UpcastDispatcher.fire (emittermixin.js:209)

Exception for Firefox:

TypeError: data.modelRange is null

📃 Other details

It works fine with the table plugin alone, when no table properties are enabled.

  • Browser: Any
  • OS: Windows 10 (probably any?)
  • CKEditor version: Latest master

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

@mlewand mlewand added type:bug This issue reports a buggy (incorrect) behavior. package:table type:regression This issue reports a bug that was not present in the previous releases. labels Feb 3, 2020
@Mgsy
Copy link
Member

Mgsy commented Feb 3, 2020

I can reproduce it.

@Reinmar
Copy link
Member

Reinmar commented Feb 3, 2020

Is it a regression? Is it reproducible on a normal table demo (without table styles)?

@mlewand
Copy link
Contributor Author

mlewand commented Feb 3, 2020

@Reinmar it's not reproducible when table properties plugin is not enabled.

It works fine with the table plugin alone, when no table properties are enabled.

@Reinmar Reinmar added this to the iteration 30 milestone Feb 18, 2020
@mlewand mlewand changed the title Table properties crashes some pasting scenarios Table properties crashes in some pasting scenarios Feb 24, 2020
@Reinmar Reinmar self-assigned this Mar 10, 2020
@oleq
Copy link
Member

oleq commented Mar 11, 2020

The problem comes down to nested tables. The minimal HTML to paste is:

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

The converter for table border style fails because when data.viewItem is the child table, data.modelRange is null.

I see 2 solutions since we don't support nested tables:

  1. Return immediately if !data.modelRange.
  2. Copy-paste from upcasthelpers.js
diff --git a/src/converters/tableproperties.js b/src/converters/tableproperties.js
index 34a9719..ab2b6c3 100644
--- a/src/converters/tableproperties.js
+++ b/src/converters/tableproperties.js
@@ -60,6 +60,10 @@ export function upcastBorderStyles( conversion, viewElementName ) {
                            return;
                   }

+                  if ( !data.modelRange ) {
+                           data = Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) );
+                  }
+
                   const modelElement = [ ...data.modelRange.getItems( { shallow: true } ) ].pop();

                   conversionApi.consumable.consume( data.viewItem, matcherPattern );

I'd rather go with the later. Do you have any idea what's the right solution, @Reinmar?

@Reinmar
Copy link
Member

Reinmar commented Mar 11, 2020

I'd rather go with the later. Do you have any idea what's the right solution, @Reinmar?

I can never remember why modelRange can be null. It's not the first time we have this issue and not the first time we have to copy that fragment of code. But it seems to work in other cases so if it solves this one, then let's do this.

As for whether we support nested tables – not officially :D But we know that it worked so far if someone was enabling nested tables in the schema.

@oleq
Copy link
Member

oleq commented Mar 11, 2020

I created a PR. Let's find out how it performs.

@Reinmar Reinmar assigned oleq and unassigned Reinmar Mar 11, 2020
mlewand added a commit to ckeditor/ckeditor5-table that referenced this issue Mar 11, 2020
Fix: Table border styles conversion handler should not throw when it approaches a nested table. Closes ckeditor/ckeditor5#6177.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:table type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
4 participants