Opened 11 years ago
Closed 11 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 ¶
- Open samples/plugins/image2/image2.html
- Select a widget.
- In elementspath, click "body"
- 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:2 Changed 11 years ago by
Priority: | Normal → High |
---|
comment:3 Changed 11 years ago by
Status: | new → confirmed |
---|
comment:4 Changed 11 years ago by
Owner: | set to Piotrek Koszuliński |
---|---|
Status: | confirmed → assigned |
comment:5 Changed 11 years ago by
My proposal:
- 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. - 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).
- Nested editables are represented in elements path by their names in
widgetDefinition.editables
hash. In the future we can addnestedEditable.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.
comment:6 Changed 11 years ago by
Owner: | changed from Piotrek Koszuliński to Marek Lewandowski |
---|
comment:7 Changed 11 years ago by
comment:9 Changed 11 years ago by
Status: | review → review_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 11 years ago by
Status: | review_failed → review |
---|
Fixes for this issues pushed to t/10869 dev.
comment:11 Changed 11 years ago by
Status: | review → review_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 11 years ago by
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 11 years ago by
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.
comment:14 Changed 11 years ago by
Status: | review_failed → review |
---|
comment:15 Changed 11 years ago by
Status: | review → review_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 11 years ago by
Status: | review_failed → review |
---|
Solution changed in t/10869 branch dev and tests.
comment:17 Changed 11 years ago by
Status: | review → review_passed |
---|
comment:18 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Pushed to major dev with git:e4325a2de06 and major tests with 81bb7b2b87.
Other issues where IndexSizeError is mentioned: #10869, #10411.