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

patch(Canvas): move event mouse:up:before earlier in the logic for more control #9434

Merged
merged 7 commits into from
Oct 27, 2023

Conversation

ShaMan123
Copy link
Contributor

Motivation

I want to disable mouseup default behavior from the mouse:up:before event
In mouse down it is possible with delete canvas._target, see

fabric.js/src/canvas/Canvas.ts

Lines 1061 to 1063 in 9c8e102

this._cacheTransformEventData(e);
this._handleEvent(e, 'down:before');
let target: FabricObject | undefined = this._target;

In mouse up it isn't because of the order of execution.
So I changed the const target to be defined after the mouse:up:before event is called

Description

Please add this to beta15

Changes

Gist

In Action

@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2023

Build Stats

file / KB (diff) bundled minified
fabric 912.372 (-0.283) 305.624 (-0.052)

Copy link
Contributor

@jiayihu jiayihu left a comment

Choose a reason for hiding this comment

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

LGTM. For a long-term solution, returning false from handleEvent sounds a better solution, or even checking event. defaultPrevented on the event.

@ShaMan123
Copy link
Contributor Author

LGTM. For a long-term solution, returning false from handleEvent sounds a better solution, or even checking event. defaultPrevented on the event.

definately, I agree

@ShaMan123 ShaMan123 closed this Oct 23, 2023
@ShaMan123 ShaMan123 reopened this Oct 23, 2023
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Changed isClick to populate only on up events including up:before
Removed used currentTarget currentSubTargets from the event data #9450

src/canvas/Canvas.ts Show resolved Hide resolved
src/canvas/Canvas.ts Show resolved Hide resolved
src/canvas/Canvas.ts Show resolved Hide resolved
src/canvas/__tests__/__snapshots__/eventData.test.ts.snap Outdated Show resolved Hide resolved
test/unit/canvas_events.js Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Coverage after merging patch-mouse-up into master will be

82.95%

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.71%94.64%86.67%97.09%101, 104, 207–208, 233–234
   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/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, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts78.85%77.22%83.05%79.45%1000–1001, 1004–1005, 1005, 1005, 1007, 1015, 1015, 1015–1017, 1017, 1017, 1024–1025, 1033–1034, 1034, 1034–1035, 1041, 1043, 1074–1076, 1079–1080, 1084–1085, 1204–1206, 1209–1210, 1283, 1402, 152, 1524, 177, 286–287, 290–294, 299, 322–323, 328–333, 353, 353, 353–354, 354, 354–355, 36, 363, 368–369, 369, 369–370, 372, 381, 387–388, 388, 388, 40, 431, 439, 443, 443, 443–444, 446, 529–530, 530, 530–531, 537, 537, 537–539, 559, 561, 561, 561–562, 562, 562, 565, 565, 565–566, 569, 578–579, 581–582, 584, 584–585, 587–588, 600–601, 601, 601–602, 604–609, 615, 622, 659, 659, 659, 661, 663–668, 674, 680, 680, 680–681, 683, 686, 691, 704, 732, 732, 792–793, 793, 793–794, 796, 799–800, 800, 800–801, 803–804, 807, 807–809, 812–813, 889, 901, 908, 908, 908, 926, 960, 981–982, 998–999, 999, 999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts94.35%92.06%94.23%96.20%1080, 1082, 1084–1085, 301, 471–472, 474–475, 475, 475, 524–525, 586–587, 600, 640–642, 674, 679–680, 707–708, 880, 880–881, 884, 904, 904, 953, 961
   StaticCanvas.ts96.78%93.09%100%98.53%1031, 1041, 1093–1094, 1097, 1132–1133, 1209, 1218, 1218, 1222, 1222, 1269–1270, 187–188, 204, 571, 583–584, 914–915, 915, 915–916
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts84.31%71.43%91.67%88%17–18, 18, 18–19, 19, 19
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.ts7.25%0%0%13.51%103, 108, 120, 120, 120, 120, 120, 122–125, 125, 128, 135, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
   rotate.ts19.57%12.50%50%21.43%41, 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.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%131–132, 163–164, 171, 177, 179
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
&nb

@asturur asturur changed the title patch(Canvas): better respect of mouseup before patch(Canvas): move event mouse:up:before earlier in the logic for more control Oct 27, 2023
@asturur asturur merged commit 526216a into master Oct 27, 2023
24 checks passed
@asturur asturur deleted the patch-mouse-up branch October 27, 2023 08:11
asturur pushed a commit that referenced this pull request Nov 12, 2023
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.

3 participants