Opened 7 years ago

Closed 7 years ago

#5819 closed Bug (fixed)

Remove format breaks table show border

Reported by: Garry Yao Owned by: Sa'ar Zac Elias
Priority: Normal Milestone: CKEditor 3.4
Component: General Version:
Keywords: Cc:

Description

Reproducing Procedures

  1. Open any of the sample page, insert a default table with no border width;
  2. Select All content then click remove format button;
    • Actual Result: The border line of the table is removed.

Attachments (5)

5819.patch (509 bytes) - added by Sa'ar Zac Elias 7 years ago.
5819_2.patch (647 bytes) - added by Sa'ar Zac Elias 7 years ago.
5819_3.patch (1.1 KB) - added by Sa'ar Zac Elias 7 years ago.
5819_4.patch (1.6 KB) - added by Sa'ar Zac Elias 7 years ago.
5819_5.patch (636 bytes) - added by Sa'ar Zac Elias 7 years ago.

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5819.patch added

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

Keywords: Review? added
Owner: set to Sa'ar Zac Elias
Status: newassigned

comment:2 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

I don't think that the filter is a good solution here because it will make any other property remain in the table instead of applying the removal of format.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5819_2.patch added

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

Keywords: Review? added; Review- removed

comment:4 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

If the "border" is included in config.removeFormatAttributes then the patch has no effect.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5819_3.patch added

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

Keywords: Review? added; Review- removed

Nice catch.

comment:6 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The proposed patch is a simple and good fix for this ticket.

But, we could make things a bit more flexible. What about having the removeformat plugin firing an event for each element that is cleaned up, so it can be listened by other plugins and eventually fix things?

We may just check the performance impact of it, but it's worth trying.

Feel free to ask for review again if you feel this approach is not good.

comment:7 Changed 7 years ago by Alfonso Martínez de Lizarrondo

My idea about the clean up would be to fire a single event after the command has executed, so in the showborders plugin just has to do a getElementsByTagName("table") to find the tables and check them.

comment:8 Changed 7 years ago by Frederico Caldeira Knabben

That's a good option, in case the performance impact of the "per element" approach will be noticeable.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5819_4.patch added

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

Keywords: Review? added; Review- removed

The performance impact is barley noticable according to my tests.

comment:10 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed

Fixed with [5727].

comment:12 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: closedreopened

I'm still able to reproduce this issue on trunk.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5819_5.patch added

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

Keywords: Confirmed removed
Status: reopenedreview

comment:14 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

In fact, it was not only a matter of leaving the border field blank, but setting it to 0 instead.

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

Resolution: fixed
Status: review_passedclosed

Fixed with [5770].

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