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 elements (td, tr, th, ..) with an id that starts with dot (.) causes JavaScript runtime err #681

Closed
cschettle opened this issue Jul 26, 2017 · 11 comments · Fixed by #4666
Assignees
Labels
plugin:tableselection The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Milestone

Comments

@cschettle
Copy link

cschettle commented Jul 26, 2017

Are you reporting a feature or a bug?

Bug

Check if the issue is already reported

Put all reference links here…

Provide detailed reproduction steps (if any)

  1. In the html for a table add an id to a one of the table cells that starts with dot. for example make the td element have an id=".amt"
  2. Run with IE 11 and press F12 to get to the debugger
  3. Select the table cell in the editor
  4. You will see the Syntax error (in the console window) (see attached) IE11_syntax_err.PNG

Expected result

What is the expected result of the above steps?
The table cell is selected (highlighted)

Actual result

What is the actual result of the above steps?
Only IE 11 throws this exception :
0x800a139e - JavaScript runtime error: SyntaxError
in ckeditor.js (minified version) or in dom\element.js in the source version
in findOne (or also find) at line 4024

Other details

The syntax error is a result of a bad CSS selector being returned from
getContextualizedSelector( this, selector ) (see element.js) While FF and Chrome do not throw an exception, the selector does would not work properly. The selector produced using the attached table.htm file (and selecting any cell with an id=.amt.3 is
#.amt.3 td, #.amt.3 th, #.amt.3 tr, #.amt.3 tbody, #.amt.3 table

You can test this selector using an online tool like https://try.jsoup.org/
I have attached the following files:
table.txt html file that contains table with cells that have id's with dot
IE11_syntax_err.PNG shows err in IE11 dev tools with ckeditor minified version
IE11_syntax_err_src.PNG shows err in IE11 dev tools with ckeditor source
bad_selector.PNG shows value of selector returned from getContextualizedSelector which is called from find and findOne in dom\element.js
table.txt
bad_selector
ie11_syntax_err
ie11_syntax_err_src

The original code:
return '#' + id + ' ' + selector.split(/,\s*/).join(', #' + id + ' ');
The #.amt for example is not a valid selector (would need to be escaped)
I modified the code to use the attribute style selector instead of id # as follows:
return '[id="' + id + '"] ' + selector.split(/,\s*/).join(', [id="' + id + '"] ');

  • Browser: FF, IE 11, Edge, Chrome (although only IE gives exception)
  • OS: Windows 7 or Windows Server 2012 R2
  • CKEditor version: 4.7.0
  • Installed CKEditor plugins: …
@beatadelura
Copy link
Contributor

beatadelura commented Jul 27, 2017

@cschettle I can't reproduce this issue with CKEditor version 4.7.0 and 4.7.1. Could you please provide codepen, jsbin or jsfiddle example with configuration and exact steps to reproduce?

@cschettle
Copy link
Author

I updated the original issue to be more descriptive and provide additional information. I have attached a smaller .htm file (table_small.txt) for your testing... (the original test.htm file has duplicate Id's which might confuse the issue)

table_small.txt

@beatadelura
Copy link
Contributor

@cschettle Thanks for your response. Could you please take a look at this demo https://jsfiddle.net/o166vxma/? Please run this on IE11 and follow your steps. Selecting cells doesn't cause an error in the developer console.

@cschettle
Copy link
Author

cschettle commented Aug 4, 2017 via email

@beatadelura
Copy link
Contributor

@cschettle Thank you for adding more information. I see the problem now and I can reproduce this issue.

@beatadelura beatadelura added type:bug A bug. plugin:tableselection The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. and removed status:pending labels Aug 7, 2017
@cschettle
Copy link
Author

cschettle commented Aug 7, 2017 via email

@cschettle
Copy link
Author

I see this is not yet fixed in the ckeditor 4.7.3 Here is how I fixed the code in ckeditor\dom\element.js
Change original code:
function getContextualizedSelector( element, selector ) {
var id = CKEDITOR.tools.escapeCss( element.$.id );
return '#' + id + ' ' + selector.split( /,\s*/ ).join( ', #' + id + ' ' );
}
To:
function getContextualizedSelector( element, selector ) {
var id = CKEDITOR.tools.escapeCss(element.$.id);
return '[id="' + id + '"] ' + selector.split(/,\s*/).join(', [id="' + id + '"] ');;
}

@msamsel
Copy link
Contributor

msamsel commented Dec 5, 2017

Hi @cschettle,
if you have ready solution and you are sure it won't break anything, maybe you could consider contributing and creating pull request with your change?
More information about contributing you can find here: https://github.com/ckeditor/ckeditor-dev/blob/major/.github/CONTRIBUTING.md.

@mflorea
Copy link
Contributor

mflorea commented May 7, 2020

The problem is in the implementation of CKEDITOR.tools.escapeCss() which doesn't return the expected result in IE11 because IE11 doesn't support CSS.escape(). This needs to be fixed ASAP because you advertise "full support" for IE11. A basic implementation for browsers that don't support CSS.escape() could be https://learn.jquery.com/using-jquery-core/faq/how-do-i-select-an-element-by-an-id-that-has-characters-used-in-css-notation/ but you can also implement https://drafts.csswg.org/cssom/#common-serializing-idioms if you want to be bulletproof.

Note that #641 could be caused by the same issue: incomplete implementation of CKEDITOR.tools.escapeCss().

@limingli0707
Copy link
Contributor

Hi Team. It seems the bug has not been fixed. Based on the conversation above, seems someone has been already looking into this. Do we have any updates? Thanks a lot!

@sculpt0r
Copy link
Contributor

Closed with: #4666

@f1ames f1ames added this to the 4.16.2 milestone Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:tableselection The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants