Opened 6 years ago

Closed 6 years ago

#6107 closed Bug (fixed)

Remove block style via the list

Reported by: Saare Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.5.1
Component: Core : Styles Version: 3.0
Keywords: Cc:

Description (last modified by Saare)

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 Saare 6 years ago.
6107_2.patch (2.6 KB) - added by Saare 6 years ago.
6107_3.patch (3.9 KB) - added by Saare 6 years ago.
6107_4.patch (5.5 KB) - added by garry.yao 6 years ago.
6107_5.patch (5.6 KB) - added by garry.yao 6 years ago.
6107_6.patch (5.6 KB) - added by Saare 6 years ago.
6107_7.patch (5.4 KB) - added by garry.yao 6 years ago.
6107_8.patch (5.3 KB) - added by Saare 6 years ago.
6107_9.patch (6.1 KB) - added by garry.yao 6 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 6 years ago by Saare

  • Owner set to Saare
  • Status changed from new to assigned

Changed 6 years ago by Saare

comment:2 Changed 6 years ago by Saare

  • Status changed from assigned to review

comment:3 follow-up: Changed 6 years ago by tobiasz.cudnik

Seems to be DUP of #5336.

comment:4 Changed 6 years ago by wwalc

  • Milestone set to CKEditor 3.5.1

comment:5 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

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

comment:6 in reply to: ↑ 3 Changed 6 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 6 years ago by Saare

  • Description modified (diff)
  • Status changed from review_failed to assigned
  • Summary changed from Remove object style via the list to Remove block style via the list

Yep, block styles this time.

Changed 6 years ago by Saare

comment:8 Changed 6 years ago by Saare

  • Status changed from assigned to review

comment:9 Changed 6 years ago by garry.yao

  • Status changed from review to review_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 6 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 6 years ago by Saare

comment:11 Changed 6 years ago by Saare

  • Status changed from review_failed to review

comment:12 Changed 6 years ago by garry.yao

  • Status changed from review to review_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 6 years ago by garry.yao

comment:13 Changed 6 years ago by garry.yao

  • Owner changed from Saare to garry.yao
  • Status changed from review_failed to review

comment:14 Changed 6 years ago by Saare

  • Status changed from review to review_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 6 years ago by garry.yao

comment:15 Changed 6 years ago by garry.yao

  • Status changed from review_failed to review

comment:16 Changed 6 years ago by Saare

  • Status changed from review to review_failed

Block removal does not handle extra styles/attributes.

Changed 6 years ago by Saare

comment:17 Changed 6 years ago by Saare

  • Owner changed from garry.yao to Saare
  • Status changed from review_failed to review

Changed 6 years ago by garry.yao

comment:18 Changed 6 years ago by garry.yao

  • Owner changed from Saare to garry.yao

A simplified patch to 6107_6.

comment:19 Changed 6 years ago by Saare

  • Status changed from review to review_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 6 years ago by Saare

comment:20 Changed 6 years ago by Saare

  • Owner changed from garry.yao to Saare
  • Status changed from review_failed to review

Changed 6 years ago by garry.yao

comment:21 Changed 6 years ago by garry.yao

  • Owner changed from Saare 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 6 years ago by garry.yao

  • Status changed from review to review_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 6 years ago by Saare

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [6344].

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