Widgets: Errors thrown with elementspath (lack of integration)

[Olek Nowodziński]

1. Open samples/plugins/image2/image2.html
2. Select a widget.
3. In elementspath, click "body"
4. An error is thrown (Chrome, Firefox):

   Uncaught IndexSizeError: Index or size was negative, or greater than the allowed value. 
   

   IE9:

   SCRIPT5022: DOM Exception: INDEX_SIZE_ERR (1) 
   

Also when a widget is selected (i.e. block image2), elementspath indicates "div" as a topmost element (should be "figure"). Similarly it should be "img" instead of "span" for an inline widget. We should implement some kind of proxy to not to reveal an internal structure of widgets in elementspath.

[Jakub Ś]

Other issues where IndexSizeError is mentioned: #10858, #10411, #10893, #10972.

[Piotrek Koszuliński]

My proposal:

1. Widget wrapper is represented in elements path as widgetDefinition.elementName || widget.element.getName(). We don't need any additional code, because widget can be correctly focused from elements path when clicking the wrapper element.
2. Non-editable elements should not be displayed in elements path. Code filtering them out can go to elementspath plugin, because it'll be general (it bases on contenteditable element).
3. Nested editables are represented in elements path by their names in widgetDefinition.editables hash. In the future we can add nestedEditable.definition.elementName property.

[Marek Lewandowski]

After talking with PK we decided that IndexSizeError should be dealt in #11021.

In this ticket we will solve:

• displaying correct element name, rather than wrappers div in elementspath (suggested by Olek in 2nd part of ticket descr)
• all changes suggested by PK in ​comment5

[Marek Lewandowski]

Dev and tests pushed to t/10869 branch.

[Frederico Caldeira Knabben]

• Strangely, this branch is breaking drag and drop of inline widgets (placeholder).
• I would renamed the "privateContext" variable to, simply, "elementsPath". It'll be easier to understand the code.
• We don't need to initialize properties and variables (like "onClick: null"). I understand that this is done for good, eventually, but this is not a common practice in the project.

[Marek Lewandowski]

Fixes for this issues pushed to t/10869 dev.

[Frederico Caldeira Knabben]

This R- is more to start a discussion than to really complain about something. Let’s check if there is room to simplify things.

• As we already have “cke-display-name” to be used to customize the name to be shown in the elements-path, couldn’t we use it instead? This would simply remove the requirement of having this filter thing at all.

• Also, it would be nice for widgets to show nice names like “image” or “placholder” (localizable).

• Why the definition of editor._.elementsPath has been moved to init()? initElementsPath() was a better place for it.

Then, I’m ok with the idea of stripping out contenteditable=false elements.

[Piotrek Koszuliński]

Also, it would be nice for widgets to show nice names like “image” or “placholder” (localizable).

There's a widgetDefinition's property for that. It can be set to lang property.

As we already have “cke-display-name” to be used to customize the name to be shown in the elements-path, couldn’t we use it instead? This would simply remove the requirement of having this filter thing at all.

Unfortunately I've already forgotten if I had something deeper in mind when I asked Marek to not use that attribute. Maybe it was just that I didn't want to overuse DOM for this, but as I think about this now, we already use data attributes heavily, so why not in this case too.

[Marek Lewandowski]

As we already have “cke-display-name” to be used to customize the name to be shown in the elements-path, couldn’t we use it instead? This would simply remove the requirement of having this filter thing at all.

I've used these attributes at the very begining but we decided with PK that allows us to deal this issue in more flexible and readable way. In addition to that we already have perfect spot to add widgets hiding, or anything (which imho is highly possible in future), with very little of work.

As for me it's way to go, since:

• it has very good spot in single responsibility function
• it allows us to customize it very easily later on

it would be nice for widgets to show nice names like “image” or “placholder” (localizable)

Its now achievable using pathName property, so we can create editor.lang.image2.pathName and assign it to definition if needed, but that sounds like next ticket after merging this one.

In fact, this mistake lead to dead code in plugins/link/​plugin.js where code can not ever execute.

Why the definition of editor._.elementsPath has been moved to init()? initElementsPath() was a better place for it.

The issue was that initElementsPath() is called upon uiSpace, which is very late. Filter initialization is not good idea, because it will not be even ready for all other plugins afterInit(). Moving it into init() ensures us that it will be ready in afterInit() for dependent plugins.

[Frederico Caldeira Knabben]

Following Reinmar's comments, I still believe that using cke-display-name is a better way for it, simplifying things.

I'm sorry for the changes in the solution direction, but re-thinkings happen... better earlier, than later :D

[Marek Lewandowski]

Solution changed in t/10869 branch dev and tests.

[Marek Lewandowski]

Pushed to major dev with ​git:e4325a2de06 and major tests with 81bb7b2b87.