fix(): typeAssertions not respecting subclasses

[ShaMan123]

Motivation

Was working on a subclass of IText when selection rendering started having bugs, being cleared because passing this condition (should pass isInteractiveTextObject and isEditing):

fabric.js/src/canvas/Canvas.ts

Lines 890 to 896 in 0f0ed96

	!(
	isInteractiveTextObject(this._activeObject) &&
	this._activeObject.isEditing
	)
	) {
	this.renderTop();
	}

fabric.js/src/util/typeAssertions.ts

Lines 54 to 60 in 0f0ed96

	export const isInteractiveTextObject = (
	fabricObject?: FabricObject
	): fabricObject is IText | Textbox => {
	// we could use instanceof but that would mean pulling in Text code for a simple check
	// @todo discuss what to do and how to do
	return !!fabricObject && fabricObject.isType('IText', 'Textbox');
	};

Of course it will because types are deprecated and the name of the class doesn't match.

Indeed this line is more types than logic but the rest aren't

Description

Came up with a solution in the form of static TAGS and an is method that is extremely similar to isType

Changes

Gist

There is no decent way for me to test this

In Action

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	915.877 (+0.659)	304.296 (+0.220)

[github-actions]

Coverage after merging fix-type-assertions into master will be

83.67%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	47.83%	100%	25%	60%	17, 20, 23, 40, 43, 46
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	10
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.21%	91.89%	90%	93.33%	116, 127, 136, 29, 92
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	107, 107, 107, 109, 111, 113–115, 117–120, 127–128, 135, 137, 22–23, 31–35, 39–43, 50–53, 61–65, 67, 75, 75, 75, 75, 75–76, 78, 78, 78–81, 83, 91–92, 94, 96–98
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	106, 106, 106, 106, 106–107, 109–110, 117–118, 120, 122–126, 135, 139–140, 140, 148, 148, 148–151, 153–156, 16, 160–161, 163, 165–168, 17, 171, 178–179, 181, 183–184, 186, 19, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 21, 21, 211, 22–23, 27, 36, 43, 50, 57, 64, 83–85, 93–95, 97–98
src/canvas
Canvas.ts	78.87%	77.54%	81.67%	79.41%	1001–1002, 1002, 1002–1004, 1006–1007, 1007, 1007, 1009, 1017, 1017, 1017–1019, 1019, 1019, 1025–1026, 1034–1035, 1035, 1035–1036, 1041, 1043, 1074–1076, 1079–1080, 1084–1085, 1198–1200, 1203–1204, 1277, 1396, 1519, 1589, 162, 187, 297–298, 301–305, 310, 333–334, 339–344, 364, 364, 364–365, 365, 365–366, 37, 374, 379–380, 380, 380–381, 383, 392, 398–399, 399, 399, 41, 442, 450, 454, 454, 454–455, 457, 539–540, 540, 540–541, 547, 547, 547–549, 569, 571, 571, 571–572, 572, 572, 575, 575, 575–576, 579, 588–589, 591–592, 594, 594–595, 597–598, 610–611, 611, 611–612, 614–619, 625, 632, 669, 669, 669, 671, 673–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 742, 742, 800–801, 801, 801–802, 804, 807–808, 808, 808–809, 811–812, 815, 815–817, 820–821, 891, 903, 910, 931, 963, 984–985
SelectableCanvas.ts	94.39%	91.16%	94.64%	96.62%	1119, 1119–1120, 1123, 1143, 1143, 1201, 1254–1255, 1276, 1284, 1409, 1411, 1413–1414, 518, 698–699, 701–702, 702, 702, 751–752, 813–814, 867–869, 901, 906–907, 934–935
StaticCanvas.ts	96.86%	92.91%	100%	98.61%	1102–1103, 1103, 1103–1104, 1224, 1234, 1288–1289, 1292, 1327–1328, 1405, 1414, 1414, 1418, 1418, 1465–1466, 310–311, 328, 759, 771–772
TextEditingManager.ts	100%	100%	100%	100%
src/color
Color.ts	92.16%	86.49%	100%	94.29%	314–315, 319–320, 323–324, 42, 72–73, 73, 75, 75, 75–76, 78–79
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
Control.ts	93.90%	88.89%	90.91%	97.73%	232, 319, 319, 354
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	5.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.ts	19.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.ts	93.57%	92.94%	100%	93.67%	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/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	100%	100%	100%	100%
node.ts	74.07%	33.33%	66.67%	88.89%	27, 31–32, 32, 32, 37
src/filters
BaseFilter.ts	21.62%	23.21%	32%	18.27%	100, 100, 100–101, 108–111, 111, 111–112, 118, 118, 118–121, 139, 157, 171–176, 180–1

[ShaMan123]

looks ok

[asturur]

mmm this is like isType but different.
The solution would be instanceOf that we are not using because it makes impossible to split the classes for which we have a type assertion.

I would prefer we detect the classes by a specific class property instead of having our own tag system.

So the bug is that this isType can't work because of course your subclass name is different.
an text interactive object has the enterEditing method for example and i m sure the activeSelection has something like that.
Can't we go on that direction?

I don't want to add custom systems to do this.

[ShaMan123]

Hmm...
But then a class can have enterEditing on it but not be a text object.
What about a dummy method then?
isInteractiveText():void so we check for that?

[ShaMan123]

Well if we manage one day to flatten the object tree instanceof will be possible. But until then...

[asturur]

the empty methods could be good enough but is just like a tag.
Right now we have an issue that is that legit subclasses won't work.
So we need to fix.
The class that will have enterEditing but won't be a text class is not an issue of today. We can also take something less generic than enterEditing.

[ShaMan123]

I have a different concept, using class registry. I will POC

[ShaMan123]

So the concept isn't that good
Simply moving the tag concept to the registry.
I don't like the methods thing because it feels unsafe but I don't have a better concept.

[asturur]

@ShaMan123 it feels unsafe but is not.
When is really unsafe we find a better fix.
Fix you detection issue using a method, don't refactor all the code around for it at this stage

[ShaMan123]

I see it as a design issue.
And after looking closely I found must of these methods part of bad design.
Left overs from pre v6.
And most are changesI wanted/tried in the past.