Opened 10 years ago

Closed 10 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 10 years ago.
6107_2.patch (2.6 KB) - added by Sa'ar Zac Elias 10 years ago.
6107_3.patch (3.9 KB) - added by Sa'ar Zac Elias 10 years ago.
6107_4.patch (5.5 KB) - added by Garry Yao 10 years ago.
6107_5.patch (5.6 KB) - added by Garry Yao 10 years ago.
6107_6.patch (5.6 KB) - added by Sa'ar Zac Elias 10 years ago.
6107_7.patch (5.4 KB) - added by Garry Yao 10 years ago.
6107_8.patch (5.3 KB) - added by Sa'ar Zac Elias 10 years ago.
6107_9.patch (6.1 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (32)

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

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

Changed 10 years ago by Sa'ar Zac Elias

Attachment: 6107.patch added

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

Status: assignedreview

comment:3 Changed 10 years ago by Tobiasz Cudnik

Seems to be DUP of #5336.

comment:4 Changed 10 years ago by Wiktor Walc

Milestone: CKEditor 3.5.1

comment:5 Changed 10 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 10 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 10 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 10 years ago by Sa'ar Zac Elias

Attachment: 6107_2.patch added

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

Status: assignedreview

comment:9 Changed 10 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 10 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 10 years ago by Sa'ar Zac Elias

Attachment: 6107_3.patch added

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

Status: review_failedreview

comment:12 Changed 10 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 10 years ago by Garry Yao

Attachment: 6107_4.patch added

comment:13 Changed 10 years ago by Garry Yao

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

comment:14 Changed 10 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 10 years ago by Garry Yao

Attachment: 6107_5.patch added

comment:15 Changed 10 years ago by Garry Yao

Status: review_failedreview

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

Status: reviewreview_failed

Block removal does not handle extra styles/attributes.

Changed 10 years ago by Sa'ar Zac Elias

Attachment: 6107_6.patch added

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

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

Changed 10 years ago by Garry Yao

Attachment: 6107_7.patch added

comment:18 Changed 10 years ago by Garry Yao

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

A simplified patch to 6107_6.

comment:19 Changed 10 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 10 years ago by Sa'ar Zac Elias

Attachment: 6107_8.patch added

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

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

Changed 10 years ago by Garry Yao

Attachment: 6107_9.patch added

comment:21 Changed 10 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 10 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 10 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: review_passedclosed

Fixed with [6344].

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