Changes for text dragging #8065

asturur · 2022-07-17T21:55:01Z

This PR makes the first change for the text drag pr.
It removes the need to enter and exit editing the textboxes in order to make the drag effect work.
I m actually comparing right now if i introduced some regressions.
One regression for sure is there, and is the fact that if i drag/drop in the same text box the dragStart selection isn't visible.

Why this change?
EnterEdit and exitEdit do change the status of the textbox, changing their selection start and selection end, plus is an expensive operation since we create a textarea and we add it to the dom, we style it, and we manipulate it, on top of that we are making state changes to the textboxes in order to render a cursor, instead of just rendering a cursor.

The rendering needs to be slow down anyway, i see too many render loop in one second, it must be capped at 60fps OR it must be called once for each time the drop position actually change.

I plan on finishing this tomorrow.

Will follow other small tenatives to reduce the code of this feature, while maintaining all the functionalities.

defer to requestAnimation frame to render the drag drop effects
remove the boolean _isDraggingOver ( should be possible ).
reduce the event handlers to simple event handlers and call the effect explicitly instead of delegating them to firing events., avoiding that a simple text.off('drop') will eventually break the functionality.
check if there is a way to reuse current canvas pixels for
For merging the main branch, this is the only PR that is important to me, the other are small details that can follow if they require more than on day each.

Performances:
Original PR, 5 frames of dragover

this PR, 5 frames of dragover

after additional changes doesn't seem we lost or gained much

asturur · 2022-07-17T22:23:34Z

This PR solves a bug by chance.
The original PR using the actual enterEditing functionality was considering the modifier keys when dragging over, making the drop position a text selection instead of a text cursor. The drop effect would then occur on the cursor and not on the selection.

There is also another small bug in the original PR, and is not a regression ( maybe is a generic bug in fabricJS right now ), sometimes the style of the dropped text is applied uncorrectly, shifting right by one char or by as many char as the newlines contained in the selection.
Does not happen in all cases.

ShaMan123 · 2022-07-19T04:50:08Z

The original PR using the actual enterEditing functionality was considering the modifier keys when dragging over, making the drop position a text selection instead of a text cursor. The drop effect would then occur on the cursor and not on the selection.

I am not sure I understand what you mean. HTML drop selects the text that was dropped to the drop target. This PR changed it and I disagree with it.

See this gif showing regressions:

no cursor showing on selected drop target
wrong selection on drop (wrong selection if dragSource ===dropTarget, no selection elsewise) - should be changed to the dropped text
selection not showing if dragSource ===dragOverTarget (as you mentioned)

ShaMan123 · 2022-07-19T04:51:38Z

src/mixins/itext_behavior.mixin.js

      }
