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.

Sign up for GitHub

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(textarea): ensure the position of hiddenTextarea stay within the boundaries… #9948

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(textarea): ensure the position of hiddenTextarea stay within the boundaries… #9948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/shapes/IText/ITextBehavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type { TOptions } from '../../typedefs';
import { getDocumentFromElement } from '../../util/dom_misc';
import { LEFT, RIGHT, reNewline } from '../../constants';
import type { IText } from './IText';
import { getFabricDocument } from '../../env';

/**
* extend this regex to support non english languages
Expand Down Expand Up @@ -616,6 +617,13 @@ export abstract class ITextBehavior<
p.x += this.canvas._offset.left;
p.y += this.canvas._offset.top;

const doc =
(this.canvas && getDocumentFromElement(this.canvas.getElement())) ||
getFabricDocument();
if (p.y > doc.body.clientHeight - charHeight) {
p.y = doc.body.clientHeight - charHeight;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how this doc.body.clientHeight - charHeight relates to maxHeight = upperCanvasHeight - charHeight that we check up before, and why we can't have a single max vertical limit and use that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/fabricjs/fabric.js/pull/9948/files#diff-355c433500e45e82cda2cb4339613f1dacbb1d6ef43d8b0b98bf8fd293712976R588

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maxHeight ensures that the position of the hiddenTextArea does not exceed the bounds of the canvas, which means the calculated absolute coordinates are relative to the canvas's position. However, our hiddenTextarea is appended to the document.body, and there's no problem as long as the canvas stays within the viewport. But if the canvas extends beyond the viewport, the absolutely positioned hiddenTextarea would exceed the viewport, causing the browser to shift all elements upward to ensure hiddenTextArea is visible on screen. So I add a final check to ensure the calcualted absolute coordinates not exceed the viewport.

Copy link
Author

@zhang759740844 zhang759740844 Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
The image on the left shows the scenario where maxHeight is effective, with hiddenTextarea displayed within the canvas boundary. The image on the right illustrates the situation where the canvas extends beyond the viewport; the hiddenTextarea meets the maxHeight condition, but it exceeds the range of the viewport, causing the browser to shift all elements upwards to ensure the hiddenTextArea is visible on the screen.

Copy link
Author

@zhang759740844 zhang759740844 Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, in my view, maxHeight should actually be expressed as maxCanvasHeight, then the canvas offset is added to obtain the true position:p.y += this.canvas._offset.top;. Therefore, I didn't use the same maxHeight to make the judgment, but instead added another check afterward to ensure thatthe existing logic is not affected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i also see the bad behaviour of the fiddle, but i would see it reproduced in a normal page to discuss it better. Maybe i can add a simple test case in the sandboxes of the repo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the move page up issue. What i do not understand is if the textarea is below the fold and you don't move the page up, i tried your fiddle in a number of ways. There are 2 issues here, one is that the page scrolls and one is that it scrolls also when the text is completely outside the canvas so it scrolls in the white.

Now i do think the textarea should follow the position until is on the html canvas element and make the page scroll, but scrolling where the canvas is not available is useless.

I can see how the scrolling part is more an app behaviour that is subject to opinions rather than a real bug - non bug.

This should be configurable in some way, but i m not certain i want to add it as a configurable options, maybe if there is a way to pass in some kind of limits for the textbox placement.

I'm not sure if I understand correctly. Do you mean you want me to scroll the canvas up when the textarea is about to go below the fold?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i also see the bad behaviour of the fiddle, but i would see it reproduced in a normal page to discuss it better. Maybe i can add a simple test case in the sandboxes of the repo

Thank you very much. I also hope to resolve this issue better and contribute a little to this project. ^_^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry i wrote my message in a confusing way.
Let me start a couple of files to discuss this better

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all, and I'll dive deeper into the source code as well. I'm looking forward to discussing this further.

}

return {
left: `${p.x}px`,
top: `${p.y}px`,
Expand Down
2 changes: 1 addition & 1 deletion src/shapes/IText/ITextKeyBehavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export abstract class ITextKeyBehavior<
const { top, left, fontSize } = this._calcTextareaPosition();
// line-height: 1px; was removed from the style to fix this:
// https://bugs.chromium.org/p/chromium/issues/detail?id=870966
textarea.style.cssText = `position: absolute; top: ${top}; left: ${left}; z-index: -999; opacity: 0; width: 1px; height: 1px; font-size: 1px; padding-top: ${fontSize};`;
textarea.style.cssText = `position: absolute; top: ${top}; left: ${left}; z-index: -999; opacity: 0; width: 1px; height: 1px; font-size: 1px; border: 0; padding-bottom: 0; padding-top: ${fontSize};`;

(this.hiddenTextareaContainer || doc.body).appendChild(textarea);

Expand Down