Opened 6 years ago

Closed 6 years ago

#6334 closed Bug (fixed)

Use data-* attributes for custom attributes

Reported by: Saare Owned by: Saare
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 Saare 6 years ago.
6334_2.patch (45.9 KB) - added by Saare 6 years ago.
6334_3.patch (46.0 KB) - added by Saare 6 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 6 years ago by Saare

  • Status changed from new to assigned

Changed 6 years ago by Saare

comment:2 Changed 6 years ago by Saare

  • Milestone set to CKEditor 3.5
  • Status changed from assigned to review

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 6 years ago by garry.yao

  • Status changed from review to review_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 6 years ago by garry.yao

BTW which exactly Firefox version could we reveal the bug?

comment:5 Changed 6 years ago by Saare

  • 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 6 years ago by matti

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 6 years ago by alfonsoml

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 6 years ago by garry.yao

  • Description modified (diff)

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

comment:10 Changed 6 years ago by Saare

  • Status changed from review_failed to review

comment:11 follow-up: Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

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

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

comment:13 in reply to: ↑ 11 Changed 6 years ago by Saare

  • Status changed from review_failed to review

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 6 years ago by garry.yao

  • Status changed from review to review_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 6 years ago by Saare

  • Status changed from review_failed to review

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 6 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 6 years ago by Saare

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 6 years ago by garry.yao

  • Status changed from review to review_passed

comment:19 Changed 6 years ago by Saare

  • Keywords Discussion removed
  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [6111].

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