fix(): Draggable Text Migration Regression #8534

ShaMan123 · 2022-12-22T09:30:29Z

Motivation

Description

Draggable text was broken in #8013 (db8739a) and #8421 (due to ts-nocheck)

Changes

Gist

I have noticed that undoing a text drop isn't supported natively and I wonder why. Would love your feedback.
The textarea value is changed programmatically on drop. Is there a way to do that and have the change enter the native history stack?

In Action

My screen recorder doesn't record the drag image... so it is pointless

d69e3dd of #8013

github-actions · 2022-12-22T09:32:09Z

Build Stats

file / KB (diff)	bundled	minified
fabric	937.616 (+2.357)	295.783 (+0.842)

github-actions · 2022-12-22T09:33:49Z

Coverage after merging fix-text-dnd into master will be

83.19%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.ts	100%	100%	100%	100%
src
cache.ts	96.97%	90%	100%	100%	56
config.ts	77.27%	66.67%	66.67%	84.62%	130, 138–140, 151–153
constants.ts	100%	100%	100%	100%
env.ts	72.73%	53.33%	100%	79.17%	22, 22–23, 23, 23, 25, 25, 27, 29, 31–32, 62
intersection.class.ts	100%	100%	100%	100%
pattern.class.ts	92.19%	85.71%	100%	96.30%	118, 124, 135, 144, 96
point.class.ts	100%	100%	100%	100%
shadow.class.ts	98.48%	96%	100%	100%	156
typedefs.ts	100%	100%	100%	100%
src/brushes
base_brush.class.ts	100%	100%	100%	100%
circle_brush.class.ts	0%	0%	0%	0%	100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 134, 141, 143, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
pattern_brush.class.ts	97.06%	87.50%	100%	100%	21
pencil_brush.class.ts	91.81%	85.42%	100%	93.52%	122–123, 152, 152–154, 276, 280, 285–286, 68–69, 84–85
spray_brush.class.ts	0%	0%	0%	0%	100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 89–91, 99
src/canvas
TextEditingManager.ts	100%	100%	100%	100%
canvas.class.ts	93.67%	90.35%	94%	96.05%	1137, 1137–1138, 1141, 1161, 1161, 1220, 1270–1271, 1292, 1300, 1413–1414, 1416–1417, 1437–1438, 556–557, 562, 572, 705–706, 708–709, 709, 709, 755–756, 817–818, 871–873, 903, 908–909, 938–939
canvas_events.ts	78.83%	76.63%	83.33%	79.69%	1012–1013, 1013, 1013–1015, 1017–1018, 1018, 1018, 1020, 1028, 1028, 1028–1030, 1030, 1030, 1036–1037, 1045–1046, 1046, 1046–1047, 1052, 1054, 1085–1087, 1090–1091, 1095–1096, 1213–1215, 1218–1219, 1292, 1412, 1506–1507, 1513, 1517–1518, 1534, 1556, 1603, 1608, 1656, 175, 200, 309–310, 313–317, 322, 345–346, 351–356, 376, 376, 376–377, 377, 377–378, 386, 391–392, 392, 392–393, 395, 404, 410–411, 411, 411, 454, 462, 466, 466, 466, 466, 466–467, 469, 551–552, 552, 552–553, 559, 559, 559–561, 581, 583, 583, 583–584, 584, 584, 587, 587, 587–588, 591, 600–601, 603–604, 606, 606–607, 609–610, 622–623, 623, 623–624, 626–630, 636, 643, 683, 683, 683, 685, 687–691, 697, 703, 703, 703–704, 706, 709, 714, 727, 754, 811–812, 812, 812–813, 815, 818–819, 819, 819–820, 822–823, 826, 826–828, 831–832, 902, 914, 921, 942, 974, 995–996
static_canvas.class.ts	95.18%	90.66%	97.92%	97.24%	1089–1090, 1090, 1090–1091, 1211, 1221, 1275–1276, 1279, 1314–1315, 1393, 1402, 1407, 1456–1457, 1685, 1685–1686, 1735, 1738, 1741, 1741, 1741, 1744, 1747, 1747, 1747, 286–287, 384–385, 387–388, 849
src/color
color.class.ts	92.16%	86.49%	100%	94.29%	330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
changeWidth.ts	100%	100%	100%	100%
control.class.ts	93.90%	88.89%	90.91%	97.73%	235, 319, 319, 354
controls.render.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
default_controls.ts	86.67%	66.67%	100%	100%	122, 129
drag.ts	100%	100%	100%	100%
polyControl.ts	6.35%	0%	0%	11.43%	100, 105, 119, 121–124, 124, 127, 134, 17, 25–28, 30, 30, 30, 30, 30, 30, 30, 30, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	20%	12.50%	50%	22.22%	45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.41%	92.68%	100%	93.59%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	130–131, 162–163, 170, 176, 178
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/filters
2d_backend.class.ts	92%	83.33%	100%	93.75%	35–36
FilterBackend.ts	88.89%	88.89%	100%	85.71%	15–16
WebGLProbe.ts	37.14%	40%	60%	30%	28–30, 30, 30–31, 33–35, 43, 46–48, 48, 48–51, 53, 58
base_filter.class.ts	20.83%	20.83%	33.33%	18.18%	102–104, 104,

