Opened 9 years ago

Closed 9 years ago

#13872 closed Bug (fixed)

Read-only mode allows cut

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

Description

Steps to reproduce

  1. Load CKEditor read-only, http://ckeditor.com/demo#read-only
  2. Select text
  3. Ctrl-X or Command-X
  4. Ctrl-Z or Command-Z

Expected result

  1. During cut, nothing should happen, or text should be placed on clipboard without being removed.
  2. Undo should have no effect.

Actual result

  1. Cut removes text and puts in on the clipboard.
  2. Undo exits out of read-only mode.

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

Tested on Chrome/Linux, Chrome/Mac, Safari/Mac.

Change History (8)

comment:1 Changed 9 years ago by egli

Actually, this seems to be Chrome only, and does not affect undo. I had loaded the wrong demo when testing against Safari.

comment:2 Changed 9 years ago by Jakub Ś

Status: newconfirmed
Version: 4.5.44.5.0

Problem can be reproduced from CKEditor 4.5.0 in all browsers.

comment:3 Changed 9 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5

comment:4 Changed 9 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:5 Changed 9 years ago by Tomasz Jakut

Status: assignedreview

There was missing check in clipboard plugin if the editor is in read-only mode. Pushed fix to branch:t/13872

comment:6 Changed 9 years ago by Marek Lewandowski

Status: reviewreview_failed
  1. Now CKEDITOR.plugins.clipboard.initPasteDataTransfer will be called no matter what, resulting with changing your clippboard buffer if you'll do cut in readOnly mode.

The reason for that is that you're checking wrong event property evt.type where it should be evt.name. I've found this one out while adding new tests.

  1. If I'm not mistaken this condition can be _shortened_ down to:
if ( !editor.readOnly || evt.name != 'cut' ) {

I've emphasized shortened, because I can't tell if it's equally easy to understand at first glance. We should go with easier to understand one, it's up to you to pick easier one.

  1. Please update manual tests accordiginly for testser also to ensure that nothing got copied to the clipboard.

I've pushed some my changes to branch:t/13872, make sure to pull them.

Last edited 9 years ago by Marek Lewandowski (previous) (diff)

comment:7 Changed 9 years ago by Tomasz Jakut

Status: review_failedreview

1, 2. Yes, you're right.

  1. Updated.

I also updated unit tests because they were failing in IE. Pushed changes to branch:t/13872.

comment:8 Changed 9 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

Merged to master with git:48311f8.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy