Opened 6 years ago

Closed 6 years ago

#11281 closed Bug (fixed)

Drag handler and mask are duplicated when reinitializing lost widget

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.3.2
Component: UI : Widgets Version: 4.3
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

  1. Open placeholder sample.
  2. Focus placeholder.
  3. Click list button multiple times.
  4. See that placeholder widget contains many drag handlers.

The reason is that when we reinitialize widget, we take the old DOM and repeat the initialization process, which creates drag handlers, masks and perhaps something else. During reinitialization we should search for those elements first and reuse them / recreate them if they already exist.

It may be related to #11112.

Change History (15)

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

Description: modified (diff)
Summary: Drag handler is duplicated when reinitializing lost widgetDrag handler and mask are duplicated when reinitializing lost widget

comment:2 Changed 6 years ago by Marek Lewandowski

Status: newconfirmed

comment:3 Changed 6 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:4 Changed 6 years ago by Marek Lewandowski

Pushed to t/11281 at dev and t/11281 at tests.

TC is within tt, since it depends on list plugin. I tried to reproduce it using purely contentDomInvalidated event, but with no luck.

Issue happens when CKEDITOR.plugins.widget.repository#checkWidgets() comes into a play.

From what i have seen, it's called on contentDomInvalidated event. It's attempting to create a widgets for new (unknown) widget wrappers (because in this instance widget was moved into list). As a result setupDragHandler is called with already inited wrapper code - creating yet another handler.

The same situation occurs with setupMask mask method, which is subject of issue #11112.

I added simplified solution, which in addition to cke_widget_new class adds cke_widget_reinit, which indicates that widget already has been inited at least once.

We may think about more sophisticated solution, storing all interface elements, and automatically creating or recovering them depending on situation.

comment:5 Changed 6 years ago by Marek Lewandowski

Status: assignedreview

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

I havent seen the patch yet, but it does not sound good. My initial idea is that if widget is reinitialised, if mask or drag handler already exist they should be reused. That's all.

Also, dt may be created for this issue. Before reinitialasing reset editable's innerHTML to break Dom references. This should be a test for checkwidgets, not contentdominvalidated.

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

Status: reviewreview_failed

I checked the code and in my opinion solution without class should be proposed as I described in comment:6. Then, we can have optimisation for new widgets and reinitialised widgets, but that requires a parameter and other modifications which are beyond this tc's scope.

comment:8 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview

Adjusted approach pushed here t/11281b. Tests for t/11281b are based on t/11281.

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

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

Status: reviewreview_failed

I rebased both branches and pushed one commit to t/11281b@dev.

R- because:

  • when old container is reused event listeners are not added,
  • as I mentioned in comment:6 a dt should be (because it may be) created for this, not tt - it works this way by design. It should be purely checkWidgets tests.

comment:10 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview_passed

Listener was in fact not added. TC was renamed to test widgets.checkWidgets draghandler re usage.

Pushed changes to t/11281b at dev and tests.

comment:11 Changed 6 years ago by Marek Lewandowski

Status: review_passedreview

Ticket status missclick, changed to review.

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

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

Status: reviewreview_failed

Drag handler reinitialization was fixed, but mask is still duplicated.

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

Owner: changed from Marek Lewandowski to Piotrek Koszuliński
Status: review_failedassigned

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

Status: assignedreview

I added a test + the same fix for mask.

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

Resolution: fixed
Status: reviewclosed

Fixed on master with git:eee34a6 on dev and 0a23f3f on tests.

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