Opened 12 years ago

Closed 9 years ago

#617 closed Bug (wontfix)

Table: TD should use style to set borderColor

Reported by: Geir Helge Tjøstolvsen-Schmidt Owned by: Martin Kou
Priority: Normal Milestone:
Component: General Version: FCKeditor 2.4.3
Keywords: Review- Cc:

Description

When selecting borderColor for a TD it's set as an attribute. There's no borderColor attribute for the TD element (works in most browsers, but not valid). Style="bordercolor:#xxxxxx" should be used instead.

Attaching suggested fix

Attachments (3)

fck_tablecell.html.patch (1.7 KB) - added by Geir Helge Tjøstolvsen-Schmidt 12 years ago.
Suggested fix
617.patch (2.1 KB) - added by Martin Kou 12 years ago.
Proposed patch for solving #617.
617_2.patch (9.3 KB) - added by Martin Kou 12 years ago.
Proposed fix for #617, version 2.

Download all attachments as: .zip

Change History (16)

Changed 12 years ago by Geir Helge Tjøstolvsen-Schmidt

Attachment: fck_tablecell.html.patch added

Suggested fix

comment:1 Changed 12 years ago by Martin Kou

Milestone: FCKeditor 2.6
Owner: set to Martin Kou
Status: newassigned

comment:2 Changed 12 years ago by Martin Kou

Keywords: HasPatch added

comment:3 Changed 12 years ago by Frederico Caldeira Knabben

Note on this: it must be backward compatible, looking for the color value in the borderColor attribute on load, and removing the attribute when setting the style on save.

comment:4 Changed 12 years ago by Martin Kou

The proposed patch does not work under Firefox, so I'm proposing another patch.

Changed 12 years ago by Martin Kou

Attachment: 617.patch added

Proposed patch for solving #617.

comment:5 Changed 12 years ago by Martin Kou

Keywords: Review? added; HasPatch removed

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

The problem from my point of view would be that if someone pasted content from another source, usually Word, (or manually edited the source), and that cell have a custom border like different colors for each border or width!=1px or style!=solid, then as soon as he edited the cell those properties would be lost.

I would rather have settings to also change the width and style of the border, and being able to set all the borders the same or each one independently.

But in fact I would rather have a style editor instead of putting everything in the main dialog for the cell (because people also want to customize color, background-color, background-image...)

comment:7 Changed 12 years ago by Martin Kou

Keywords: Review? removed

Another problem I just found is that different browsers are giving out vastly different HTML outputs for the border styles. Firefox gives the cleanest HTML output, while Safari attaches a huge assortment of CSS styles to the HTML output.

Changed 12 years ago by Martin Kou

Attachment: 617_2.patch added

Proposed fix for #617, version 2.

comment:8 Changed 12 years ago by Martin Kou

Keywords: Review? added

I've added an updated patch for fixing this bug. The changes are moderately extensive for a seemingly simple bug.

The updated patch improves 2 things:

  1. If there's any already existing border styles in the table cell ( e.g. a dashed 2px border), the non-color border styles will not be destroyed.
  2. The HTML output of border styles is now the same across all browsers.

comment:9 Changed 12 years ago by Martin Kou

The v2 patch should have fixed #460 as well.

comment:10 in reply to:  8 Changed 12 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Replying to martinkou:

The changes are moderately extensive for a seemingly simple bug.

You are correct Martin, and the extension of the changes could bring more problems than benefits. I think we must review the strategy for this ticket.

To easily give a Review-, the changes on _GetMainXmlString should be moved to GetXHTML, together with all other string processing we are doing there. In any case, I would ask a hold in this ticket for better understanding over the current patch.

In any case, this ticket has been mistakenly targeted to the 2.6. I'm re-targeting it to the 2.7, which is related to the table support review.

comment:11 Changed 12 years ago by Frederico Caldeira Knabben

Milestone: FCKeditor 2.6FCKeditor 2.7

comment:12 Changed 12 years ago by Martin Kou

#171 is also partially related to this ticket, in that part of the proposed patch can be used to fix #171.

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

Resolution: wontfix
Status: assignedclosed

Fixed in CKEditor with #4893

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