Opened 7 years ago

Closed 7 years ago

#6334 closed Bug (fixed)

Use data-* attributes for custom attributes

Reported by: Sa'ar Zac Elias Owned by: Sa'ar Zac Elias
Priority: Normal Milestone: CKEditor 3.5
Component: General Version:
Keywords: Cc:

Description (last modified by Garry Yao)

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.

Attachments (3)

6334.patch (44.5 KB) - added by Sa'ar Zac Elias 7 years ago.
6334_2.patch (45.9 KB) - added by Sa'ar Zac Elias 7 years ago.
6334_3.patch (46.0 KB) - added by Sa'ar Zac Elias 7 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by Sa'ar Zac Elias

Status: newassigned

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6334.patch added

comment:2 Changed 7 years ago by Sa'ar Zac Elias

Milestone: CKEditor 3.5
Status: assignedreview

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

comment:3 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

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.

comment:4 Changed 7 years ago by Garry Yao

BTW which exactly Firefox version could we reveal the bug?

comment:5 Changed 7 years ago by Sa'ar Zac Elias

Keywords: Discussion added

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

comment:6 Changed 7 years ago by 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.

comment:7 Changed 7 years ago by 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.

comment:8 Changed 7 years ago by Garry Yao

Description: modified (diff)

comment:9 Changed 7 years ago by 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.

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6334_2.patch added

comment:10 Changed 7 years ago by Sa'ar Zac Elias

Status: review_failedreview

comment:11 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

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

comment:12 Changed 7 years ago by 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?

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6334_3.patch added

comment:13 in reply to:  11 Changed 7 years ago by Sa'ar Zac Elias

Status: review_failedreview

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.

comment:14 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

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.

comment:15 Changed 7 years ago by Sa'ar Zac Elias

Status: review_failedreview

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.

comment:16 Changed 7 years ago by 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?

comment:17 Changed 7 years ago by 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).

comment:18 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:19 Changed 7 years ago by Sa'ar Zac Elias

Keywords: Discussion removed
Resolution: fixed
Status: review_passedclosed

Fixed with [6111].

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