Opened 7 years ago

Closed 7 years ago

#6107 closed Bug (fixed)

Remove block style via the list

Reported by: Sa'ar Zac Elias Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.5.1
Component: Core : Styles Version: 3.0
Keywords: Cc:

Description (last modified by Sa'ar Zac Elias)

Currently it's impossible to remove block styles via the combo by clicking on the active option.

Attachments (9)

6107.patch (1.6 KB) - added by Sa'ar Zac Elias 7 years ago.
6107_2.patch (2.6 KB) - added by Sa'ar Zac Elias 7 years ago.
6107_3.patch (3.9 KB) - added by Sa'ar Zac Elias 7 years ago.
6107_4.patch (5.5 KB) - added by Garry Yao 7 years ago.
6107_5.patch (5.6 KB) - added by Garry Yao 7 years ago.
6107_6.patch (5.6 KB) - added by Sa'ar Zac Elias 7 years ago.
6107_7.patch (5.4 KB) - added by Garry Yao 7 years ago.
6107_8.patch (5.3 KB) - added by Sa'ar Zac Elias 7 years ago.
6107_9.patch (6.1 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (32)

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

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

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6107.patch added

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

Status: assignedreview

comment:3 Changed 7 years ago by Tobiasz Cudnik

Seems to be DUP of #5336.

comment:4 Changed 7 years ago by Wiktor Walc

Milestone: CKEditor 3.5.1

comment:5 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

The patch mixes "applyBlockStyle" and "removeBlockStyle" logic together which is bad.

comment:6 in reply to:  3 Changed 7 years ago by Garry Yao

Replying to tobiasz.cudnik:

Seems to be DUP of #5336.

We're talking of block style, no longer object style here.

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

Description: modified (diff)
Status: review_failedassigned
Summary: Remove object style via the listRemove block style via the list

Yep, block styles this time.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6107_2.patch added

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

Status: assignedreview

comment:9 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Two problems:

  1. Current implementation doesn't preserve other styles on block;
    <h3 style="color: blue;background-color:blue">heading</h3>
    
  2. Format combo not aligned;

comment:10 Changed 7 years ago by Garry Yao

We could reuse "removeFromElement" function:

  1. Remove only styles in case of partial match;
  2. Revert block in case of full match.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6107_3.patch added

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

Status: review_failedreview

comment:12 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

The patch fails the following TC:

  1. Remove "h1" format from the following content:
    <div>
    	<h1>^paragraph1</h1>
    </div>
    
  • Expected Result:
    <div>
    	<p>paragraph1</p>
    </div>
    
  • Actual Result:
    <div>
    	paragraph1
    </div>
    

Changed 7 years ago by Garry Yao

Attachment: 6107_4.patch added

comment:13 Changed 7 years ago by Garry Yao

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

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

Status: reviewreview_failed

Using enterMode=BR and the following contents and selection:

<h3 style="color: red;">
	Test</h3>

Removing the style should not put a BR on top (first element in the doc).

Changed 7 years ago by Garry Yao

Attachment: 6107_5.patch added

comment:15 Changed 7 years ago by Garry Yao

Status: review_failedreview

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

Status: reviewreview_failed

Block removal does not handle extra styles/attributes.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6107_6.patch added

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

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

Changed 7 years ago by Garry Yao

Attachment: 6107_7.patch added

comment:18 Changed 7 years ago by Garry Yao

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

A simplified patch to 6107_6.

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

Status: reviewreview_failed

The new patch doesn't cover the case when you apply "red title" style and then remove "heading 3" style (result should be a styled DIV).

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6107_8.patch added

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

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

Changed 7 years ago by Garry Yao

Attachment: 6107_9.patch added

comment:21 Changed 7 years ago by Garry Yao

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

Though not related, I'd like to propose the alignment of behavior when removing inline style, e.g. remove bold style from the following content:

<p><strong style="color:red">sam[ple</strong> text].</p>

comment:22 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

As #6026 is already opened for the above case, let's just stack the patch to be used there. R+ for 6107_8.patch

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

Resolution: fixed
Status: review_passedclosed

Fixed with [6344].

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