[Umbrella] Widget drag handle improvements

[Piotrek Koszuliński]

1. Performance: #11001.
2. Repositioning: #11161.
3. Initial position: #11176.

[Piotrek Koszuliński]

Postponing entire set of tickets.

[Marek Lewandowski]

Along with PK we decided to change drag handler position calculation time to mouseover (instead doing it right at widget instance init time). Since mouseover event is called very rarely (and suprisingly it's pretty well handled across our supported browsers), there is no need to cache results. At the same time it will fix our issues with wrong position after widget repositioning.

Solution commited to ​t/11177 at dev and t/11177 at tests.

[Marek Lewandowski]

Since we had small chat about changing listening object, from editable, to each widgets wrapper.

The thing about event delegation are:

• it does scale up better (but yeah... we'll not witness documents with 40+ widgets per page)
• it's simpler
• does listen for all elements of widget, even these which will be added dynamically to DOM after widget creation
• less listeners count

drawbacks:

• at the time being it involves extra computing on each event (working with widgetRepo#getByElement which is not cheap)

I see no problem with either solution, adding a listeners to each wrapper, or listening only on editable.

[Piotrek Koszuliński]

The alternative for single listener on editable is to have listeners on every widget wrapper. This version requires more code because those listeners have to be removed when we destroy widget instances. However, it does not need getByElement calls at all. Additionally, it doesn't have to consume more memory if listeners will be added with simple:

wrapper.on( 'mouseover', wrapper.repositionDragHandler, wrapper );

Because there will not be additional closures created.

So after few hours since our last chat I changed my mind and agree that this solution will be better :).

[Marek Lewandowski]

Ok, so updated solution pushed to ​t/11177b at dev and t/11177b at tests.

(note tests are skipped for IE8, since other drag-related tests are also disabled)

[Piotrek Koszuliński]

Tests should work in IE8 too. Some of them are disabled because DnD of inline widgets is disabled. But drag handler is displayed for block widgets and can be tested on them.

[Marek Lewandowski]

Correct, with block widgets we are able to test it with IE8, tests update pushed.

[Piotrek Koszuliński]

1. positionDragHandler is executed on every mouseover caught on widget wrapper, so also for every wrapper's ancestor. It should be executed only on wrapper.
2. @context Widget is not in accordance with the rest of code. Lowercased "widget" is used there.
3. I think that positionDragHandler should become public widget's method. A better name for it would be updateDragHandlerPosition.

[Piotrek Koszuliński]

Additionally:

1. Assertion names in 'test handler - positioning' are incorrect.
2. It should be mentioned that 'test handler - init position' is a regression test, because if it weren't for this ticket it would be unnecessary.
3. I've got no idea how it works, but it looks as wait() was executed after resume() in 'test handler - positioning'. In this case test should fail, but it does not what makes YUI Test even more incomprehensible. Anyway - we don't need wait and resume at all, because this is synchronous test.

[Marek Lewandowski]

1. Actually recalculating position upon children mouseover is done intentionally, consider following scenario (lets call it TC1):

• open image2 sample
• place mouse cursor in middle of image (without clicking)
• press F5 to refresh page
• in case when we listen to child mouseovers, most of the times handler is being repositioned instantly, if we will reduce listening only to wrapper, it will not reposition itself until we will hover wrapper explicitly
  however some browsers still may not fire mouseover event in above situation

2. but constructor function is called Widget (uppercased) imo we should adjust other occurrences to upercased name
3. done

second part:

1. done
2. done
3. done

Pushed 2 branches

• minimalistic solution - ​t/11177b - have an issue with TC1, it will show drag handler in invalid position (a little bit away from bottom left widget point) and it looks really bad
• extra mouseleave solution - ​t/11177c - it handles additionally showing/hiding drag handler by js rather than css. That way even if we have no mouseenter event, handler will not be displayed (therefore most likely our user will move mouse of the widget and back, and then it will be fine)

[Piotrek Koszuliński]

None of the solutions is good enough.

• Missing undo manager integration - before changing handler's position we have to lock snapshot. It should be done only if drag handler position will be modified, because it's not a lightweight operation.
• CSS-based hover handling should be used instead of JS-based solution (t/11177c).
• TC1 should be handled by hiding drag handler before first repositioning.

So the perfect solution will be a combination of b + c + new idea. We can hide handlers initially by setting the left to -9999px. This way we don't need any additional CSS property and we can use CSS-based hover.

Additionally, we should limit repositioning to mouseenter only. Mouseover is fired too often. For browsers which don't support mouseenter we need a solution with mouseover and target check. We can consider putting cross-browser mouseenter implementation in dom.element.

[Marek Lewandowski]

New solution propsed in ​t/11177d

it includes

• drag handler reposition method become public member
• position is recalculated only at wrapper mouseenter event
• last call offset caching
• snapshot locking

mouseenter is supported pretty well among our supported browser IE8 - IE11, FF, Chrome

only one concern is that for chrome:
While dragging chrome tends to spam wrapper mouseenter. Do we want to fix also this issue or is it not that important?

[Piotrek Koszuliński]

Please rebase branch on current master and update tests.

[Marek Lewandowski]

Rebased ​t/11177d dev and t/11177d tests.

[Piotrek Koszuliński]

R- because of:

1. No comment for what 'left:-9999px;' + does and why.
2. No test for updateDragHandlerPosition. Tests should check if saveSnapshot&&unlockSnapshot is fired if position changed and if correctly nothing happens if position is not changed.

Also, tests has to be rebased again and checked on IE8.

[Marek Lewandowski]

Ok i've rebased both branches, and added requested fixes. Also i had to change object comparing method within updateDragHandlerPosition, since test shown that it was not working as expected.

[Piotrek Koszuliński]

I rebased both branches and pushed few commits to both of them.

The solution works very well. It definitely solves all three issues mentioned in ticket description.

However, there are cases in which handler should be repositioned and isn't. E.g.:

• when changing image2 alignment from right to center without triggering new mouseover,
• when resizing image2.

All these cases trigger widget#setData, but we don't want to use it from the beginning (#11001). Instead we can add it after 100ms. Apparently we need both solutions combined together.

[Piotrek Koszuliński]

Back on review - additionally reposition drag handler on widget#data. Listener is added after a short timeout (50ms).

[Marek Lewandowski]

Good note, althought current solution still might be tuned.

setTimeout( function() {
	//widget.on( 'data', widget.updateDragHandlerPosition, widget );
}, 50 );

there are 2 issues with that:

• it's attached only when drag container is not present, in case when it's already available and reused (as a result of #11281), se TC1
• i guess that instead of relying on settimeout we can move it to more appropriate place (i'm not sure now, perhaps sth like widget#ready event?)

as of tests 52cbfc185db56cd3a0d9661758be9b180b98e7d8, i don't think that change in dt/plugins/widget/dnd.html line 89 was necessary (test handler - positioning TC update)

---

TC1

• open ​Image plugin sample
• englare first image ( so it is a little wider than half of an editor)
• select resized image widget
• click bullet list, ordered list, bullet list, ordered list
• click once more ordered list (to remove list wrapper)
• move mouse back to middle of image, and double click
• select center alignment, and click ok

Image resizer will not be repositioned

[Marek Lewandowski]

k after updates it works as expected

[Piotrek Koszuliński]

Fixed on master with ​git:32e096f on dev and 86b76f7 on tests.