feat(env): support iframe with relative window/document #8897

ShaMan123 · 2023-05-07T09:26:01Z

Motivation

Description

Changes the event targets to the window/document containing the canvas element of the fabric canvas so it should work inside multiple iframes

Changes

Gist

In Action

https://codesandbox.io/p/sandbox/fabric-nextjs-sandbox-forked-6pkg55

With images: https://codesandbox.io/p/sandbox/fabric-nextjs-sandbox-forked-7y55kw?file=%2FREADME.md

Parcel.Sandbox.-.Google.Chrome.2023-05-07.20-53-06_Trim.mp4

github-actions · 2023-05-07T09:27:43Z

Build Stats

file / KB (diff)	bundled	minified
fabric	902.091 (+0.811)	304.377 (+0.268)

github-actions · 2023-05-07T10:30:37Z

Coverage after merging feat/rel-window into master will be

83.67%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 29, 32, 35
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
Pattern.ts	92.21%	91.89%	90%	93.33%	114, 138, 149, 158, 51
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%
typedefs.ts	100%	100%	100%	100%
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	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.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%	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.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.55%	96.63%	1113, 1113–1114, 1117, 1137, 1137, 1195, 1248–1249, 1270, 1278, 1403, 1405, 1407–1408, 512, 692–693, 695–696, 696, 696, 745–746, 807–808, 861–863, 895, 900–901, 928–929
StaticCanvas.ts	96.46%	92.11%	100%	98.36%	1112–1113, 1113, 1113–1114, 1234, 1244, 1298–1299, 1302, 1337–1338, 1415, 1424, 1424, 1428, 1428, 1475–1476, 1690–1691, 325–326, 343, 774, 786–787, 872
TextEditingManager.ts	100%	100%	100%	100%
src/color
Color.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
Control.ts	93.90%	88.89%	90.91%	97.73%	235, 322, 322, 357
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%
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	76.92%	50%	66.67%	88.24%	27, 31–32, 32, 32
src/filters
BaseFilter.ts	21.93%	22.81%	32%	19.05%	106–109, 109, 109–110, 116, 116, 116–119, 137, 155, 169–174, 178–179, 179, 179–182, 182, 182, 182, 182–184,

src/canvas/StaticCanvas.ts

asturur · 2023-05-07T11:09:57Z

I m mostly onboard apart naming.
getWindow and getElementWindow at this point are so much similar and is unclear when i should use wich one.

getWindow at this point is the window where fabric lives and would need to reflect that.

Instatiating a fabric element across an iframe seems so weird to me and dangerous.
For exampe images, if you use the loadImage functionality you have an image loaded in fabric window, and then you add it on canvas instance that is in the fabric window, but the draw command is going to act on a context that is in the iframe referencing pixels that are in the main window.

Would that work?

Should those be method of the canvas rather than utilities?

ShaMan123 · 2023-05-07T11:39:58Z

good point
I have expose getWindow/Document on canvas but didn't commit
Maybe we should have an env for each canvas and pass that around

ShaMan123 · 2023-05-07T11:44:58Z

~~https://codesandbox.io/s/hybc13~~ https://codesandbox.io/p/sandbox/fabric-nextjs-sandbox-forked-6pkg55 works, need to check all cases

asturur · 2023-05-07T15:22:38Z

Also let's add disclaimers about cross origin work probably being impossible.
Developers should not expect to iframe a website with a canvas and take over their canvas from another domain.

An env per canvas seems to much.
But if those functions always need the canvas element, they can live on the canvas instance.
Maybe is a good idea to differentiate them.

ShaMan123 · 2023-05-07T22:24:16Z

Also let's add disclaimers about cross origin work probably being impossible.
Developers should not expect to iframe a website with a canvas and take over their canvas from another domain.

this is why I think we should have an env scoped to canvas. That way when everything belongs to same domain.
I will try that out

ShaMan123 · 2023-05-08T00:04:22Z

I think best to do it in 2 steps. This PR as is and another one for the scoped env concept.

ShaMan123 · 2023-05-08T04:25:26Z

looks good: https://codesandbox.io/p/sandbox/fabric-nextjs-sandbox-forked-7y55kw?file=%2FREADME.md

ShaMan123 · 2023-05-08T04:26:13Z

I think this is good enough as a first step
Eventually we will make a guide for all envs

ShaMan123 · 2023-05-08T04:34:21Z

Instatiating a fabric element across an iframe seems so weird to me and dangerous. For exampe images, if you use the loadImage functionality you have an image loaded in fabric window, and then you add it on canvas instance that is in the fabric window, but the draw command is going to act on a context that is in the iframe referencing pixels that are in the main window.

Would that work?

https://codesandbox.io/p/sandbox/fabric-nextjs-sandbox-forked-sy6726

ShaMan123

Naming is left

src/shapes/IText/DraggableTextDelegate.ts

src/canvas/StaticCanvas.ts

ShaMan123 · 2023-05-08T04:43:39Z

I would like to add tests but I don't know how to tests that an iframe receives events etc.

asturur · 2023-05-08T07:17:00Z

I would like to add tests but I don't know how to tests that an iframe receives events etc.

We can do later with that new test suite probably. I didn't have time to write proper tests but seems exactly useful for those kind of high level interactions

asturur · 2023-05-08T07:30:37Z

Do not open follow up prs for this concept because if this works, all we need is just docs.

My initial comment still stand

I m mostly onboard apart naming.
getWindow and getElementWindow at this point are so much similar and is unclear when i should use wich one.

getWindow at this point is the window where fabric lives and would need to reflect that.

