Widget crashes if editable region has a numeric ID

[egli]

CKEditor throws an error if a valid widget has an editable element with a numeric ID (or any ID that needs escaped/serialized).

Underlying cause is getContextualizedSelector() in CKEDITOR.dom.element not escaping/serializing IDs when converting to a CSS selector, resulting in an invalid query passed to querySelectorAll() inside of CKEDITOR.dom.element.find(). Something akin to CSS.escape() is needed to correctly format the ID.

• W3C specification for valid CSS identifiers: ​https://www.w3.org/TR/CSS21/syndata.html#value-def-identifier
• W3C documentation on escaping/serializing IDs: ​https://drafts.csswg.org/cssom/#serialize-an-identifier
• MDN article on CSS.escape(): ​https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape

Steps to reproduce

1. Navigate to "Enhanced Image" example on ​http://ckeditor.com/demo#widgets
2. Go into source mode
3. Add a numeric id to the figcaption element (say, id="100")
4. Exit source mode

Expected result

CKEditor successfully returns to WYSIWYG mode; Widget and CKEditor continue to function normally.

Actual result

Everything is terrible. A JavaScript error is thrown. Editing area returns to WYSIWYG, but buttons are not re-enabled. Tons of CKEditor features don't work. Double clicking on widget does not bring up the editor dialog. Image in widget cannot be resized.

Other details (browser, OS, CKEditor version, installed plugins)

Only tested on CKEditor 4.5.7, but getContextualizedSelector is unchanged since it was added in 4.3.

Issue occurs in every browser I tested:

• Chrome 48.0.2564.116 (64-bit) on both Mac and Linux
• Safari 9.0.3 (11601.4.4) on OS X 10.11.3

[egli]

Another solution: the element ID could be replaced by createTmpId() and restored by removeTmpId(), removing the need to escape the current ID.

[Jakub Ś]

I have been able to reproduce this issue from CKEditor 4.4 (works fine in CKE 4.3.5)

[Jakub Chalupa]

Hi, I'd like to solve this issue if it's really still unassigned.

[Jakub Chalupa]

#14451 - Widget crashes if editable region has a numeric ID patch

[Jakub Chalupa]

Fixed & tested. Patch included, pull request created - ​https://github.com/ckeditor/ckeditor-dev/pull/257

[Jakub Chalupa]

fixed patch

[egli]

Replying to chaluja7:

Fixed & tested. Patch included, pull request created - ​https://github.com/ckeditor/ckeditor-dev/pull/257

Thanks for picking this up! Unfortunately since filing this I have run into several IDs of the form something.100, which your patches would does not fix. It might be better to address this problem through temporarily replacing existing IDs in createTmpId().

[Jakub Chalupa]

Replying to egli:

Replying to chaluja7:

Fixed & tested. Patch included, pull request created - ​https://github.com/ckeditor/ckeditor-dev/pull/257

Thanks for picking this up! Unfortunately since filing this I have run into several IDs of the form something.100, which your patches would does not fix. It might be better to address this problem through temporarily replacing existing IDs in createTmpId().

Thanks for report. However I'm not sure if you noticed the conversation about this issue here: ​https://github.com/ckeditor/ckeditor-dev/pull/257. In case that you didn't, please leave comment there. Thanks!

[egli]

Actually, disregard my comment because I only care about behavior in Chrome where the native CSS.escape() will be used anyway. So no complaints with your pull request from me. Thanks again for tackling this!

[Marek Lewandowski]

Let's continue the discussion here.

[Tomasz Jakut]

I've simplified checkings in CKEDITOR.tools.escapeCss & moved tests into widget folder. Pushed ​branch:t/14451.

[Tomasz Jakut]

There are also other issues with escaping selectors: #14533.

[Tade0]

I DRYied one fragment so that there are less of those long chains of references.

Apart from that it works and is adequately tested.

Fixed with ​git:d02bb27f52a769aa58c8795e909c94219d0fd790.