Opened 8 years ago

Closed 8 years ago

#4246 closed Bug (fixed)

Avoid deprecated image attributes

Reported by: mike890 Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.1
Component: Core : Output Data Version: 3.0 RC
Keywords: IBM Confirmed Review+ Cc: jkavanag@…, mimo@…

Description

The latest build of FCK 3.0 still generates the deprecated image attributes as align/hspace/vspace. All these attributes has been deprecated for several years already for the xhtml strict mode. They are also deprecated in HTML5.

Maybe the new version of the editor would be the great time to switch to the styles instead of these attributes? Otherwise, the generated HTML cannot pass validation.

Attachments (7)

fck_image.js (15.1 KB) - added by Florian 8 years ago.
4246.patch (16.0 KB) - added by Garry Yao 8 years ago.
4246_2.patch (21.4 KB) - added by Garry Yao 8 years ago.
4246_3.patch (23.8 KB) - added by Garry Yao 8 years ago.
4246_4.patch (23.5 KB) - added by Garry Yao 8 years ago.
4246_5.patch (24.2 KB) - added by Garry Yao 8 years ago.
4246_6.patch (24.8 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 8 years ago by Garry Yao

Component: GeneralCore : Output Data
Keywords: Confirmed added
Milestone: CKEditor 3.1

comment:2 Changed 8 years ago by W. Andrew Loe III

+1

This is very frustrating since we use a comprehensive reset stylesheet that removes all margins and padding to ensure we have full control.

comment:3 Changed 8 years ago by Frederico Caldeira Knabben

Summary: FCK 3.0 generates deprecated image attributesAvoid deprecated image attributes

We must just be sure the editor will remain backwards compatible with content produced in the current way.

Changed 8 years ago by Florian

Attachment: fck_image.js added

comment:4 Changed 8 years ago by Florian

Hello

I have written a workaround for using css instead of img tags, but has some restrictions. Please take a look and correct the code.

Important!!!:

  • It replaces old hspace, vspace, align automatically.
  • You can`t use stylefield in advanced tab any more.

comment:5 Changed 8 years ago by Garry Yao

#4631 has been marked as DUP.

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

Just for reference, #4631 also included the border handling into the list of things to be changed by this ticket.

comment:7 Changed 8 years ago by Joe Kavanagh

Cc: jkavanag@… added
Keywords: IBM added

comment:8 Changed 8 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 8 years ago by Garry Yao

Attachment: 4246.patch added

comment:9 Changed 8 years ago by Garry Yao

Keywords: Review? added

comment:10 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • The border doesn't get removed by simply emptying the border field.
  • We should not have two separated options "Align" and "Float". For the eyes of end users, we're always talking about "Align". So, for now, we could reduce the number of alignment options to "Left" and "Right" only. When loading an image with the "align" attribute, we should remove it only if it matches "left" or "right", otherwise we just leave it intact. No vertical alignment should be considered at this moment.
  • Actually... it looks like changes are not allowed at all, once you set some styles in the image.

comment:11 Changed 8 years ago by Garry Yao

Provide the spec behind the idea of only looking at the short-hand (composed) rule to determinate the inline style is here:

http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSS2Properties

Changed 8 years ago by Garry Yao

Attachment: 4246_2.patch added

comment:12 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Fixing the above mentioned issues.

comment:13 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • If you set the border or alignment and confirm and then editing it you remove the previously set value, an empty "style" attribute remains in the source.
  • The border style has a space before the semicolon (e.g. "border: 1px solid ;" instead of "border: 1px solid;". That's just a small detail, of course.
  • The dialog is broken when inserting new images. It works well for existing images only.
  • In IE, when editing an image with src only, when loading the dialog the HSpace value is "-1". By cleaning this field and confirming the dialog, "-1" margins get added to the image.

I'm not adding other issues here as it looks like we already have several listed. I think the next patching round must be tested better before putting it under review.

Changed 8 years ago by Garry Yao

Attachment: 4246_3.patch added

comment:14 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Ticket TCs updated.

comment:15 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

It's working as a charm in Firefox. Still issues in IE though:

  • It's not possible to add borders when editing an image that previously had no border.
  • It's not possible to remove the borders of images with borders.

Changed 8 years ago by Garry Yao

Attachment: 4246_4.patch added

comment:16 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

The culprit in IE is just onChange is not fired when typing, instead we have 'onKeyUp' now.

comment:17 Changed 8 years ago by Garry Yao

The border style has a space before the semicolon (e.g. "border: 1px solid ;" instead of "border: 1px solid;" in Firefox.

A browser issue for me, Firefox is expecting the third component of 'border' short-hand in position, which leave there an empty space.

comment:18 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

With IE:

  1. Load the following HTML:
<p><img alt="" src="http://www.google.com/intl/en_ALL/images/logo.gif" style="border-bottom: 5px solid; border-left: 5px solid; border-top: 5px solid; border-right: 5px solid" title="" /></p>
  1. Open the dialog for the image.
  1. Click the "Reset size" button.
  1. Confirm the dialog.

The sizes will not be in the output.

Changed 8 years ago by Garry Yao

Attachment: 4246_5.patch added

comment:19 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:20 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

With IE:

  1. Load the following HTML:
<p><img alt="" src="http://www.google.com/intl/en_ALL/images/logo.gif" style="width: 276px; height: 110px" title="" /></p>
  1. Open the dialog for the image.
  1. Set the width to "100" the height will be automatically set to "40".
  1. Confirm the dialog.

The height will not change.

Changed 8 years ago by Garry Yao

Attachment: 4246_6.patch added

comment:21 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:22 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:23 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [4685] and [4686].

comment:24 Changed 8 years ago by mimo

Cc: mimo@… added
Resolution: fixed
Status: closedreopened
Type: BugNew Feature
Version: 3.0 RC3.2

I can confirm that the new version fixes the align= problem. Would it be possible/difficult to add support for replacing the style= with a CSS class for more consistent design (similar to the justifyClasses and indentClasses)? Maybe call it imageAlignClasses...

comment:25 Changed 8 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: reopenedclosed
Type: New FeatureBug
Version: 3.23.0 RC

Please do no reopen tickets for feature requests.

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