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(): Text broken when doing inputs #8775

Merged
merged 9 commits into from
Mar 16, 2023
Merged

fix(): Text broken when doing inputs #8775

merged 9 commits into from
Mar 16, 2023

Conversation

asturur
Copy link
Member

@asturur asturur commented Mar 12, 2023

Motivation

A recent change for removing the stateful mix has broken text input

Description

Every input is going to clear the dimension cache.
For the few cases in which we swap and a with another a in the same place, well we will pay the price of recalculation.
Not a big deal.

I could have added this._forceClearCache = true in onInput, but since updateTextarea is the part that immediately consumes it, and is also the only place from where that function is called, i opted to have updateTextarea always clear the dimension cache and set as dirty in case the initDimensions would return the exact same size ( swapping a letter with one with the same size )

Added 2 tests for onInput ( a small test but better than nothing ) and one for updateFromTextarea.

Changes

Gist

In Action

@asturur
Copy link
Member Author

asturur commented Mar 12, 2023

I need to write a test and verify that things like insertChars are working ro not

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2023

Build Stats

file / KB (diff) bundled minified
fabric 895.688 (-0.136) 304.382 (-0.175)

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2023

Coverage after merging fix-text into master will be

83.63%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 29, 32, 35
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.71%94.64%86.67%97.09%100, 103, 206–207, 232–233
   CommonMethods.ts96.55%87.50%100%100%10
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%144–145, 170–171, 39–40, 48, 57, 91, 99
   Pattern.ts92.21%91.89%90%93.33%114, 138, 149, 158, 51
   Point.ts100%100%100%100%
   Shadow.ts98.51%96%100%100%168
   cache.ts97.06%90%100%100%56
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
   typedefs.ts100%100%100%100%
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 133–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
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.01%82.35%100%93.75%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%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
   Canvas.ts78.98%77.54%81.67%79.60%1000, 1000, 1000–1002, 1004–1005, 1005, 1005, 1007, 1015, 1015, 1015–1017, 1017, 1017, 1023–1024, 1032–1033, 1033, 1033–1034, 1039, 1041, 1072–1074, 1077–1078, 1082–1083, 1197–1199, 1202–1203, 1276, 1396, 1519, 1589, 161, 186, 295–296, 299–303, 308, 331–332, 337–342, 36, 362, 362, 362–363, 363, 363–364, 372, 377–378, 378, 378–379, 381, 390, 396–397, 397, 397, 40, 440, 448, 452, 452, 452–453, 455, 537–538, 538, 538–539, 545, 545, 545–547, 567, 569, 569, 569–570, 570, 570, 573, 573, 573–574, 577, 586–587, 589–590, 592, 592–593, 595–596, 608–609, 609, 609–610, 612–616, 622, 629, 669, 669, 669, 671, 673–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 740, 740, 798–799, 799, 799–800, 802, 805–806, 806, 806–807, 809–810, 813, 813–815, 818–819, 889, 901, 908, 929, 961, 982–983, 999
   SelectableCanvas.ts94.40%91.30%94.44%96.61%1138, 1138–1139, 1142, 1162, 1162, 1221, 1271–1272, 1293, 1301, 1426, 1428, 1430–1431, 533, 717–718, 720–721, 721, 721, 770–771, 832–833, 886–888, 920, 925–926, 953–954
   StaticCanvas.ts96.01%91.48%100%97.93%1112–1113, 1113, 1113–1114, 1234, 1244, 1298–1299, 1302, 1337–1338, 1415, 1424, 1424, 1428, 1428, 1475–1476, 1690–1691, 311–312, 329, 409–410, 412–413, 774, 786–787, 872
   TextEditingManager.ts100%100%100%100%
src/color
   Color.ts92.16%86.49%100%94.29%330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   Control.ts93.90%88.89%90.91%97.73%235, 319, 319, 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%
   polyControl.ts5.97%0%0%11.11%100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   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%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts84.21%77.78%50%100%14, 17
   index.ts91.67%50%100%100%20
   node.ts76.92%50%66.67%88.24%27, 31–32, 32, 32
src/filters
   BaseFilter.ts21.93%22.81%32%19.05%106–109, 109, 109–110, 116, 116, 116–119, 137, 155, 169–174, 178–179, 179, 179–182,

this.canvas.requestRenderAll();
}
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

moved this up since it doesn't depend from any of the let/const.

Comment on lines -268 to -273
this.updateFromTextArea();
this.fire('changed');
if (this.canvas) {
this.canvas.fire('text:changed', { target: this });
this.canvas.requestRenderAll();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

deduped this

@asturur asturur requested a review from ShaMan123 March 12, 2023 23:29
@asturur
Copy link
Member Author

asturur commented Mar 12, 2023

this is ready

@asturur
Copy link
Member Author

asturur commented Mar 16, 2023

@ShaMan123 have a look tomorrow i want to have master fixed

Copy link
Contributor

@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.

This looks fine but is it enough?
What about #8711 ? It should be merged into this PR as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for removing the animation? I thought it is good as a showcase but it is annoying when developing

Copy link
Member Author

Choose a reason for hiding this comment

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

that is also an internal tool, not a showcase thing.
I needed to test text, i started with my first experience with something movinng that i couldn't click, when i managed to click it i figured out it wasn't editable.
I trashed it.
There is also the issue that when running it invalidates the cache every frame and is just more work on a laptop that is rebuilding the library in the background and rebuilding the next pages.
Everyone will use the template to add something that is needed in that occasion, maybe the best default is a rect, not even a textbox

@asturur
Copy link
Member Author

asturur commented Mar 16, 2023

#8711 probably.
As often happens the description of your pr are cryptic.

_shouldClearDimensionCache was changed in https://github.com/fabricjs/fabric.js/pull/8663, not taking into account state changes any longer including changes to text which should trigger a layout - text was changed but layout was skipped
This PR moderates those calls

Isn't clear what is broken and what is being fixed.
What does it means moderates? is removeChar broken? is insertChar broken?

I will merge both

@asturur asturur merged commit 8e058a0 into master Mar 16, 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.

2 participants