Opened 8 years ago

Closed 7 years ago

#5455 closed Bug (fixed)

we can't remove the format of the copied & pasted text by selecting the text and clicking on Formatted icon

Reported by: Satya Minnekanti Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.4
Component: Core : Styles Version: 3.0
Keywords: IBM Cc: Damian, joek

Description

To reproduce the defect:

  1. Open Ajax sample.
  1. Type few lines of text.
  1. Apply Bold Formatting to the text in the First line.
  1. Copy the Formatted text in the first line and Paste it at the end of last line.
  1. Select the Pasted text and see that the Bold icon in Toolbar is highlighted.
  1. Click on Bold Icon.

Expected Result:

Bold Format applied to the text is removed

Actual Result:

Bold Format applied to the text is not removed.

We can remove the format using only the following two ways.

Go to HTML Source and Come back to WYSIWYG mode and select the text again and click Bold icon or

Select the Pasted text and Click on Remove Format Icon.

Same behaviour happens with Italic,Underline,Strike Through,Subscript and Superscript formatting options.

Tested against FF3,IE 6&7

Attachments (6)

5455.patch (846 bytes) - added by brooks 7 years ago.
5455_2.patch (863 bytes) - added by brooks 7 years ago.
5455_3.patch (864 bytes) - added by brooks 7 years ago.
5455_4.patch (772 bytes) - added by Garry Yao 7 years ago.
5455_5.patch (2.2 KB) - added by Sa'ar Zac Elias 7 years ago.
5455_6.patch (833 bytes) - added by Frederico Caldeira Knabben 7 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added
Milestone: CKEditor 3.3CKEditor 3.4
Version: 3.23.0

Confirmed with CKEditor 3.0.

comment:2 Changed 7 years ago by brooks

Owner: set to brooks
Status: newassigned

comment:3 Changed 7 years ago by brooks

only reproduceable in FF

Changed 7 years ago by brooks

Attachment: 5455.patch added

comment:4 Changed 7 years ago by brooks

Keywords: review? added

Changed 7 years ago by brooks

Attachment: 5455_2.patch added

Changed 7 years ago by brooks

Attachment: 5455_3.patch added

comment:5 Changed 7 years ago by brooks

pls just review for 5455_3.patch

comment:6 Changed 7 years ago by Garry Yao

Keywords: Discussion review- added; Confirmed review? removed

The patch doesn't work when both attributes present on element.

On the other hands, I think our styles system is doing wrong in this sense, though we have kept it for a long time since v2, e.g. user put a class attribute on the strong element could easily break the style removing, which doesn't make sense.

Changed 7 years ago by Garry Yao

Attachment: 5455_4.patch added

comment:7 Changed 7 years ago by Garry Yao

Component: GeneralCore : Styles

Proposing a solution to have it discussed.

comment:8 Changed 7 years ago by Garry Yao

Keywords: Review? added; review- removed

comment:9 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from brooks to Garry Yao
Status: assignednew

comment:10 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The proposed approach is wrong, because now we'll leave an empty <span> when removing the bold formatting, just because of the presence of the _cke_expando attribute.

We need to check where we're using element.hasAttributes(), but maybe we can make it ignore the _cke_expando attribute. In that case the change would be much simpler and safer.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5455_5.patch added

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

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

The patch makes the hasAttributes method to ignore the _moz_dirty attribute, as well as adds Garry's solution.

comment:12 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Having or not to handle removal of styles partially valid for the selection, replacing them with <span>, is something out of the scope of this ticket, and needs discussions, together with a real world test case.

comment:13 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from Sa'ar Zac Elias to Frederico Caldeira Knabben
Status: review_failedassigned

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 5455_6.patch added

comment:14 Changed 7 years ago by Frederico Caldeira Knabben

Status: assignedreview

KISS

comment:15 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

Filed #6026 to discuss Garry's proposal.

comment:16 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [5741].

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