getWindow => getFabricWindow
getDocument => getFabricDocument
getElementDocument => getDocumentForElement (for/of)
getElementWindow => getWindowForElement (for/of)

asturur

would like to see a different naming scheme that makes the function apart.
The difference between elementDocument ( i think intented as element's document ) and the documentElement ( the root node of the document ) is too subtle

getWindow => getFabricWindow getDocument => getFabricDocument getElementDocument => getDocumentFromElement (for/of) getElementWindow => getWindowFromElement (for/of)

ShaMan123 · 2023-05-08T10:09:54Z

renamed used for instead of for/of

This reverts commit a180281.

getWindow => getFabricWindow getDocument => getFabricDocument getElementDocument => getDocumentFromElement (for/of) getElementWindow => getWindowFromElement (for/of)

ShaMan123

renamed, noisy diff

ShaMan123 · 2023-05-08T10:16:51Z

test/lib/event.simulate.js

I am pretty sure this is not correct

but it is outdated anyways.
What is it?

i don't remember.
Is probably some primitive way to fire events before JSDOM or maybe we still use it.

ShaMan123 added 2 commits May 7, 2023 18:00

step 1

34a3ac4

remove _createCanvasElement

11c4195

ShaMan123 added 4 commits May 7, 2023 18:31

Update DraggableTextDelegate.ts

6cdc20d

fix

d1efcef

Update ITextKeyBehavior.ts

0eb9235

fix tests (event target)

937304d

fix

b7f479f

asturur reviewed May 7, 2023

View reviewed changes

src/canvas/StaticCanvas.ts Show resolved Hide resolved

ShaMan123 linked an issue May 7, 2023 that may be closed by this pull request

getElementOffset is broken when used with shadow DOM #7979

Closed

Update CHANGELOG.md

a41b16c

Merge branch 'master' into feat/rel-window

ddd5af9

ShaMan123 changed the title ~~feat(env): relative window/document~~ feat(env): support iframe with relative window/document May 8, 2023

canvas init error

6e48007

ShaMan123 commented May 8, 2023

View reviewed changes

src/shapes/IText/DraggableTextDelegate.ts Outdated Show resolved Hide resolved

src/canvas/StaticCanvas.ts Show resolved Hide resolved

src/canvas/StaticCanvas.ts Show resolved Hide resolved

ShaMan123 requested a review from asturur May 8, 2023 04:41

asturur requested changes May 8, 2023

View reviewed changes

rename

a180281

getWindow => getFabricWindow getDocument => getFabricDocument getElementDocument => getDocumentFromElement (for/of) getElementWindow => getWindowFromElement (for/of)

ShaMan123 added 3 commits May 8, 2023 19:11

Revert "rename"

bef89f2

This reverts commit a180281.

Merge branch 'master' into feat/rel-window

e2721fe

rename

9196f42

getWindow => getFabricWindow getDocument => getFabricDocument getElementDocument => getDocumentFromElement (for/of) getElementWindow => getWindowFromElement (for/of)

ShaMan123 commented May 8, 2023

View reviewed changes

asturur approved these changes May 8, 2023

View reviewed changes

asturur merged commit fe8c42f into master May 8, 2023
18 checks passed

asturur deleted the feat/rel-window branch May 8, 2023 11:43

asturur mentioned this pull request May 19, 2023

Version 6.0.0-beta6 #8935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(env): support iframe with relative window/document #8897

feat(env): support iframe with relative window/document #8897

ShaMan123 commented May 7, 2023 •

edited

github-actions bot commented May 7, 2023 •

edited

github-actions bot commented May 7, 2023 •

edited

asturur commented May 7, 2023

ShaMan123 commented May 7, 2023

ShaMan123 commented May 7, 2023 •

edited

asturur commented May 7, 2023 •

edited

ShaMan123 commented May 7, 2023

ShaMan123 commented May 8, 2023

ShaMan123 commented May 8, 2023

ShaMan123 commented May 8, 2023

ShaMan123 commented May 8, 2023

ShaMan123 left a comment

ShaMan123 commented May 8, 2023

asturur commented May 8, 2023

asturur commented May 8, 2023

asturur left a comment

ShaMan123 commented May 8, 2023

ShaMan123 left a comment

ShaMan123 May 8, 2023

ShaMan123 May 8, 2023

asturur May 8, 2023

feat(env): support iframe with relative window/document #8897

feat(env): support iframe with relative window/document #8897

Conversation

ShaMan123 commented May 7, 2023 • edited

Motivation

Description

Changes

Gist

In Action

github-actions bot commented May 7, 2023 • edited

github-actions bot commented May 7, 2023 • edited

asturur commented May 7, 2023

ShaMan123 commented May 7, 2023

ShaMan123 commented May 7, 2023 • edited

asturur commented May 7, 2023 • edited

ShaMan123 commented May 7, 2023

ShaMan123 commented May 8, 2023

ShaMan123 commented May 8, 2023

ShaMan123 commented May 8, 2023

ShaMan123 commented May 8, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 commented May 8, 2023

asturur commented May 8, 2023

asturur commented May 8, 2023

asturur left a comment

Choose a reason for hiding this comment

ShaMan123 commented May 8, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 May 8, 2023

Choose a reason for hiding this comment

ShaMan123 May 8, 2023

Choose a reason for hiding this comment

asturur May 8, 2023

Choose a reason for hiding this comment

ShaMan123 commented May 7, 2023 •

edited

github-actions bot commented May 7, 2023 •

edited

github-actions bot commented May 7, 2023 •

edited

ShaMan123 commented May 7, 2023 •

edited

asturur commented May 7, 2023 •

edited