Opened 14 years ago

Closed 14 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 14 years ago.
5819_2.patch (647 bytes) - added by Sa'ar Zac Elias 14 years ago.
5819_3.patch (1.1 KB) - added by Sa'ar Zac Elias 14 years ago.
5819_4.patch (1.6 KB) - added by Sa'ar Zac Elias 14 years ago.
5819_5.patch (636 bytes) - added by Sa'ar Zac Elias 14 years ago.

Download all attachments as: .zip

Change History (20)

Changed 14 years ago by Sa'ar Zac Elias

Attachment: 5819.patch added

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

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

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

Attachment: 5819_2.patch added

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

Keywords: Review? added; Review- removed

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

Attachment: 5819_3.patch added

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

Keywords: Review? added; Review- removed

Nice catch.

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

Attachment: 5819_4.patch added

comment:9 Changed 14 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 14 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed

Fixed with [5727].

comment:12 Changed 14 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: closedreopened

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

Changed 14 years ago by Sa'ar Zac Elias

Attachment: 5819_5.patch added

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

Keywords: Confirmed removed
Status: reopenedreview

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

Resolution: fixed
Status: review_passedclosed

Fixed with [5770].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy