Opened 6 years ago

Closed 6 years ago

#10869 closed Bug (fixed)

Widgets: Errors thrown with elementspath (lack of integration)

Reported by: Olek Nowodziński Owned by: Marek Lewandowski
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.3
Component: General Version: 4.3 Beta
Keywords: Cc:

Description

  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.

Change History (18)

comment:1 Changed 6 years ago by Jakub Ś

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

Last edited 6 years ago by Jakub Ś (previous) (diff)

comment:2 Changed 6 years ago by Piotrek Koszuliński

Priority: NormalHigh

comment:3 Changed 6 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:4 Changed 6 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:5 Changed 6 years ago by 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. We'll need additional code handling clicks on nested editables in elements path, because we need to select their contents, not them.
Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:6 Changed 6 years ago by Marek Lewandowski

Owner: changed from Piotrek Koszuliński to Marek Lewandowski

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

comment:8 Changed 6 years ago by Marek Lewandowski

Status: assignedreview

Dev and tests pushed to t/10869 branch.

comment:9 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed
  • 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.

comment:10 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview

Fixes for this issues pushed to t/10869 dev.

comment:11 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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.

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

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

Last edited 6 years ago by Marek Lewandowski (previous) (diff)

comment:14 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview

comment:15 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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

comment:16 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview

Solution changed in t/10869 branch dev and tests.

comment:17 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:18 Changed 6 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

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

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