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): styles #9081

Closed
wants to merge 40 commits into from
Closed

fix(text): styles #9081

wants to merge 40 commits into from

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jul 9, 2023

Motivation

closes #9028

Description

Extracts style logic to a manager
Styles has been refactored into an array mirroring the chars to make our lives easier.
In addition I have exposed a method getDiffFromInputEvent to make our lives easier.
I have left the interface untouched so not to break, though I think we should remove all that bad interface.

Changes

Gist

TODO:

  • test

In Action

ShaMan123 and others added 17 commits June 19, 2023 10:42
fix!!!

fix!!!
commit b78dda2
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 12:58:28 2023 +0900

    Update Text.ts

commit 7119251
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 12:53:55 2023 +0900

    Update Text.ts

commit 5b904af
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 12:46:28 2023 +0900

    FINALLY OMG

commit 2cc92a5
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 12:24:31 2023 +0900

    strict

commit ac081d1
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 12:06:40 2023 +0900

    go

commit 652a345
Merge: 0ba2c8d e0034f5
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 09:10:36 2023 +0900

    Merge branch 'fix-9010' of https://github.com/fabricjs/fabric.js into fix-9010

commit 0ba2c8d
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 09:10:10 2023 +0900

    f******9

commit e0034f5
Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Date:   Sun Jul 9 00:07:11 2023 +0000

    update CHANGELOG.md

commit 4c3f9f2
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 08:55:10 2023 +0900

    bullshit

commit f2ccf8a
Author: ShaMan123 <shacharnen@gmail.com>
Date:   Sun Jul 9 07:32:39 2023 +0900

    fix but not

    fix

    fix

    Revert "fix"

    This reverts commit 656e25f.

    Update text.js
Update TextSVGExportMixin.ts
@ShaMan123 ShaMan123 requested a review from asturur July 9, 2023 06:50
@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

Coverage after merging patch-text-styles into master will be

83.26%

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, 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.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%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, 37, 44, 51, 58, 65, 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.42%92.24%94.23%96.15%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.92%93.23%100%98.72%1037–1038, 1038, 1038–1039, 1159, 1169, 1223–1224, 1227, 1318, 1327, 1327, 1331, 1331, 1378–1379, 309–310, 326, 694, 706–707
   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%
   node.ts74.07%33.33%66.67%88.89%27,

@asturur
Copy link
Member

asturur commented Jul 9, 2023

@ShaMan123 you are welcome to fix the bug, but not accepting those kind of refactors right now.
Did you find the bug?

@ShaMan123
Copy link
Contributor Author

The mess is unbearable

@asturur
Copy link
Member

asturur commented Jul 9, 2023

Let's keep the conversation to the PR, since the related issue doesn't really say much else apart showing to video to be interpreted.

@asturur asturur closed this Jul 9, 2023
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jul 9, 2023

The bug is to do with bad design
Somewhere style is used with unwrapped text, and in another place with the wrapped value
I will not dig into the existing style logic - did you see the amount of code I removed just by moving to a simple array corresponding to the chars???? That fixed the bug

@ShaMan123
Copy link
Contributor Author

text is amess @asturur
I don't see how to move forward without completely breaking it
It can remain broken for a long time and then it dies...
Too much work to fix fabric

@ShaMan123
Copy link
Contributor Author

Why is this bad design?

Styles depend on layout (with no good reason) instead of depending on char offset from start.
Now let's say I have an app and I need to change the default size of textbox.
I can't iterate over the server, changing the width value because it will break all styles.
So I will have to run some ugly code on the client or spawn a node env to simply do textbox.width=150.
That makes no sense and is a nightmare.
Considering responsive layouts... I might want to restrict the size of the textbox in some cases and again I am in this pain of breaking styles.
Initializing textbox with a style is also impossible because you need to know the layout beforehand but the style depends on the layout and the layout depends on the style so you can't!

Styles should not depend on layout.

@asturur
Copy link
Member

asturur commented Jul 10, 2023

You know is bad design also because i told you the story of Itext, it was a demo and so textbox was pushed in by some contributor that needed wrappingtext, all was made with no planning. Fixing countless of style bug between 1.5 and 2.0 was a nightmare for me too.

We know text is a mess, repeating it won't make reafactor being accepted in this moment nor make the release happen faster.

I drew a line for no more refactors untill a stable release and docs.
a line is a line, i said it many times no more refactors before release at some point i have to keep my own word.

This will need to wait for the next breaking release, meanwhile we can try to fix the bug with the ugly code or we can keep the bug a bit longer since is here since 3.x

@ShaMan123
Copy link
Contributor Author

You are right
I tried and got frustrated
It cannot be fixed IMO because of style depending on layout and input changing all that
Backing off

@asturur
Copy link
Member

asturur commented Jul 10, 2023

Yeah but if you give me reproduction steps i can try.
I don't see what you do in the videos.

@ShaMan123
Copy link
Contributor Author

Start the vanilla app
Click on the bold text (I clicked before the .)
Hit enter a few times
Hit delete a few times
Styles shift badly

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.

[BUG]: text style onInput
2 participants