Opened 7 years ago

Closed 6 years ago

#5819 closed Bug (fixed)

Remove format breaks table show border

Reported by: garry.yao Owned by: Saare
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 Saare 6 years ago.
5819_2.patch (647 bytes) - added by Saare 6 years ago.
5819_3.patch (1.1 KB) - added by Saare 6 years ago.
5819_4.patch (1.6 KB) - added by Saare 6 years ago.
5819_5.patch (636 bytes) - added by Saare 6 years ago.

Download all attachments as: .zip

Change History (20)

Changed 6 years ago by Saare

comment:1 Changed 6 years ago by Saare

  • Keywords Review? added
  • Owner set to Saare
  • Status changed from new to assigned

comment:2 Changed 6 years ago by alfonsoml

  • 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 6 years ago by Saare

comment:3 Changed 6 years ago by Saare

  • Keywords Review? added; Review- removed

comment:4 Changed 6 years ago by alfonsoml

  • Keywords Review- added; Review? removed

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

Changed 6 years ago by Saare

comment:5 Changed 6 years ago by Saare

  • Keywords Review? added; Review- removed

Nice catch.

comment:6 Changed 6 years ago by fredck

  • 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 6 years ago by alfonsoml

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 6 years ago by fredck

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

Changed 6 years ago by Saare

comment:9 Changed 6 years ago by Saare

  • Keywords Review? added; Review- removed

The performance impact is barley noticable according to my tests.

comment:10 Changed 6 years ago by fredck

  • Status changed from review to review_passed

comment:11 Changed 6 years ago by Saare

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

Fixed with [5727].

comment:12 Changed 6 years ago by fredck

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Changed 6 years ago by Saare

comment:13 Changed 6 years ago by Saare

  • Keywords Confirmed removed
  • Status changed from reopened to review

comment:14 Changed 6 years ago by fredck

  • Status changed from review to review_passed

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

comment:15 Changed 6 years ago by Saare

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

Fixed with [5770].

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