fix: sceneCoordsToViewportCoords, jumping text when there is an offset (#4413) #4630

zsviczian · 2022-01-22T12:14:58Z

This resolves #4413 and fixes the incorrect sceneCoordsToViewportCoords function.

Before

old.mp4

After

new.mp4

Text edit events

Zoom to fit api

On drop event

fixed zoom to max when empty, and target null

Update App.tsx

Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com>

vercel · 2022-01-22T12:15:04Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/6waJcPj6ZB2JBKkSx23s8kSY2EMG
✅ Preview: https://excalidraw-git-fork-zsviczian-fix-jumpingtext-excalidraw.vercel.app

…ext" This reverts commit ef7e1e4, reversing changes made to 2aea28f.

zsviczian · 2022-01-23T15:42:14Z

@ad1992, this simplifies and resolves the errors (and heuristic "magic") with text positioning in textwysiwyg. This change greatly improves the user experience for Excalidraw when it is rendered embedded in an app with offsetLeft & offsetTop !=0.

Please take a look and let's push this important improvement to production.

dwelle · 2022-01-23T16:37:24Z

src/element/textWysiwyg.tsx

-          Number(
-            getComputedStyle(
-              excalidrawContainer?.parentNode as Element,
-            ).marginRight.slice(0, -2),
-          );


note sure about what this solved before, but shouldn't w keep this?

I removed this deliberately and after extensive testing. I think it is based on the misunderstanding that appState.width is the entire screen width, while in reality, it is the canvasDOMWidth. Subtracting the right-hand side margin is unnecessary.

excalidraw/src/components/App.tsx

Lines 380 to 395 in c009e03

const {

width: canvasDOMWidth,

height: canvasDOMHeight,

viewModeEnabled,

} = this.state;

const canvasWidth = canvasDOMWidth * canvasScale;

const canvasHeight = canvasDOMHeight * canvasScale;

if (viewModeEnabled) {

return (

<canvas

className="excalidraw__canvas"

style={{

width: canvasDOMWidth,

height: canvasDOMHeight,

cursor: CURSOR_TYPE.GRAB,

}}

ad1992 · 2022-01-23T18:25:54Z

This is a great improvement @zsviczian ❤️. Could you also write specs for this ? Will do one round of testing and then should be good to merge.

zsviczian · 2022-01-23T18:28:31Z

This is a great improvement @zsviczian ❤️. Could you also write specs for this ? Will do one round of testing and then should be good to merge.

Great! A second (and 3rd, 4th...) round of testing is always essential. What do you mean by specs?

ad1992 · 2022-01-23T18:32:24Z

This is a great improvement @zsviczian ❤️. Could you also write specs for this ? Will do one round of testing and then should be good to merge.

Great! A second (and 3rd, 4th...) round of testing is always essential. What do you mean by specs?

I meant tests for the change just to make sure we don't break in future as this part has been broken often in the past.

zsviczian · 2022-01-23T18:41:35Z

This is a great improvement @zsviczian ❤️. Could you also write specs for this ? Will do one round of testing and then should be good to merge.

Great! A second (and 3rd, 4th...) round of testing is always essential. What do you mean by specs?

I meant tests for the change just to make sure we don't break in future as this part has been broken often in the past.

I think this broke frequently in the future because there was a formula error. sceneCoordsToViewportCoords was wrong, and we were trying to resolve its impact by applying complex calculations in textwysiwyg. That should be resolved for good.

Regarding testing... I don't really know how to test for zoom and offset situations... I haven't yet looked at excalidraw tests, I'll take a look to see if this sort of testing can be automated. My recommendation would be to deploy a second app with every change, the second should have offsets and righthand side margin.

ad1992 · 2022-01-23T19:03:49Z

This is a great improvement @zsviczian ❤️. Could you also write specs for this ? Will do one round of testing and then should be good to merge.

Great! A second (and 3rd, 4th...) round of testing is always essential. What do you mean by specs?

I meant tests for the change just to make sure we don't break in future as this part has been broken often in the past.

I think this broke frequently in the future because there was a formula error. sceneCoordsToViewportCoords was wrong, and we were trying to resolve its impact by applying complex calculations in textwysiwyg. That should be resolved for good.

Regarding testing... I don't really know how to test for zoom and offset situations... I haven't yet looked at excalidraw tests, I'll take a look to see if this sort of testing can be automated.

Sure 👍🏻 . The tests would probably be tricky for this but lets give it a try and decide accordingly :).

My recommendation would be to deploy a second app with every change, the second should have offsets and righthand side margin.

What do you mean by deploying an app with offsets and right handed margin ? We have the same code sandbox example running with package example in port 3001 which can be used for testing

zsviczian · 2022-01-24T11:57:49Z

What do you mean by deploying an app with offsets and right handed margin ? We have the same code sandbox example running with package example in port 3001 which can be used for testing

To make the question concrete. If I would want to test a PR with screen offsets, how can I do that. For sake of example, the current preview release for the hyperlinks PR is https://excalidraw-f13roaq8h-excalidraw.vercel.app/ I tried https://excalidraw-f13roaq8h-excalidraw.vercel.app:3001/ but that did not bring up anything...

ad1992 · 2022-01-24T12:35:31Z

What do you mean by deploying an app with offsets and right handed margin ? We have the same code sandbox example running with package example in port 3001 which can be used for testing

To make the question concrete. If I would want to test a PR with screen offsets, how can I do that. For sake of example, the current preview release for the hyperlinks PR is https://excalidraw-f13roaq8h-excalidraw.vercel.app/ I tried https://excalidraw-f13roaq8h-excalidraw.vercel.app:3001/ but that did not bring up anything...

Ohh ok I meant it will work in localhost:3001, we can also deploy it as well so its available in preview too

zsviczian · 2022-01-28T07:03:46Z

Ohh ok I meant it will work in localhost:3001, we can also deploy it as well so its available in preview too

That would be great! I was looking around the repository but didn't yet have the patience to figure it out. If you give me some pointers I am also happy to propose a PR with port 3001 being available in preview too.

BTW Have you had time to test this fix? I've deployed this to the Obsidian Excalidraw build a week ago and have not experienced any issues since. I am pretty confident that this is working as expected. Would be good to get this deployed before further functions build on the current erroneous sceneCoordsToViewportCoords() function.

ad1992

+1 from my side, tested it out and it works great, thanks @zsviczian for fixing this 💯 , lets wait for @dwelle's +1 as well and then we are good to 🚀

dwelle · 2022-01-29T13:26:45Z

For sure, let's merge! Thanks!

zsviczian and others added 30 commits July 30, 2021 08:52

onBeforeTextEdit and onBeforeTextSubmit

9da58e5

onBeforeTextEdit and onBeforeTextSubmit

b101b32

onBeforeTextEdit and onBeforeTextSubmit

40c53eb

onBeforeTextEdit and onBeforeTextSubmit

7f60fbd

onBeforeTextEdit and onBeforeTextSubmit

dc28210

onBeforeTextEdit & onBeforeTextSubmit

d7854c2

removed "canvas": "2.8.0",

03e2408

added null check for onBeforeTextSubmit result

f89a6eb

Merge branch 'excalidraw:master' into master

6c0ab9c

Delete yarn.lock

da4501c

revert yarn.lock

df012b3

Merge pull request #3 from zsviczian/textEditEvents

5169d3a

Text edit events

zoomToFit

30aea3b

readme corrected

bc8e2e5

Update README.md

482b4cf

Update README_NEXT.md

779cfe2

Merge pull request #4 from zsviczian/zoomToFit-api

4133679

Zoom to fit api

publish on npmjs

aab61f7

onDrop

04ee5d6

Update README_NEXT.md

74b199b

Merge pull request #5 from zsviczian/onDropEvent

beb8d83

On drop event

npm publis obsidian-5

e9daa53

fixed zoom to max when empty, and target null

3a9f819

Merge pull request #6 from zsviczian/zoomToFit-api

fcad887

fixed zoom to max when empty, and target null

Update App.tsx

85ea783

Update package.json

8454888

Merge pull request #7 from zsviczian/zoomToFit-api

53fca36

Update App.tsx

Webpack to package everything in a single file

fb1c49c

Update src/components/App.tsx

663534a

Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com>

Update CHANGELOG.md

72930e2

zsviczian added 7 commits January 22, 2022 10:49

Merge branch 'excalidraw:master' into master

f0793ee

merge

d5a5980

Merge branch 'excalidraw-master' into for-obsidian

3eff11a

merge upstream

6513eb0

Merge branch 'excalidraw-master' into for-obsidian

8c8f903

fixed

5fe0a0a

fixed

2aea28f

vercel bot deployed to Preview January 22, 2022 12:15 View deployment

zsviczian requested a review from dwelle January 22, 2022 12:22

Merge branch 'for-obsodian-fix-jumpingText' into fix-jumpingText

ef7e1e4

vercel bot had a problem deploying to Preview January 22, 2022 13:02 Failure

Revert "Merge branch 'for-obsodian-fix-jumpingText' into fix-jumpingT…

b2bb7ca

…ext" This reverts commit ef7e1e4, reversing changes made to 2aea28f.

vercel bot deployed to Preview January 22, 2022 13:04 View deployment

dwelle reviewed Jan 23, 2022

View reviewed changes

ad1992 approved these changes Jan 28, 2022

View reviewed changes

dwelle approved these changes Jan 29, 2022

View reviewed changes

dwelle merged commit e4edda4 into excalidraw:master Jan 29, 2022

ad1992 mentioned this pull request May 13, 2022

Viewport incorrectly calculated in textWysiwyg #4098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: sceneCoordsToViewportCoords, jumping text when there is an offset (#4413) #4630

fix: sceneCoordsToViewportCoords, jumping text when there is an offset (#4413) #4630

zsviczian commented Jan 22, 2022

vercel bot commented Jan 22, 2022 •

edited

zsviczian commented Jan 23, 2022

dwelle Jan 23, 2022

zsviczian Jan 23, 2022

ad1992 commented Jan 23, 2022

zsviczian commented Jan 23, 2022

ad1992 commented Jan 23, 2022

zsviczian commented Jan 23, 2022

ad1992 commented Jan 23, 2022 •

edited

zsviczian commented Jan 24, 2022

ad1992 commented Jan 24, 2022

zsviczian commented Jan 28, 2022 •

edited

ad1992 left a comment •

edited

dwelle commented Jan 29, 2022

	const {
	width: canvasDOMWidth,
	height: canvasDOMHeight,
	viewModeEnabled,
	} = this.state;
	const canvasWidth = canvasDOMWidth * canvasScale;
	const canvasHeight = canvasDOMHeight * canvasScale;
	if (viewModeEnabled) {
	return (
	<canvas
	className="excalidraw__canvas"
	style={{
	width: canvasDOMWidth,
	height: canvasDOMHeight,
	cursor: CURSOR_TYPE.GRAB,
	}}

fix: sceneCoordsToViewportCoords, jumping text when there is an offset (#4413) #4630

fix: sceneCoordsToViewportCoords, jumping text when there is an offset (#4413) #4630

Conversation

zsviczian commented Jan 22, 2022

Before

After

vercel bot commented Jan 22, 2022 • edited

zsviczian commented Jan 23, 2022

dwelle Jan 23, 2022

Choose a reason for hiding this comment

zsviczian Jan 23, 2022

Choose a reason for hiding this comment

ad1992 commented Jan 23, 2022

zsviczian commented Jan 23, 2022

ad1992 commented Jan 23, 2022

zsviczian commented Jan 23, 2022

ad1992 commented Jan 23, 2022 • edited

zsviczian commented Jan 24, 2022

ad1992 commented Jan 24, 2022

zsviczian commented Jan 28, 2022 • edited

ad1992 left a comment • edited

Choose a reason for hiding this comment

dwelle commented Jan 29, 2022

vercel bot commented Jan 22, 2022 •

edited

ad1992 commented Jan 23, 2022 •

edited

zsviczian commented Jan 28, 2022 •

edited

ad1992 left a comment •

edited