ShaMan123

READY!
9ec6440 can be reverted if you think isIdentityMatrix should remain internal

ShaMan123 · 2022-12-22T10:19:29Z

src/mixins/itext_behavior.mixin.ts

    //  prepare instance for drag image snapshot by making all non selected text invisible
    const bgc = this.backgroundColor;
-    const styles = object.clone(this.styles, true);
+    const styles = clone(this.styles, true);


bug from #8421

happened due to use of the transformer and no test to discover it

ShaMan123 · 2022-12-22T10:19:48Z

src/mixins/itext_behavior.mixin.ts

@@ -679,7 +686,8 @@ export abstract class ITextBehaviorMixin<
   * @param {object} options
   * @param {DragEvent} options.e
   */
-  dragOverHandler({ e }: TEvent<DragEvent>) {
+  dragOverHandler(ev: TEvent<DragEvent>) {


happened because no test

ShaMan123 · 2022-12-22T10:20:00Z

src/mixins/itext_behavior.mixin.ts

   */
-  dropHandler({ e }: TEvent<DragEvent>) {
+  dropHandler(ev: TEvent<DragEvent>) {


happened because no test

ShaMan123 · 2022-12-22T10:20:42Z

src/mixins/itext_click_behavior.mixin.ts

@@ -106,6 +106,7 @@ export abstract class ITextClickBehaviorMixin<
    if (
      !this.canvas ||
      !this.editable ||
+      this.__isDragging ||


bug from #8013
Found thanks to Git + VSCODE merge conflict UX!!

merge conflict

ShaMan123 · 2022-12-22T10:21:30Z

src/mixins/itext_behavior.mixin.ts

      ctx.scale(1 / retinaScaling, 1 / retinaScaling);
+      const [a, b, c, d] = vpt;
+      const origin = new Point().transform(vpt, true);
+      ctx.transform(a, b, c, d, -origin.x, -origin.y);


fix drag image vpt

this is a fix to a bug unrelated to the migration

ShaMan123

added test
now it is possible to see there is a blur in the drag image.
no idea how to handle it

asturur · 2023-01-09T01:31:32Z

I didn't see this PR here.
What was the actual bug and how did we make it?
I see there was a bad if with a missing condition in a conflict resolution, but what did it cause?
there is also seems to be more than one fix that is fine, but would be nice to figure if those are merge bugs we caused or existing bugs we discovered later

src/mixins/itext_behavior.mixin.ts

ShaMan123 · 2023-01-09T05:14:51Z

I commented all fixes and changes and referenced where they originated from.

ShaMan123

Added tests

ShaMan123

fixed another small bug, focusing hiddenTextarea after drag end since it got blurred during the drag

@asturur I have done a follow up to this PR #8598 , please move to that once this is merged

ShaMan123 · 2023-01-14T12:08:29Z

I prefer changes to be made there instead of porting.

asturur · 2023-01-15T14:19:37Z

For the blur in the dragged image i can fix it, i think the principle is the same as retina scaling

asturur · 2023-01-15T14:21:29Z

Doing this today and the next, now i have guests i ll be off some hours

asturur · 2023-01-15T22:50:17Z

src/shapes/itext.class.ts

@@ -283,6 +283,18 @@ export class IText extends ITextClickBehaviorMixin<ITextEvents> {
    this.renderCursorOrSelection();
  }

+  /**
+   * @override block cursor/selection logic while rendering the exported canvas
+   * @todo this workaround should be replaced with a more robust solution


i made years ago a renderExport function for those cases.
I would run the usual render code but without side effects and without runing the current caches and with less blur.
I should officialize it, it covers also those cases.

This is fine anyway

I am for. An object should have a method for that.
BTW #8298 offers a solution as well.

asturur · 2023-01-15T22:55:14Z

src/mixins/itext_click_behavior.mixin.ts

@@ -192,6 +195,8 @@ export abstract class ITextClickBehaviorMixin<
      return;
    }

+    shouldSetCursor && this.setCursorByClick(options.e);


What is unclear to me is why this shouldn't have triggered on mouse down.
Not a big deal, this is not relevant to merging.
Can you try to explain anyway this is here? Setting cursor was usually mouse down tas

I see the mouseDown has an extra this.__isDragging || how do we trigger dragging before mouse down event?

_mouseDownHandlerBefore sets this.__isDragging to keep selection, it is clearer in #8598

fabric.js/src/mixins/DraggableTextDelegate.ts

Lines 12 to 21 in 27dcd9e

/**

* #### Dragging IText/Textbox Lifecycle

* - {@link start} is called from `mousedown:before` {@link IText#_mouseDownHandlerBefore} and determines if dragging should start by testing {@link isPointerOverSelection}

* - if true `mousedown` {@link IText#_mouseDownHandler} is blocked to keep selection

* - if the pointer moves, canvas fires numerous mousemove {@link Canvas#_onMouseMove} that we make sure **aren't** prevented ({@link IText#shouldStartDragging}) in order for the window to start a drag session

* - once/if the session starts canvas calls {@link onDragStart} on the active object to determine if dragging should occur

* - canvas fires relevant drag events that are handled by the handlers defined in this scope

* - {@link end} is called from `mouseup` {@link IText#mouseUpHandler}, blocking IText default click behavior

* - in case the drag session didn't occur, {@link end} handles a click, since logic to do so was blocked during `mousedown`

*/

)

ok. so mosedown:before shoud allow for the developer to interact before fabric does anything.
That was the point of that event.
Let's try to not break that contract.

So you suggest moving logic to mouse down?
Sounds correct

asturur · 2023-01-15T23:34:55Z

I have noticed that undoing a text drop isn't supported natively and I wonder why. Would love your feedback. The textarea value is changed programmatically on drop. Is there a way to do that and have the change enter the native history stack?

I do think yes.
Probably we should attempt at forwarding the drop event with the same data to the native textarea.
So we enter edit, we capture the event on canvas, but we send it to the textarea, we check if it generates an input event to automatically adjust text.

But please let this idea at rest now we have other stuff to do

ShaMan123 · 2023-01-16T06:28:27Z

So we enter edit, we capture the event on canvas, but we send it to the textarea, we check if it generates an input event to automatically adjust text.

But please let this idea at rest now we have other stuff to do

I don't think the DOM will let us retarget an event.
Letting it rest.

History isn't hard to impl by yourself, dragging is. So I am OK with it.
In my projects I will do so.

asturur · 2023-01-16T08:28:13Z

I m sure you can clone events and forward them where you want, i did so in the past to have a layer that was capturing some events but setting others to a sibling below

#8534 (comment)

…port (#8534)

ShaMan123 added 3 commits December 20, 2022 22:19

Update itext_behavior.mixin.ts

f750f9d

Update itext_behavior.mixin.ts

617bc87

fix(): bad conflict resolution

db8739a

d69e3dd of #8013

ShaMan123 added 3 commits December 22, 2022 11:44

fix no dpr on drag image offset

5555c5c

fix(): drag image + vpt

df95065

test isIdentityMatrix

9ec6440

ShaMan123 commented Dec 22, 2022

View reviewed changes

ShaMan123 requested a review from asturur December 22, 2022 10:31

Update CHANGELOG.md

c385df1

ShaMan123 added text v6 labels Dec 22, 2022

ShaMan123 changed the title ~~fix(): Draggable Text~~ fix(): Draggable Text Migration Dec 22, 2022

ShaMan123 added 4 commits December 22, 2022 12:55

cleanup

057af08

fix(): acount for stroke style

7fb8378

drag image visual tests

7899a25

Update itext_behavior.mixin.ts

92cfc43

ShaMan123 commented Dec 22, 2022

View reviewed changes

ShaMan123 added 2 commits December 22, 2022 14:16

better goldens

328a016

Merge branch 'master' into fix-text-dnd

2bdf139

ShaMan123 changed the title ~~fix(): Draggable Text Migration~~ fix(): Draggable Text Migration Regression Jan 3, 2023

ShaMan123 added 4 commits January 3, 2023 23:27

Update CHANGELOG.md

9ca3c4f

Merge branch 'master' into fix-text-dnd

ecb8073

export isIdentityMatrix

ee30fec

Merge branch 'master' into fix-text-dnd

86242d6

asturur reviewed Jan 9, 2023

View reviewed changes

src/mixins/itext_behavior.mixin.ts Outdated Show resolved Hide resolved

Merge branch 'master' into fix-text-dnd

e26c326

ShaMan123 added 4 commits January 13, 2023 03:29

Update text.js

e6bd3c8

Update CHANGELOG.md

68b036c

cleanup

fd48378

Update text.js

4fcb2ea

ShaMan123 commented Jan 13, 2023

View reviewed changes

ShaMan123 requested a review from asturur January 13, 2023 01:36

ShaMan123 added 4 commits January 13, 2023 08:40

renderEffects

565a9a8

Update canvas_events.ts

fa0a91d

fix(): focus hiddentextarea in edge cases

f4fee32

correct comment

de198d4

ShaMan123 mentioned this pull request Jan 14, 2023

refactor(IText): extract draggable text logic to a delegate #8598

Merged

ShaMan123 commented Jan 14, 2023

View reviewed changes

Merge branch 'master' into fix-text-dnd

c0fa8fb

asturur reviewed Jan 15, 2023

View reviewed changes

asturur previously approved these changes Jan 15, 2023

View reviewed changes

added coment and extract dropTarget variable

3dd25c2

asturur dismissed their stale review via 3dd25c2 January 15, 2023 23:51

prettier

7ecb5ea

asturur approved these changes Jan 16, 2023

View reviewed changes

asturur merged commit dbc4824 into master Jan 16, 2023

ShaMan123 added a commit that referenced this pull request Jan 16, 2023

start on mouse down + better drag interface

2aadf5d

#8534 (comment)

ShaMan123 deleted the fix-text-dnd branch January 16, 2023 13:54

ShaMan123 added a commit that referenced this pull request Mar 16, 2024

fix(): Draggable Text Regression and improvements for retina and view…

5593c3f

…port (#8534)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(): Draggable Text Migration Regression #8534

fix(): Draggable Text Migration Regression #8534

ShaMan123 commented Dec 22, 2022 •

edited

Loading

github-actions bot commented Dec 22, 2022 •

edited

Loading

github-actions bot commented Dec 22, 2022 •

edited

Loading

ShaMan123 left a comment

ShaMan123 Dec 22, 2022

ShaMan123 Jan 9, 2023

ShaMan123 Dec 22, 2022

ShaMan123 Jan 9, 2023

ShaMan123 Dec 22, 2022

ShaMan123 Jan 9, 2023

ShaMan123 Dec 22, 2022 •

edited

Loading

ShaMan123 Jan 9, 2023

ShaMan123 Dec 22, 2022

ShaMan123 Jan 9, 2023

ShaMan123 left a comment

asturur commented Jan 9, 2023 •

edited

Loading

ShaMan123 commented Jan 9, 2023

ShaMan123 left a comment

ShaMan123 left a comment

ShaMan123 commented Jan 14, 2023 •

edited

Loading

asturur commented Jan 15, 2023

asturur commented Jan 15, 2023

asturur Jan 15, 2023

asturur Jan 15, 2023

ShaMan123 Jan 16, 2023

asturur Jan 15, 2023

asturur Jan 15, 2023

ShaMan123 Jan 16, 2023

asturur Jan 16, 2023

ShaMan123 Jan 16, 2023

ShaMan123 Jan 16, 2023

asturur commented Jan 15, 2023

ShaMan123 commented Jan 16, 2023 •

edited

Loading

asturur commented Jan 16, 2023

	/**
	* #### Dragging IText/Textbox Lifecycle
	* - {@link start} is called from `mousedown:before` {@link IText#_mouseDownHandlerBefore} and determines if dragging should start by testing {@link isPointerOverSelection}
	* - if true `mousedown` {@link IText#_mouseDownHandler} is blocked to keep selection
	* - if the pointer moves, canvas fires numerous mousemove {@link Canvas#_onMouseMove} that we make sure aren't prevented ({@link IText#shouldStartDragging}) in order for the window to start a drag session
	* - once/if the session starts canvas calls {@link onDragStart} on the active object to determine if dragging should occur
	* - canvas fires relevant drag events that are handled by the handlers defined in this scope
	* - {@link end} is called from `mouseup` {@link IText#mouseUpHandler}, blocking IText default click behavior
	* - in case the drag session didn't occur, {@link end} handles a click, since logic to do so was blocked during `mousedown`
	*/

fix(): Draggable Text Migration Regression #8534

fix(): Draggable Text Migration Regression #8534

Conversation

ShaMan123 commented Dec 22, 2022 • edited Loading

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Dec 22, 2022 • edited Loading

github-actions bot commented Dec 22, 2022 • edited Loading

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

ShaMan123 Dec 22, 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

ShaMan123 left a comment

Choose a reason for hiding this comment

asturur commented Jan 9, 2023 • edited Loading

ShaMan123 commented Jan 9, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 commented Jan 14, 2023 • edited Loading

asturur commented Jan 15, 2023

asturur commented Jan 15, 2023

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 Jan 15, 2023

ShaMan123 commented Jan 16, 2023 • edited Loading

asturur commented Jan 16, 2023

ShaMan123 commented Dec 22, 2022 •

edited

Loading

github-actions bot commented Dec 22, 2022 •

edited

Loading

github-actions bot commented Dec 22, 2022 •

edited

Loading

ShaMan123 Dec 22, 2022 •

edited

Loading

asturur commented Jan 9, 2023 •

edited

Loading

ShaMan123 commented Jan 14, 2023 •

edited

Loading

ShaMan123 commented Jan 16, 2023 •

edited

Loading