Opened 9 years ago

Last modified 7 years ago

#5528 assigned New Feature

Protect style attribute

Reported by: Alfonso Martínez de Lizarrondo Owned by: Alfonso Martínez de Lizarrondo
Priority: Normal Milestone:
Component: Core : Output Data Version:
Keywords: Discussion Cc:


Browsers parse the contents of the style attribute and do strange things with its contents, so we should protect like we do for href and src so it's modified only when the user request it, and not by the browser.

Attachments (2)

5528.patch (1.6 KB) - added by Alfonso Martínez de Lizarrondo 9 years ago.
basic idea
5528.2.patch (9.6 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Proposed patch

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by Garry Yao

Keywords: Discussion added

I believe, but providing an piece of reproducing sample will also helping convincing others.
Protecting it would make line styles manipulation even complicated, how about adding a parsing and formatting plugin for inline styles, just like "htmldataprocessor"?

Changed 9 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5528.patch added

basic idea

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

The patch is just a quick test of the most basic idea, with that patch loading a content like

	This is <span style="color:#ff0000; background:#FFFFFF;">some </span><strong>sample text</strong>. You are using <a href="">CKEditor</a>.</p>

is correctly preserved when switching from code to design and back to code view.

The next step would be to adjust the element.setStyle and element.removeStyle methods so they update the _cke_saved_style attribute (in the first step we can set it to the current contents of style and we get the current behavior). If there are other functions used to change the styles of an element they will have to be reviewed as well.

And finally a special plugin could be added like you have described that could parse a css string and create an object with those values and the calls to change the style like setStyle are done using that object that it's serialized without using the browser parser for css. For this plugin we could look at a project like JSCSSP

comment:3 Changed 9 years ago by Garry Yao

Milestone: CKEditor 3.4

Back to the very beginning, we need to understand what exactly has been touched by the browser, beside color values and font family.

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 5528.2.patch added

Proposed patch

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

Owner: set to Alfonso Martínez de Lizarrondo
Status: newreview

This patch implements the proposed idea.

It includes also the patch from #5930 and #1653, but I've removed the line introduced in #6403 because it doesn't seem to fail according to the provided steps and it doesn't look a good idea to remove quotes from the style attributes if they existed.

The logic is as follows:
When content is loaded, the current style is stored in _cke_saved_style.
If element.setStyle() is called, that property is reset to signal that we shouldn't rely on it any longer (a second ticket can be created afterwards to provide intelligent merging, taking into account shorthands).
When serializing back to data format, we check if the _cke_saved_style exists, if it's missing, the we proceed to format the current style as suggested in #1653 so the output is packed and trying to provide uniformity across browsers.

If the _cke_saved_style exists, then we must take into account that the user might have resized the element or moved if it's absolutely positioned, so we take those values from the current style and replace just those in the stored style. In IE we could use the onresizeend event to update the style only as needed, but I'm not sure if something like that is available for moving elements (maybe some drag and drop event?), but anyway I had to code it for Firefox, so that way we know that every browser will behave in the same way.

As long as the element isn't edited in a dialog (that will call element.setStyle), anything in the style should remain untouched, and when it's modified, it tries to provide uniform output across browsers.

comment:5 Changed 8 years ago by Garry Yao

First the suggested style cache approach looks interesting, while it contains a wrong consumption for me by saying that all styles change are done through element::setStyles, especially for plugins that are not written by us.

And it looks like also in this patch there's still no solution for dealing with css short-hand problems like the background style.

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

It's quite easy to tell people to switch to use the API if they have problems while setting styles on the element, and that will make their plugins work with current, past and future versions of CKEditor.

Shorthands shouldn't be a problem with the patch. This patch doesn't try to be too smart, just store the original style and return it if there are no changes. If the API sets some style with shorthand or full properties it "removes itself" and continues with the previous behavior.

A future ticket can improve element::setStyle so it tries to be intelligent and update itself (without relying on the browser) the stored value with smart merging, but that part isn't needed to bring this first part of the feature that solves some of the current problems.

comment:7 Changed 8 years ago by Wiktor Walc

#2810 was marked as duplicate

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

Currently it seems that IE is the only browser that still modifies the contents of the attribute.

I've done some basic tests and all the current versions of Firefox, Chrome, Opera and Safari and all of the respect exactly the contents of the style attribute. IE9 does all the strange things and the current preview of IE10 doesn't work with the samples, so I don't know if they have done any change about this issue.

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

I've submitted to Microsoft. The testcase shows that other browsers maintain the style attribute just as it's set, but IE modifies several ones, although since IE9 it's much better and it doesn't require as much adjusting as the previous versions.

comment:10 Changed 8 years ago by Frederico Caldeira Knabben

Status: reviewassigned

Gentlemen, if you don't mind, I would prefer waiting a bit regarding this one to understand both how crucial this is in the real world, as well as waiting for feedback from MS.

@alfonsoml, please correct me if you understand that this is a "must have" thing.

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

The fact that only IE changes the output now it's great as it can simplify the code or skip it for non-IE browsers.

Is it a "must have"?, we'll so far it hasn't been added and the earth is still spinning...
More seriously: it's hard to say what's a must have, older versions of the browsers used to be quite nasty, in fact old versions of IE are very nasty and that's why there's code trying to normalize the output.

The thing that I would check is that the output generated by IE9 and the rest of browsers is compatible, so that editing in one or another doesn't generate little changes that might be not-nice for diff tools (marking something as changed just because a semicolon in a style is such example), so you can try to review that the statements made in older bugs about the uniform output are valid, when I created this patch it wasn't right.

Also, keep in mind that if MS decides to fix the issue it will be IE10 only; IE8 will remain forever broken.

comment:12 Changed 8 years ago by Frederico Caldeira Knabben

I would be happier to have it in a dedicated plugin ("Normalize Output Plugin") in the future, keeping the core simpler (if we could ever call it simple).

comment:13 Changed 7 years ago by Jakub Ś

Here is an example reported on our support channel.

<p style="margin-left: 5; margin-top: 2; margin-bottom: 2">...</p>

IE8 and IE9 simply remove the whole style attribute while other browsers leave it untouched.

It seems that not invalid property margin-left: 5 is the problem but the lack of units. If you change the styles to:

<p style="margin-left: 5px; margin-top: 2px; margin-bottom: 2px">...</p>

IE will leave the attribute untouched.

Version 0, edited 7 years ago by Jakub Ś (next)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy