Opened 3 years ago

Closed 2 years ago

#14451 closed Bug (fixed)

Widget crashes if editable region has a numeric ID

Reported by: egli Owned by: Tomasz Jakut
Priority: Normal Milestone: CKEditor 4.5.10
Component: General Version: 4.4.0
Keywords: Cc:

Description

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.

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

Attachments (2)

14451.patch (2.9 KB) - added by Jakub Chalupa 3 years ago.
#14451 - Widget crashes if editable region has a numeric ID patch
14451-2.patch (6.5 KB) - added by Jakub Chalupa 3 years ago.
fixed patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 3 years ago by egli

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

comment:2 Changed 3 years ago by Jakub Ś

Status: newconfirmed
Version: 4.5.74.4.0

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

We should allow both numbers and letters as restricions for id value were loosen up - https://www.w3.org/TR/html-markup/datatypes.html#common.data.id

Last edited 3 years ago by Jakub Ś (previous) (diff)

comment:3 Changed 3 years ago by Jakub Chalupa

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

Changed 3 years ago by Jakub Chalupa

Attachment: 14451.patch added

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

comment:4 Changed 3 years ago by Jakub Chalupa

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

Changed 3 years ago by Jakub Chalupa

Attachment: 14451-2.patch added

fixed patch

comment:5 in reply to:  4 ; Changed 3 years ago by 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().

comment:6 in reply to:  5 ; Changed 3 years ago by 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!

comment:7 in reply to:  6 Changed 3 years ago by 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!

comment:8 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8

Let's continue the discussion here.

comment:9 Changed 3 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:10 Changed 3 years ago by Tomasz Jakut

Status: assignedreview

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

comment:11 Changed 3 years ago by Tomasz Jakut

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

comment:12 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:13 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:14 Changed 2 years ago by Tade0

Resolution: fixed
Status: reviewclosed

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.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy