Protect style attribute

[Alfonso Martínez de Lizarrondo]

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.

[Garry Yao]

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"?

[Alfonso Martínez de Lizarrondo]

basic idea

[Alfonso Martínez de Lizarrondo]

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

<p>
	This is <span style="color:#ff0000; background:#FFFFFF;">some </span><strong>sample text</strong>. You are using <a href="http://ckeditor.com/">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

[Garry Yao]

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

[Alfonso Martínez de Lizarrondo]

Proposed patch

[Alfonso Martínez de Lizarrondo]

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.

[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.

[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.

[Wiktor Walc]

#2810 was marked as duplicate

[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.

[Alfonso Martínez de Lizarrondo]

I've submitted ​https://connect.microsoft.com/IE/feedback/details/684058/style-attribute-can-be-modified-by-the-browser to Microsoft. The testcase ​http://www.martinezdelizarrondo.com/bugs/style.html 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.

[Frederico Caldeira Knabben]

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.

[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.

[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).

[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 is the problem but the lack of units (which is also invalid by the way). If you change the styles to:

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

IE will leave the attribute untouched.