Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(): block enterEditing after endCurrentTransform #9513

Merged
merged 4 commits into from
Nov 30, 2023

Conversation

ShaMan123
Copy link
Contributor

Motivation

Description

  1. transform object
  2. call endCurrentTransform
  3. mouse up inside the object
  4. object enters editing => this is not expected

Changes

Gist

In Action

Copy link
Contributor

github-actions bot commented Nov 27, 2023

Build Stats

file / KB (diff) bundled minified
fabric 906.440 (+0.027) 304.743 (+0.025)

@ShaMan123 ShaMan123 changed the title fix(): enterEditing after endCurrentTransform fix(): block enterEditing after endCurrentTransform Nov 27, 2023
@asturur
Copy link
Member

asturur commented Nov 30, 2023

@ShaMan123 i need to understand why you call endCurrentTransform manually.
This is an issue with the transition of the api, and is important to understand which is the use case for that.
Is not the first misfunction of endCurrentTransform that wasn't a thing indeed in 5.x

How do you use it?

@@ -84,7 +84,7 @@ export abstract class ITextClickBehavior<
this.__lastLastClickTime = this.__lastClickTime;
this.__lastClickTime = this.__newClickTime;
this.__lastPointer = newPointer;
this.__lastSelected = this.selected;
this.__lastSelected = this.selected && !this.getActiveControl();
Copy link
Member

Choose a reason for hiding this comment

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

__lastSelected is at this point a wrong name.
It is used exclusively to stop the mouse up default action and does nothing else, is already private.

I would rename it to `_willEnterEditOnMouseUp' and add a JSDOC block on the top where is declared.

What do you think?

I would discourage anyone to build custom logic on those internals state flip flops

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't even need to be done now. We can do an extra PR with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the name
Since it is a very crucial part of state there are use cases that you need to block the enter editing somehow.
For the majority of cases this small fix should do

asturur
asturur previously approved these changes Nov 30, 2023
@asturur asturur merged commit 7d9f631 into master Nov 30, 2023
8 checks passed
@asturur asturur deleted the fix-mouse-up-after-end-transform branch November 30, 2023 11:08
Copy link
Contributor

Coverage after merging fix-mouse-up-after-end-transform into master will be

82.76%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 35, 38, 41
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.85%94.83%87.10%97.14%109, 112, 215–216, 241–242
   CommonMethods.ts96.55%87.50%100%100%9
   Intersection.ts100%100%100%100%
   Observable.ts87.76%85.29%87.50%89.58%134–135, 160–161, 32–33, 41, 50, 78, 89
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
src/LayoutManager
   LayoutManager.ts85.80%90.91%68%87.32%127–128, 130–131, 131, 131, 131, 131, 231, 294–295, 299, 39, 60, 78
   constants.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts81.58%72.22%100%88.24%29–30, 40, 52, 55–56, 63
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts11.11%0%0%25%19–20, 22, 22, 22, 22, 22
   LayoutStrategy.ts86.54%64.71%100%96.30%36, 43, 43, 43, 51, 78–79
   utils.ts89.47%80%100%100%28, 34
src/Pattern
   Pattern.ts92.31%91.89%90%93.55%119, 130, 139, 32, 95
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79%76.99%83.05%79.84%1002–1003, 1003, 1003–1004, 1010, 1012, 1040–1042, 1045–1046, 1050–1051, 1174–1176, 1179–1180, 1253, 1368, 1490, 155, 180, 286, 286–291, 296, 319–320, 325–330, 350, 350, 350–351, 351, 351–352, 360, 365–366, 366, 366–367, 369, 378, 384–385, 385, 385, 39, 428, 43, 436, 440, 440, 440–441, 443, 525–526, 526, 526–527, 533, 533, 533–535, 555, 557, 557, 557–558, 558, 558, 561, 561, 561–562, 565, 574–575, 577–578, 580, 580–581, 583–584, 596–597, 597, 597–598, 600–605, 611, 618, 655, 655, 655, 657, 659–664, 670, 676, 676, 676–677, 679, 682, 687, 700, 728, 780–781, 781, 781, 781, 781, 781, 784–785, 788, 788–790, 793–794, 876, 883, 883, 883, 896, 929, 950–951, 967–968, 968, 968–970, 973–974, 974, 974, 976, 984, 984, 984–986, 986, 986, 993–994
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts93.21%91.60%94.44%94.22%1013, 1021, 1140, 1142, 1144–1145, 302, 472–473, 475–476, 476, 476, 525–526, 587–588, 601, 641–643, 675, 680–681, 708–709, 769–770, 775–779, 781, 940, 940–941, 944, 964, 964
   StaticCanvas.ts96.78%93.09%100%98.53%1019, 1029, 1081–1082, 1085, 1120–1121, 1197, 1206, 1206, 1210, 1210, 1257–1258, 178–179, 195, 559, 571–572, 902–903, 903, 903–904
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts86%71.43%91.67%91.67%17, 17, 17–18, 18, 18
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts97.50%88.89%100%100%33
   util.ts86.67%80.56%83.33%93.94%14, 26, 63–64, 67, 67, 74, 93–94
src/color
   Color.ts94.96%91.67%96.30%96.05%233, 258–259, 267–268, 48
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts85.71%76.92%100%89.74%55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
   Control.ts94.44%93.10%91.67%96.77%183, 249, 354
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   fireEvent.ts88.89%75%100%100%13
   polyControl.ts8.57%0%0%15.79%100, 106, 111, 126, 126, 126, 126, 126, 128–131, 131, 134, 141, 20, 28–32, 32, 32, 32, 32, 32, 32, 32, 53–59, 59, 59, 59, 59, 61, 66–67, 69, 79, 85–87, 87, 89, 92–93, 93, 93, 93, 93, 95
   rotate.ts19.57%12.50%50%21.43%41,

@ShaMan123
Copy link
Contributor Author

The use case:
A user is transforming, during which endCurrentTransform is called (reasons could be a user action like keyboard input, a db update changing something in the object etc.).
The user's mouse is still down and moving.
If mouseup occurs over the transform target it would enter editing and that is not expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants