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.

Sign up for GitHub

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

Jump to bottom

BREAKING fix(Text): styles onInput #9096

Closed
wants to merge 42 commits into from
Closed

BREAKING fix(Text): styles onInput #9096

wants to merge 42 commits into from

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jul 15, 2023 •
edited
Loading

Motivation

closes #9028

Description

I had an Eureka about text styles and saw it to be true
There was a confusion in the code, styles correlates to the UNWRAPPED lines so I decided to make it strict.
This was found across a lot of logic

Changes

BERAKING: get2DCursorLocation => getCursorPosition, getStyleCursorPosition to avoid confusion
The dev will be happy to find this breaking because it will uncover bugs.
BREAKING: rm getCurrentCharFontSize, getCurrentCharColor, _getCurrentCharIndex
BREAKING: rm cleanStyle, removeStyle, _extendStyles, _setStyleDeclaration, _deleteStyleDeclaration, _getLineStyle, _setLineStyle, _deleteLineStyle
BREAKING: rm _getStyleDeclaration, getCompleteStyleDeclaration => getStyleDeclaration

Gist

In Action

https://github.com/fabricjs/fabric.js/assets/34343793/e31635bb-906c-432b-bf01-d7524b995d35
video.webm

Fresh from PLAYWRIGHT

FIXED

video.webm

test-finished-1

MASTER

video.webm

test-finished-1

@ShaMan123 ShaMan123 requested a review from asturur July 15, 2023 00:53
@github-actions
Copy link
Contributor

github-actions bot commented Jul 15, 2023 •
edited
Loading

Build Stats

file / KB (diff) bundled minified
fabric 914.875 (-3.607) 305.132 (-0.281)

@github-actions
Copy link
Contributor

github-actions bot commented Jul 15, 2023 •
edited
Loading

Coverage after merging fix-text-styles! into master will be

82.92%

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%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
   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%118, 129, 138, 31, 94
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.ts79.05%77.54%83.05%79.57%1000–1001, 1001, 1001–1003, 1005–1006, 1006, 1006, 1008, 1016, 1016, 1016–1018, 1018, 1018, 1024–1025, 1033–1034, 1034, 1034–1035, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1515, 161, 186, 296–297, 300–304, 309, 332–333, 338–343, 36, 363, 363, 363–364, 364, 364–365, 373, 378–379, 379, 379–380, 382, 391, 397–398, 398, 398, 40, 441, 449, 453, 453, 453–454, 456, 538–539, 539, 539–540, 546, 546, 546–548, 568, 570, 570, 570–571, 571, 571, 574, 574, 574–575, 578, 587–588, 590–591, 593, 593–594, 596–597, 609–610, 610, 610–611, 613–618, 624, 631, 668, 668, 668, 670, 672–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 741, 741, 799–800, 800, 800–801, 803, 806–807, 807, 807–808, 810–811, 814, 814–816, 819–820, 890, 902, 909, 930, 962, 983–984
   SelectableCanvas.ts94.43%92.24%94.23%96.17%1115, 1115–1116, 1119, 1139, 1139, 1188, 1196, 1315, 1317, 1319–1320, 519, 694–695, 697–698, 698, 698, 747–748, 809–810, 863–865, 897, 902–903, 930–931
   StaticCanvas.ts96.78%93.09%100%98.53%1036–1037, 1037, 1037–1038, 1158, 1168, 1224–1225, 1228, 1263–1264, 1340, 1349, 1349, 1353, 1353, 1400–1401, 308–309, 325, 693, 705–706
   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.ts100%100%100%100%
   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.ts93.33%87.88%91.67%97.78%175, 240, 327, 327, 362
   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.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%131–132, 163–164, 171, 177, 179
   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.ts100%100%100%100%

@ShaMan123
BREAKING: rm unused methods
0222e0a 
BREAKING: rm `getCurrentCharFontSize`, `getCurrentCharColor`, `_getCurrentCharIndex`
@ShaMan123
Merge branch 'fix-text-styles!' of https://github.com/fabricjs/fabric.js
5928fdb 
 into fix-text-styles!
@ShaMan123
Update CHANGELOG.md
7c8d943
ShaMan123
ShaMan123 commented Jul 15, 2023
View reviewed changes
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.

DONE

CHANGELOG.md
Comment on lines +6 to +7
**BERAKING**: `get2DCursorLocation` => `getCursorPosition`, `getStyleCursorPosition`
**BREAKING**: rm `getCurrentCharFontSize`, `getCurrentCharColor`, `_getCurrentCharIndex`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add to #8299

src/shapes/Text/StyledText.ts
@@ -182,7 +185,7 @@ export abstract class StyledText<
}

private _extendStyles(index: number, styles: TextStyleDeclaration): void {
const { lineIndex, charIndex } = this.get2DCursorLocation(index);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BUG!

@ShaMan123 ShaMan123 changed the title fix(): text styles offsetting patch(): text styles offsetting Jul 15, 2023
@ShaMan123 ShaMan123 changed the title patch(): text styles offsetting fix(Text): onInput styles Jul 15, 2023
ShaMan123 and others added 9 commits July 15, 2023 12:42
@ShaMan123
Squashed commit of the following:
0722748 
commit 05da60d
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Jul 15 12:40:06 2023 +0530

    cleanup

commit ac82663
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Jul 15 12:38:34 2023 +0530

    Update Text.ts

commit bfb99e9
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Jul 15 10:32:28 2023 +0530

    progress

commit 610bcb8
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Jul 15 09:26:27 2023 +0530

    d
@github-actions
update CHANGELOG.md
c01c5d6
@ShaMan123
Merge branch 'fix-text-styles!' of https://github.com/fabricjs/fabric.js
46911e4 
 into fix-text-styles!
@ShaMan123
almost
d596268
@ShaMan123
better
88cc93c
@ShaMan123
Update Text.ts
92b2552
@ShaMan123
FIX!@#
996426e
@ShaMan123
Update ITextKeyBehavior.ts
098575b
@ShaMan123
Update StyledText.ts
5be1fde
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jul 15, 2023 •
edited
Loading

Progress
Bugs:

  • 0:40
  • 0:59
Parcel.Sandbox.-.Google.Chrome.2023-07-15.14-05-42_Trim.mp4

@ShaMan123
Copy link
Contributor Author

Almost all the calls to _getStyleDeclaration are wrong

@ShaMan123
Squashed commit of the following:
3709b71 
commit 8c7f09f29d5a095aa1a25299c9729b5a063537e9
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Jul 15 15:43:34 2023 +0530

    artifacts

commit 849cc479c781be1246521b40e413d5a8d9dafba0
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sat Jul 15 15:39:26 2023 +0530

    cleanup!
ShaMan123
ShaMan123 commented Jul 15, 2023
View reviewed changes
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looking stable
I want to add tests but I do not know how to generate playwright tests yet

@ShaMan123
Copy link
Contributor Author

blocked by #9078 to perform snapshots

ShaMan123
ShaMan123 commented Jul 15, 2023
View reviewed changes
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.

passing tests

ShaMan123
ShaMan123 commented Jul 15, 2023
View reviewed changes
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.

Apart from adding snapshot for the playwright test this is DONE so feel free to review

ShaMan123
ShaMan123 commented Jul 15, 2023
View reviewed changes
src/shapes/Text/Text.ts
*/
get2DCursorLocation(selectionStart: number, skipWrapping?: boolean) {
const lines = skipWrapping ? this._unwrappedTextLines : this._textLines;
getCursorPosition(index: number, lines = this._textLines) {
Copy link
Contributor Author

@ShaMan123 ShaMan123 Jul 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

not sure 100% this method is not buggy

ShaMan123
ShaMan123 commented Jul 15, 2023
View reviewed changes
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ooops I broke setSelectionStyles - should merge the style and not override - jest has already proven its value

@ShaMan123 ShaMan123 changed the title fix(Text): onInput styles BREAKING fix(Text): styles onInput Jul 16, 2023
@ShaMan123
Copy link
Contributor Author

updated desc, playwright is the bomb

@ShaMan123 ShaMan123 closed this Jul 16, 2023
@ShaMan123 ShaMan123 reopened this Jul 16, 2023
@ShaMan123 ShaMan123 added the text label Jul 19, 2023
@ShaMan123 ShaMan123 mentioned this pull request Sep 1, 2023
Merged
@asturur asturur closed this Nov 3, 2023
ShaMan123
ShaMan123 commented Nov 16, 2023
View reviewed changes
src/env/types.ts
import type { GLProbe } from '../filters/GLProbes/GLProbe';
import type { DOMWindow } from 'jsdom';

export type TCopyPasteData = {
copiedText?: string;
copiedStyle?: Record<string, string>;
copiedTextStyle?: TextStyleDeclaration[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asturur this bug was not fixed in your PR, was it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asturur asturur Awaiting requested review from asturur

Assignees
No one assigned
Labels
text
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG]: text style onInput
2 participants
@ShaMan123 @asturur