Opened 8 years ago

Closed 8 years ago

#8342 closed Bug (fixed)

ReadOnly sample - paste buttons reenabled, when "make read only" pressed

Reported by: Krzysztof Studnik Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.2
Component: UI : Toolbar Version: 3.6.2
Keywords: WebKit Cc:

Description

Safari/Mac

TC

  • go to sample ReadOnly
  • press "make read only" button : Editing is disabled, Copy/paste buttons are disabled
  • click editor body, and press some random keys on keyboard

Result

Paste as text, paste, paste from word buttons on toolbar are reenabled. When paste from word button is pressed, dialog window opens and user is able to paste ms word content. Text, pasted into dialog window, is not pasted to editor body.

Expected

Operations done on editor body, while read only mode is enabled, should not have influence on toolbar. Disabled buttons should stay disabled.

Attachments (4)

8342.patch (611 bytes) - added by Sa'ar Zac Elias 8 years ago.
8342_2.patch (1.4 KB) - added by Garry Yao 8 years ago.
8342_3.patch (1.2 KB) - added by Garry Yao 8 years ago.
8342_4.patch (602 bytes) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by Sa'ar Zac Elias

Keywords: WebKit added; Safari removed
Status: newconfirmed

Regression of this version, also in Chrome.

comment:2 Changed 8 years ago by Frederico Caldeira Knabben

Caused by [7242].

comment:3 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: WebKit removed

Confirmed in all browsers.

comment:4 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.2

comment:5 in reply to:  3 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: WebKit added

Replying to fredck:

Confirmed in all browsers.

My fault... that's WebKit only.

Changed 8 years ago by Sa'ar Zac Elias

Attachment: 8342.patch added

comment:6 Changed 8 years ago by Sa'ar Zac Elias

Owner: set to Sa'ar Zac Elias
Status: confirmedreview

Since I didn't find anything else that doesn't work.. I think it's safe to just check read only mode to avoid problems.

comment:7 Changed 8 years ago by Garry Yao

Keywords: NeedsTest added
Status: reviewreview_failed

Not all clipboard commands are to be excluded from readonly, e.g. "copy". Please add a automatic test for it.

Changed 8 years ago by Garry Yao

Attachment: 8342_2.patch added

comment:8 Changed 8 years ago by Garry Yao

Owner: changed from Sa'ar Zac Elias to Garry Yao
Status: review_failedreview

comment:9 Changed 8 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • Tests don't pass with that patch in IE and FF.
  • Noneditable selections are not being considered with the patch, e.g.
    <p contenteditable="false">aa^aa</p>
    

Note that the copy command is anyway disabled in WebKit, not due to the patch.

Last edited 8 years ago by Sa'ar Zac Elias (previous) (diff)

comment:10 Changed 8 years ago by Garry Yao

Status: review_failedreview_passed

R+ for first patch, I thought wrongly last review.

comment:11 Changed 8 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: review_passedclosed

Fixed with [7267], I see that the tt was already updated.

comment:12 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

After some investigation, there's a deeper reason behind this bug instead, which is caused by JS error on "selectionchange" event listeners.

Changed 8 years ago by Garry Yao

Attachment: 8342_3.patch added

comment:13 Changed 8 years ago by Garry Yao

Status: reopenedreview

comment:14 Changed 8 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

Nice catch @garry.yao.

comment:15 Changed 8 years ago by Garry Yao

Keywords: NeedsTest removed
Resolution: fixed
Status: review_passedclosed

Fixed with [7269].

Changed 8 years ago by Garry Yao

Attachment: 8342_4.patch added

comment:16 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

As previous commit doesn't pass the tt in all browsers.

comment:17 Changed 8 years ago by Garry Yao

Status: reopenedreview

comment:18 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: reviewclosed

Yet another the review reveals it a fabricated problem created by the tc instead of a real bug, the patch is obsoleted after tc is updated.

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