-      if (this.__isDraggingOver) {
+      if (this.__isDraggingOver && canDrop) {


are you sure about this?

i think this is a consequence of the previous ifs, and i made explicit.
I didn't think of it too much, could be wrong.

oh i see is not the same because this just an if and not an else if.
needs to be removed

src/mixins/itext_behavior.mixin.js

ShaMan123 · 2022-07-19T04:55:28Z

src/mixins/itext_behavior.mixin.js

-        this.renderCursorOrSelection();
+        // find cursor under the drag part.
+        var dragSelection = this.getSelectionStartFromPointer(e);
+        this.renderCursorAt(dragSelection);


If I understand correctly we should render the selection here in case dragSource===dragOverTarget

fabric.js/src/shapes/itext.class.js

Line 335 in 2db76ec

this.__isDragging && this._renderDragStartSelection(ctx);

In the pr description i wrote that:

One regression for sure is there, and is the fact that if i drag/drop in the same text box the dragStart selection isn't visible.

I know why this is broken, and is because of the double clear textarea, i was supposed to fix it yesterday but then i couldn't.

asturur · 2022-07-19T09:15:51Z

HTML drop selects the text that was dropped to the drop target. This PR changed it and I disagree with it

So HTML has only textareas that are permanently in edit state.
This concept does not apply to fabric text rendering.

I'm happy adding a feature that allow for drag/drop text, but not adding a full flow that put the developer in a rigid user experience. We implement the minimal and we let them use the events to build the experience they prefer using the drop event or drag end event (enterEdit after drop, select after drop, or just place the cursor at the end after drop)

src/shapes/itext.class.js

melchiar · 2022-07-26T02:46:35Z

src/shapes/itext.class.js

    /**
     * Renders text selection
     * @private
     * @param {{ selectionStart: number, selectionEnd: number }} selection
     * @param {Object} boundaries Object with left/top/leftOffset/topOffset
     * @param {CanvasRenderingContext2D} ctx transformed context to draw on
     */
-    _renderSelection: function (selection, boundaries, ctx) {
+    _renderSelection: function (ctx, selection, boundaries, ctx) {


Suggested change

_renderSelection: function (ctx, selection, boundaries, ctx) {

_renderSelection: function (ctx, selection, boundaries) {

melchiar · 2022-07-26T02:48:34Z

src/shapes/itext.class.js

@@ -446,6 +470,8 @@
        - charHeight * (1 - this._fontSizeFraction);

      if (this.inCompositionMode) {
+        // TODO: investigate why there isn't a return inside the if,
+        // and why can't happe top of the function
        this.renderSelection(boundaries, ctx);


need to swap these arguments

melchiar · 2022-07-26T02:57:39Z

src/shapes/itext.class.js

      ctx.restore();
    },

+    _renderCursorAt: function(selecetionStart) {


Suggested change

_renderCursorAt: function(selecetionStart) {

_renderCursorAt: function(selectionStart) {

melchiar · 2022-07-26T03:07:21Z

src/shapes/itext.class.js

@@ -434,7 +454,11 @@
     * @param {CanvasRenderingContext2D} ctx transformed context to draw on
     */
    renderCursor: function(boundaries, ctx) {


need to swap arguments here

melchiar · 2022-07-26T03:11:35Z

src/shapes/itext.class.js

      ctx.restore();
    },

+    _renderCursorAt: function(selecetionStart) {
+      var boundaries = this._getCursorBoundaries(selectionStart, true);
+      this._renderCursor(ctx, boundaries, selectionStart);


ctx is missing here

consider that yesterday i had to go to bed, i was checking where it was failing and i couldn't see this.

This will hopefully go away with rollup and sourcemaps

asturur · 2022-07-26T10:32:33Z

src/shapes/itext.class.js

@@ -462,23 +488,26 @@
     * @param {Object} boundaries Object with left/top/leftOffset/topOffset
     * @param {CanvasRenderingContext2D} ctx transformed context to draw on
     */
-    renderSelection: function (boundaries, ctx) {
+    renderSelection: function (ctx, boundaries) {


i moved those argument orders, in general, because we put ctx as first argument when we render.

Excellent
I left as is but didn't like it

src/mixins/object_interactivity.mixin.js

asturur · 2022-07-26T12:27:05Z

src/shapes/itext.class.js

-     * @private
-     */
-    __widthOfSpace: [],
-


completely unused

asturur · 2022-07-26T12:48:25Z

oh the UTs

asturur · 2022-07-26T12:53:12Z

and lint fixed

ShaMan123

I like the approach of the last commits renderDropEffect etc.
I will test behavior on browser and comment if I find regressions.

src/mixins/object_interactivity.mixin.js

ShaMan123 · 2022-07-27T14:50:02Z

src/shapes/itext.class.js

      ctx.restore();
    },

+    _renderCursorAt: function(selecetionStart) {
+      var boundaries = this._getCursorBoundaries(selectionStart, true);
+      this._renderCursor(ctx, boundaries, selectionStart);


This will hopefully go away with rollup and sourcemaps

ShaMan123 · 2022-07-27T14:50:34Z

src/shapes/itext.class.js

@@ -462,23 +488,26 @@
     * @param {Object} boundaries Object with left/top/leftOffset/topOffset
     * @param {CanvasRenderingContext2D} ctx transformed context to draw on
     */
-    renderSelection: function (boundaries, ctx) {
+    renderSelection: function (ctx, boundaries) {


Excellent
I left as is but didn't like it

ShaMan123 · 2022-07-27T14:56:17Z

src/shapes/itext.class.js

        );
      }
    },

+    renderDropTargetEffect: function(e, dontClear) {


can we rename dontClear?
preserveContextState?
or negate it to clearContext?

Actually it is ugly IMO and should be removed. MonkeyPatch written all over it.
The caller _renderDragEffects should clear context if needed and prepare it so renderDropTargetEffect receives a ctx in a state that is transformed to the object's plane (like all rendering functions across fabric). And modify this function accordingly

i tried it but didn't work, there was a case in which i was killing the source effect anyway.

What we can do is trying to transform ctx here, and pass it along and make that the effects receive a ctx.

move clearContextTop from text to object

clearContextTop for source, for target,
then draw them in sequence.

This will mean that we change the signature of the functions source and target to accept e and ctx.
is that ok?

sounds better IMO

src/mixins/itext_behavior.mixin.js

ShaMan123 · 2022-07-27T15:48:18Z

There's a small regression on dragstart. It clears the selection but should not. Minor and can be revisited

asturur · 2022-07-28T22:18:07Z

src/mixins/object_interactivity.mixin.js

+     * @param {Boolean} [restoreManually] true to don't restore the context after clear, in order to draw something else.
+     * @returns {CanvasRenderingContext2D|undefined} canvas.contextTop transformed on the object center
+     */
+    clearContextTop: function(restoreManually) {


this for me solves what i didn't like of the ctx = this.prepareContextTop() and also streamline a bit the code that was divied in 3 methods

i know that restoreManually seems a bit like dontClear but isn't exactly as that.

Ok
It is better
The callback approach sounds good
Let's merge

Maybe make jsdoc a bit better?

some function hasn't be jsdoced, i know. but i really need to stop. Let me open an issue to come back to those things.

ShaMan123

Some minor stuff

ShaMan123 · 2022-07-28T22:29:11Z

src/mixins/itext_behavior.mixin.js

@@ -172,7 +172,7 @@

      //  make sure we clear context even if instance is not editing
      if (shouldClear) {
-        var ctx = this._clearContextTop();
+        var ctx = this.clearContextTop(true);


Ctx is restored on the next line so can we let the method restore it?

because i did a blind search/replace 😄
correcting this

ShaMan123 · 2022-07-28T22:29:23Z

src/mixins/itext_behavior.mixin.js

@@ -605,7 +605,7 @@
            this._updateTextarea();
          }
          else {
-            var ctx = this._clearContextTop();
+            var ctx = this.clearContextTop(true);


graphics-et-al

ShaMan123 · 2022-07-28T22:30:18Z

src/mixins/object_interactivity.mixin.js

+     * This function is used to clear pieces of contextTop where we render ephemeral effects
+     * Example: blinking cursror text selection.
+     * // TODO: discuss swapping restoreManually with a renderCallback, but think of async issues
+     * @param {Boolean} [restoreManually] true to don't restore the context after clear, in order to draw something else.


Remove 'to'

ShaMan123 · 2022-07-28T22:31:13Z

src/mixins/object_interactivity.mixin.js

@@ -279,6 +279,32 @@
      return this;
    },

+    /**
+     * Clears the canvas.contextTop on top of the object.


ShaMan123 · 2022-07-28T22:31:35Z

src/mixins/object_interactivity.mixin.js

+     * Example: blinking cursror text selection.
+     * // TODO: discuss swapping restoreManually with a renderCallback, but think of async issues
+     * @param {Boolean} [restoreManually] true to don't restore the context after clear, in order to draw something else.
+     * @returns {CanvasRenderingContext2D|undefined} canvas.contextTop transformed on the object center


On object center is weird english

src/mixins/object_interactivity.mixin.js

ShaMan123 · 2022-07-28T23:00:46Z

src/mixins/object_interactivity.mixin.js

-     * @returns {CanvasRenderingContext2D|undefined} canvas.contextTop transformed on the object center
+     * @param {Boolean} [restoreManually] When true won't restore the context after clear, in order to draw something else.
+     * @return {CanvasRenderingContext2D|undefined} canvas.contextTop that is either still transformed
+     * with the object transformMatrix, or restord to neutral transform
     */
    clearContextTop: function(restoreManually) {


I can live with this though I think it is better to have the arg be restore instead of dontRestore
I will create a branch of that for a quick merge

we can have restore = true as soon as we use es6 syntax

src/mixins/object_interactivity.mixin.js

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

asturur · 2022-07-28T23:09:32Z

follow up #8095

asturur added 4 commits July 17, 2022 11:50

ok

9c1e483

some changes

e956204

corrected comment

c246f71

soften comment

54af366

asturur requested a review from ShaMan123 July 18, 2022 07:06

ShaMan123 reviewed Jul 19, 2022

View reviewed changes

asturur added 3 commits July 25, 2022 22:41

reintroduced html behaviour

21c1ceb

replace function name

b3a29ba

sofar-sogood

29135ab

melchiar reviewed Jul 26, 2022

View reviewed changes

src/shapes/itext.class.js Outdated Show resolved Hide resolved

melchiar reviewed Jul 26, 2022

View reviewed changes

i m just making mess

463489e

asturur commented Jul 26, 2022

View reviewed changes

asturur added 3 commits July 26, 2022 12:44

moved a lot, didnt fix anything

bd6b450

moved a lot, didnt fix anything, no dist

0afc5e5

ok works

147b848

asturur commented Jul 26, 2022

View reviewed changes

src/mixins/object_interactivity.mixin.js Show resolved Hide resolved

asturur commented Jul 26, 2022

View reviewed changes

src/shapes/itext.class.js

* @private

*/

__widthOfSpace: [],

Copy link

Member Author

asturur Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completely unused

lintfixes

93b13dd

ShaMan123 mentioned this pull request Jul 27, 2022

contributing to upstream yassilah/fabric-extensions#5

Open

ShaMan123 reviewed Jul 27, 2022

View reviewed changes

streamlined

df74504

asturur commented Jul 28, 2022

View reviewed changes

ShaMan123 approved these changes Jul 28, 2022

View reviewed changes

comments

814cde9

ShaMan123 reviewed Jul 28, 2022

View reviewed changes

src/mixins/object_interactivity.mixin.js Outdated Show resolved Hide resolved

asturur and others added 2 commits July 29, 2022 01:08

Update src/mixins/object_interactivity.mixin.js

14a13d8

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Update src/mixins/object_interactivity.mixin.js

e4abb1c

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

asturur merged commit f00e547 into draggable-text Jul 28, 2022

asturur deleted the draggable-text-render-cursor branch September 11, 2022 23:03

	_renderSelection: function (ctx, selection, boundaries, ctx) {
	_renderSelection: function (ctx, selection, boundaries) {

	_renderCursorAt: function(selecetionStart) {
	_renderCursorAt: function(selectionStart) {

Changes for text dragging #8065

Changes for text dragging #8065

Conversation

asturur commented Jul 17, 2022 • edited Loading

asturur commented Jul 17, 2022

ShaMan123 commented Jul 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jul 19, 2022

Choose a reason for hiding this comment

melchiar Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jul 26, 2022

asturur commented Jul 26, 2022

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jul 28, 2022

asturur commented Jul 17, 2022 •

edited

Loading

melchiar Jul 26, 2022 •

edited

Loading

ShaMan123 commented Jul 27, 2022 •

edited

Loading