Use data-* attributes for custom attributes

[Sa'ar Zac Elias]

As we're walking towards HTML5 support, and because of FF's ​latest problems with custom attributes (which, as seems, won't be fixed very soon), we should start using the data-* attributes for custom attributes.

[Sa'ar Zac Elias]

For FF, it should be tested with the nightly build for now (until the next release), so the data-* attributes would be recognized as "safe".

[Garry Yao]

The patch is not good in the sense of mixing the job of 'setCustomData', this FF dedicated problem should be fixed instead silently inside element::set(get)Attribute once a '_cke' prefix is seen, then the change would be also much smaller.

[Garry Yao]

BTW which exactly Firefox version could we reveal the bug?

[Sa'ar Zac Elias]

Me and Fred have talked about it before and decided to change the element::[get/set/remove]CutsomData methods. The point of this "transfer" is not only to resolve the FF bug (starting version 3.6.9, partially fixed for 3.6.11 AFAIK), but also to use the new HTML5 addition, whose definition matches our purpose. Thus the goal is to use these attributes also for browsers which currently do accept our approach. Opinions?
Anyway, let's hold the full review of this ticket to include new incoming additions to the code (e.g. the placeholder plugin) and old incomings (e.g. iframe plugin).

[Matti Järvinen]

Just that you know, FF bug: Element Attributes dropped in DesignMode/ContentEditable sections is listed as fixed for FF3.6.11 ​https://bugzilla.mozilla.org/show_bug.cgi?id=596300

Didn't bother to read through whether it is partial or full fix since I'm fine with data-attributes and I believe that is the way CKEditor should take.

[Alfonso Martínez de Lizarrondo]

Although this part of the change seems simple, we must be careful about storing new data using attributes in the DOM nodes instead of the customData object because that means extra work for the browser, it's not just setting the attribute/property on a different element, DOM nodes have different rules, so the browser does a lot of work to access them and it can mean worse performance. I think that the browser that will notice it the most are older Firefoxes.

I would vote for just changing the existing _cke to data-cke and after that, test if the performance is affected or not by extending it to the customData functions.

Side note: to improve a little the code compression it should be possible to define a variable to store the "data-cke-expando" string and use that variable instead of the full string when it's used several times in a function or object.

[Garry Yao]

The point of this "transfer" is not only to resolve the FF bug (starting version 3.6.9, partially fixed for 3.6.11 AFAIK), but also to use the new HTML5 addition, whose definition matches our purpose.

It's wrong to make our element::setCustomData identical with HMTL5 data attributes, the key difference between two is that the former is just a normal property of the DOM object (expando) that doesn't survive over element clone or clipboard, it was designed for making memory-leak-free event listener and marking elements etc.

Didn't bother to read through whether it is partial or full fix

It is not fixed at all! I'm sitting right on 3.6.12

I would vote for just changing the existing _cke to data-cke,and after that...

I totally agree the first part, but considering backward compatibility, the best way of delivering it would be the approach of #3.

[Garry Yao]

I don't think the changes in _source/core/dom/domobject.js are necessary?

[Garry Yao]

Besides it looks inconsistent when saying element.data('cke-bookmark') and element.removeAttribute('data-cke-bookmark') together, how do you like the idea of intercepting (get/set/remove)Attribute?

[Sa'ar Zac Elias]

Replying to garry.yao:

I don't think the changes in _source/core/dom/domobject.js are necessary?

The goal is to switch all of our custom attirubtes to use the data-* attributes, and also this is done to unify the attributes.

Besides it looks inconsistent when saying element.data('cke-bookmark') and element.removeAttribute('data-cke-bookmark') together

The data method now removes the attribute if the value === false.

[Garry Yao]

The goal is to switch all of our custom attirubtes to use the data-* attributes, and also this is done to unify the attributes.

It seems that I failed of explaining you the difference Saar, an element property (normally we called expando) has nothing to do with attribute, it's just safe to keep it retain.

The data method now removes the attribute if the value === false.

What's the problem you see with intercepting (get/set/remove)Attribute? Not sure about your design (the value to set could easily be a "false"), if it's jQuery you're referring to, they use safely a ​removeData.

[Sa'ar Zac Elias]

The expando attribute is sometimes being injected into the source, so this data-* convention must be ported to the expando.
I proposed the data() method, which saves us code size and unifies the code as a minor but proper addition to the API. The value should always be a string there, therefore boolean check is valid.

[Garry Yao]

The expando attribute is sometimes being injected into the source, so this data-* convention must be ported to the expando.

Perhaps I missed a test case as evidence?

[Sa'ar Zac Elias]

​http://dev.ckeditor.com/browser/CKEditor/trunk/_source/plugins/undo/plugin.js#L155
We need to unify the expando like it's an attribute, also to make sure it is not accidentally slipping out to the output (the htmlFilter will now only remove data-cke- attributes, and not _cke).

[Sa'ar Zac Elias]

Fixed with [6111].