fix(IText): NO clear context top during rendering #8560

ShaMan123 · 2023-01-02T13:20:24Z

Motivation

There is an edge case where while using context top (free drawing) and a render occurs text instances render and clear their bbox from context top.
They shouldn't
Take a look at the video ending in a mouse up.

fabric.js.sandbox.-.Google.Chrome.2023-01-02.15-26-25_Trim.mp4

Description

clearContextTop suffers from a built in problem.
It clears the context with the known width/height values.
Efforts were made to overcome that (quite a lot it seems) and most of cases (distinct case no. 1, see below) were indeed fixed but not all (e.g. transforming). It did cause a need to track every case and clear the context before the change propagates to the instance, which is folly. More interaction, more cases, a mess.

There are 2 distinct cases in which we should consider how to act:

changing selection only (which is not part of the rendering cycle due to perf, imperative in sight of context state)
changing visuals (part of rendering, declarative in sight of context state)

Seems that efforts were focused on clearing context for the first case but the second case was neglected (rendering, transforming and many others). It can be understood since the first case was/is the main dev effort for making itext work. The second case is an integration thing, app level.

Changes

removed clearing context top:

vpt change
size change: initDimensions

Gist

The render method calls renderCursorOrSelection that runs only if in editing mode.
If executed it marks top context as dirty for the canvas rendering cycle.

Looking at what brush does and applying it to itext:
If the entire canvas is rendered it is 100% proof since the top context will be cleared by the canvas and then selection will be drawn by the instance that is being edited.
If we mess with the context w/o rendering canvas to handle selection as we do with key/input events then we must clear context top as part of logic and we must take into account any changes to size of instance and we should mark top context as dirty as well.
So the rule of thumb should be if we mess with selection alone then we call clearContextTop.

initDimensions is a good example for the confusion around this. The context was cleared before changes to size were committed to avoid leaving traces, but even though it was not 100% proof. Since canvas must be rendered in order for the new size to be visible it is a pure no. 2 distinct case so we can safely hook to the rendering cycle.

In Action

github-actions · 2023-01-02T13:22:06Z

Build Stats

file / KB (diff)	bundled	minified
fabric	931.649 (-0.400)	300.759 (-0.213)

github-actions · 2023-01-02T13:23:18Z

Coverage after merging itext-render-fix into master will be

83.35%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.js	35%	12.50%	0%	54.55%	430, 432, 432, 432, 432, 432, 434–435, 437, 437, 437–438
src
cache.ts	96.97%	90%	100%	100%	56
config.ts	77.27%	66.67%	66.67%	84.62%	130, 138–140, 151–153
constants.ts	100%	100%	100%	100%
env.ts	72.73%	53.33%	100%	79.17%	22, 22–23, 23, 23, 25, 25, 27, 29, 31–32, 62
intersection.class.ts	100%	100%	100%	100%
pattern.class.ts	92.19%	85.71%	100%	96.30%	118, 124, 135, 144, 96
point.class.ts	100%	100%	100%	100%
shadow.class.ts	98.48%	96%	100%	100%	156
src/brushes
base_brush.class.ts	100%	100%	100%	100%
circle_brush.class.ts	0%	0%	0%	0%	100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 134, 141, 143, 23, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
pattern_brush.class.ts	97.06%	87.50%	100%	100%	21
pencil_brush.class.ts	91.86%	85.42%	100%	93.58%	122–123, 152, 152–154, 276, 280, 285–286, 68–69, 84–85
spray_brush.class.ts	0%	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, 77, 89–91, 99
src/canvas
canvas.class.ts	93.15%	89.14%	94%	95.93%	1153, 1153–1154, 1157, 1177, 1177, 1239, 1275–1276, 1295–1296, 1304–1305, 1326, 1334, 1447–1448, 1450–1451, 1471–1472, 509, 576–577, 582, 592, 721–722, 724–725, 725, 725, 771–772, 833–834, 887–889, 919, 924–925, 954–955
canvas_events.ts	78.22%	75.45%	82.81%	79.45%	1007–1008, 1008, 1008–1010, 1012–1013, 1013, 1013, 1015, 1023, 1023, 1023–1025, 1025, 1025, 1031–1032, 1040–1041, 1041, 1041–1042, 1047, 1049, 1079–1081, 1084–1085, 1089–1090, 1188, 1208–1210, 1213–1214, 1286, 1406, 145, 1500–1501, 1507, 1511–1512, 1528, 1550, 1597, 1602, 1641, 170, 318–319, 319, 319–320, 320, 323–327, 332, 334, 347–350, 353, 372, 372, 372–373, 373, 373–374, 382, 387–388, 388, 388–389, 391, 400, 406–407, 407, 407, 443, 447, 447, 447, 447, 447–448, 450, 525, 527, 527, 527–529, 549, 551, 551, 551–552, 552, 552, 555, 555, 555–556, 559, 568–569, 571–572, 574, 574–575, 577–578, 590–591, 591, 591–592, 594–598, 604, 611, 651, 651, 651, 653, 655–659, 665, 671, 671, 671–672, 674, 677, 682, 695, 722, 778–779, 779, 779–780, 782, 785–786, 786, 786–787, 789–790, 793, 793–795, 798–799, 809, 891, 905, 912, 933, 965, 986–987, 993
static_canvas.class.ts	94.56%	88.97%	97.92%	97.11%	1125–1126, 1126, 1126–1127, 1247, 1257, 1311–1312, 1315, 1350–1351, 1429, 1438, 1443, 1492–1493, 1721, 1721–1722, 1772, 1775, 1778, 1778, 1778, 1781, 1784, 1784, 1784, 309, 345, 358, 411–412, 414–415, 424, 428–429, 432–433, 885
src/color
color.class.ts	92.16%	86.49%	100%	94.29%	330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
changeWidth.ts	100%	100%	100%	100%
control.class.ts	93.90%	88.89%	90.91%	97.73%	235, 319, 319, 354
controls.render.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
default_controls.ts	86.67%	66.67%	100%	100%	122, 129
drag.ts	100%	100%	100%	100%
rotate.ts	20%	12.50%	50%	22.22%	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.ts	93.41%	92.68%	100%	93.59%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	130–131, 162–163, 170, 176, 178
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/filters
2d_backend.class.ts	92%	83.33%	100%	93.75%	35–36
FilterBackend.ts	88.89%	88.89%	100%	85.71%	15–16
WebGLProbe.ts	40.54%	40%	60%	36.36%	28–30, 30, 30–31, 33–35, 43, 46–48, 48, 48–51, 53, 58
base_filter.class.ts	20.83%	20.83%	33.33%	18.18%	102–104, 104, 104–105, 112–115, 115, 115–116, 122, 122, 122–125, 143, 173–178, 182–183, 183, 183–186, 186, 186, 186, 186–188,

ShaMan123 · 2023-01-02T13:45:19Z

src/shapes/itext.class.ts

@@ -273,7 +273,7 @@ export class IText extends ITextClickBehaviorMixin<ITextEvents> {
   * @param {CanvasRenderingContext2D} ctx Context to render on
   */
  render(ctx: CanvasRenderingContext2D) {
-    this.clearContextTop();
+    // this.isEditing && this.clearContextTop();


Let's use blame and lets' see when it was introduced and in which context.

6 years ago....

Linked to those 2 issues:
#3378
and
#3386
In the PR it was linked to this clearContextTop
https://github.com/fabricjs/fabric.js/pull/3388/files#diff-b8bf422caf940e8585d2c4dac83f8ef76ceb2a1c0ffbf986202492dad12a7cfaR335-R348
That was already safeguarded for isEditing and this.active ( that does not exist anymore )

in this PR here
https://github.com/fabricjs/fabric.js/pull/7802/files#diff-b8bf422caf940e8585d2c4dac83f8ef76ceb2a1c0ffbf986202492dad12a7cfaL281-L291
you removed clearContextTop from iText and you added it to generic object, but you lost the isEditing boolean.
So i would argue that the isEditing boolean should come back. no?

I am to blame indeed.
Why did I move it there in the first place? Does it make sense??
#3386 seems unrelated.
#3378 is, however the bug wasn't solved for all cases. I will add tests proving that.
I think I understand my confusion now.
https://github.com/fabricjs/fabric.js/blame/d6f9c8f98813717372c1510193edae123dbfba11/src/shapes/itext.class.js#L303-L320

i think it was moved to support dragging and in the process we did not notice the isEditing.
Is enough to add it back i suppose and add a test for isEditing.

ShaMan123

next topic is clearContextTop under initDimensions
I would argue this is not needed since canvas will have to render in order for visuals to update and now since contextTopDirty is flagged this is taken care of.
Thoughts?

tests after this

ShaMan123 · 2023-01-03T12:41:47Z

src/shapes/textbox.class.ts

-    this.clearContextTop();
+    if (this.isEditing) {
+      this.initDelayedCursor();
+      this.clearContextTop();


ShaMan123 · 2023-01-03T12:42:03Z

src/shapes/itext.class.ts

-    this.clearContextTop();
+    if (this.isEditing) {
+      this.initDelayedCursor();
+      this.clearContextTop();


src/shapes/itext.class.ts

src/canvas/canvas.class.ts

asturur · 2023-01-03T14:24:00Z

it become a large change to understand at a glance.
Can you describe it at your best with words?

src/shapes/itext.class.ts

ShaMan123

Explaining what is going on:
clearContextTop suffers from a built in problem.
It clears the context with the known width/height values.
Efforts were made to overcome that (quite a lot it seems) and most of cases (distinct case no. 1, see below) were indeed fixed but not all (e.g. transforming). It did cause a need to track every case and clear the context before the change propagates to the instance, which is folly. More interaction, more cases, a mess.

The are 2 distinct cases in which we should consider how to act:

changing selection only (which is not part of the rendering cycle due to perf, imperative in sight of context state)
changing visuals (part of rendering, declarative in sight of context state)

Seems that efforts were focused on clearing context for the first case but the second case was neglected (rendering, transforming and many others). It can be understood since the first case was/is the main dev effort for making itext work. The second case is an integration thing, app level.

It is confusing. Some parts must be handled by the instance for perf reasons, same as brush.
But then we must have a mechanism that handles rendering.

Is that a good enough explanation?

The only immune way to cases is to mark the context as dirty and let the rendering cycle handle it. Same a brush does.
initDimensions is a good example. The context was cleared before changes to size were committed to avoid leaving traces, but even though it is not 100% proof. Since canvas must be rendered in order for the new size to but visible we can safely hook to the rendering cycle.

ShaMan123 · 2023-01-03T16:41:25Z

Another thing that came to me is that Brush#needsFullRender should have kicked in and rendered the entire path while drawing. Looking into the code, top context is rendered before main context and since text render method clears the top context this is what happened

There is an edge case where while using context top (free drawing) and a render occurs text instances render and clear their bbox from context top. They shouldn't Take a look at the video ending in a mouse up.

fabric.js.sandbox.-.Google.Chrome.2023-01-02.15-26-25_Trim.mp4

ShaMan123 · 2023-01-03T16:47:47Z

updated my explanation a bit

asturur · 2023-01-03T16:50:15Z

src/mixins/itext_behavior.mixin.ts

-    if (shouldClear) {
-      this.clearContextTop();
-    }
+    shouldClear && this.clearContextTop();


we should investigate if the minifier does this for us and in case stop doing it.

in case it doesn't, u mean?

how? everything is unreadable

i think the minifier swap if (condition) { single line } for condition && singleline automatically

not an issue of this pr but i don't think it make sense to be terse manualy now

asturur · 2023-01-03T16:53:47Z

src/shapes/itext.class.ts

@@ -306,19 +296,10 @@ export class IText extends ITextClickBehaviorMixin<ITextEvents> {
    } else {
      this.renderSelection(ctx, boundaries);
    }
+    this.canvas!.contextTopDirty = true;


Ok the difference here is that swapping this with the one from our render function here https://github.com/fabricjs/fabric.js/pull/8560/files#diff-876b5f345ecc11628b120c60dfe0b3fde14630cc3940551bcb4ed23f41c580bcL275 we are swapping a partial contextTop clear with a full contextTop clear.

That is fine if it is needed and solves bugs.
Is that the case?

indeed
re read my explanation. it is now clearer I hope. redone it

The render method calls renderCursorOrSelection that runs only if in editing mode.
If the entire canvas is rendered it is 100% proof since the top context will be cleared by the canvas and then selection will be drawn by the instance that is being edited.
If we mess with the context outside w/o rendering canvas as we do with key/input events then we must clear context top as part of logic and we must take into account any changes to size of instance

I will add this to the decription

CHANGELOG.md

ShaMan123 · 2023-01-03T17:26:08Z

@asturur I have updated description and I am proud of it now.
~~Tests reveal the fix isn't complete~~ Tests are a shit storm

ShaMan123 · 2023-01-03T18:43:53Z

@asturur I am baffled
The goldens I've committed are correct.
But... freak show time.
When testing in my machine some weird shit is going on.
Look

I was thinking it is retina scaling, but I am not sure where the f*** it is

ShaMan123 · 2023-01-03T18:51:59Z

So I am fed up with this shit and in reality I fixed it 🤬😡😖😱.
Please help 😩😔😵

ShaMan123 · 2023-01-03T20:14:38Z

Found the regression causing this. see #8520 cba9325

ShaMan123 · 2023-01-03T21:21:58Z

after merging #8520 or #8565 tests pass!

asturur · 2023-01-05T14:17:46Z

Sorry i m a bit busy today but i m free tomorrow.
I ll try to see if i understand what is going on.

asturur · 2023-01-05T14:28:12Z

Merged the retina fix.

asturur · 2023-01-06T21:35:53Z

Merging now, great to have added tests.

I had to revert the pure stylistic change i couldn't help myself.

…8560)

ShaMan123 added 2 commits January 2, 2023 15:03

Update itext.class.ts

991ce79

Update itext.class.ts

06ae409

ShaMan123 added the text label Jan 2, 2023

Update CHANGELOG.md

42a48ac

ShaMan123 requested a review from asturur January 2, 2023 13:25

add test

a0cb886

ShaMan123 commented Jan 2, 2023

View reviewed changes

ShaMan123 added 5 commits January 3, 2023 12:57

Update InteractiveObject.ts

ac47c6b

fix(): use contextTopDirty to clear selection

aa979b5

initDimensiosn: safeguard clearing

28ab3b1

Update itext_behavior.mixin.ts

539274c

Merge branch 'master' into itext-render-fix

ca0edd5

ShaMan123 commented Jan 3, 2023

View reviewed changes

ShaMan123 added 5 commits January 3, 2023 15:43

cleanup

4e5a8ae

test!

4a3133e

Update text.js

e2630d2

remove clear from initDims

b5ee1d1

test

7fafa27

asturur reviewed Jan 3, 2023

View reviewed changes

src/shapes/itext.class.ts Show resolved Hide resolved

cleanup

e09826c

ShaMan123 commented Jan 3, 2023

View reviewed changes

asturur reviewed Jan 3, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

2866903

asturur and others added 4 commits January 5, 2023 15:28

Merge branch 'master' into itext-render-fix

65dce6d

some prettier fix

1a69f15

Update text.js

63ec497

revert unchanged file

fbe9b09

asturur approved these changes Jan 6, 2023

View reviewed changes

asturur merged commit 99ff2d5 into master Jan 6, 2023

ShaMan123 deleted the itext-render-fix branch February 5, 2023 05:18

ShaMan123 added a commit that referenced this pull request Mar 16, 2024

fix(IText): make clear contextTop always from the main render cycle (#…

d17af82

…8560)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(IText): NO clear context top during rendering #8560

fix(IText): NO clear context top during rendering #8560

ShaMan123 commented Jan 2, 2023 •

edited

Loading

github-actions bot commented Jan 2, 2023 •

edited

Loading

github-actions bot commented Jan 2, 2023 •

edited

Loading

ShaMan123 Jan 2, 2023 •

edited

Loading

asturur Jan 3, 2023

asturur Jan 3, 2023

asturur Jan 3, 2023 •

edited

Loading

asturur Jan 3, 2023

ShaMan123 Jan 3, 2023

asturur Jan 3, 2023

ShaMan123 left a comment

ShaMan123 Jan 3, 2023

ShaMan123 Jan 3, 2023

asturur commented Jan 3, 2023

ShaMan123 left a comment •

edited

Loading

ShaMan123 commented Jan 3, 2023 •

edited

Loading

ShaMan123 commented Jan 3, 2023

asturur Jan 3, 2023

ShaMan123 Jan 3, 2023

ShaMan123 Jan 3, 2023

asturur Jan 3, 2023

asturur Jan 3, 2023

asturur Jan 3, 2023 •

edited

Loading

ShaMan123 Jan 3, 2023

ShaMan123 Jan 3, 2023

ShaMan123 Jan 3, 2023

ShaMan123 commented Jan 3, 2023 •

edited

Loading

ShaMan123 commented Jan 3, 2023 •

edited

Loading

ShaMan123 commented Jan 3, 2023

ShaMan123 commented Jan 3, 2023 •

edited

Loading

ShaMan123 commented Jan 3, 2023 •

edited

Loading

asturur commented Jan 5, 2023

asturur commented Jan 5, 2023

asturur commented Jan 6, 2023

fix(IText): **NO** clear context top during rendering #8560

fix(IText): **NO** clear context top during rendering #8560

Conversation

ShaMan123 commented Jan 2, 2023 • edited Loading

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Jan 2, 2023 • edited Loading

github-actions bot commented Jan 2, 2023 • edited Loading

ShaMan123 Jan 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented Jan 3, 2023

ShaMan123 left a comment • edited Loading

Choose a reason for hiding this comment

ShaMan123 commented Jan 3, 2023 • edited Loading

ShaMan123 commented Jan 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 commented Jan 3, 2023 • edited Loading

ShaMan123 commented Jan 3, 2023 • edited Loading

ShaMan123 commented Jan 3, 2023

ShaMan123 commented Jan 3, 2023 • edited Loading

ShaMan123 commented Jan 3, 2023 • edited Loading

asturur commented Jan 5, 2023

asturur commented Jan 5, 2023

asturur commented Jan 6, 2023

fix(IText): NO clear context top during rendering #8560

fix(IText): NO clear context top during rendering #8560

ShaMan123 commented Jan 2, 2023 •

edited

Loading

github-actions bot commented Jan 2, 2023 •

edited

Loading

github-actions bot commented Jan 2, 2023 •

edited

Loading

ShaMan123 Jan 2, 2023 •

edited

Loading

asturur Jan 3, 2023 •

edited

Loading

ShaMan123 left a comment •

edited

Loading

ShaMan123 commented Jan 3, 2023 •

edited

Loading

asturur Jan 3, 2023 •

edited

Loading

ShaMan123 commented Jan 3, 2023 •

edited

Loading

ShaMan123 commented Jan 3, 2023 •

edited

Loading

ShaMan123 commented Jan 3, 2023 •

edited

Loading

ShaMan123 commented Jan 3, 2023 •

edited

Loading