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.

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

Conversation

zhang759740844
Copy link

@zhang759740844 zhang759740844 commented Jun 26, 2024

Description

hey, I am using the textarea to show a long article with some annotation. And I found if I want to enter new lines at the end of the textarea whose parent element is overflow hidden, the position of hiddenTextarea will exceed the boundaries of document. And it make whole element shift up.

It seems to be similar to issue #9886, but he does't replicate the bug, so I make a example
https://jsfiddle.net/zacharyzch/L4sy3gu1/134/

here's the screenshot:

Jun-26-2024.16-35-18.mp4

In Action

  • In _calcTextareaPosition, add a additional check at last, to check the position of hiddenTextarea
  • remove the default border and marginbottom of the hiddenTextarea,to make the height of hiddenTextarea is equal to charHeight

and here’s the improved result:

new2.mp4

Copy link

codesandbox bot commented Jun 26, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@zhang759740844 zhang759740844 changed the title fix(): ensure the position of hiddenTextarea stay within the boundaries… fix(textarea): ensure the position of hiddenTextarea stay within the boundaries… Jun 26, 2024
(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.

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

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

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.

@asturur
Copy link
Member

asturur commented Jul 3, 2024

I didn't forget about this, i have been doing other stuff

@mhieupham1
Copy link

mhieupham1 commented Jul 12, 2024

i think the issue in your css styles, not fabricjs bug, why it scroll when a new line is added, it is for touchscreen devices use virtual keyboard

@zhang759740844
Copy link
Author

i think the issue in your css styles, not fabricjs bug, why it scroll when a new line is added, it is for touchscreen devices use virtual keyboard

I don't think it's a CSS issue. Setting the outermost div to overflow:hidden is a common practice. Moreover, the same problem exists on mobile devices as well.

@asturur
Copy link
Member

asturur commented Jul 12, 2024

Sorry for not coming back yet

@zhang759740844
Copy link
Author

Is there any movement on this? ToT

@asturur
Copy link
Member

asturur commented Aug 21, 2024

No but as you see is top of the list of the PRs.
The issue with this PR is that testing is a mess.
I need to setup usecases where the logic can be tried

@lecramr
Copy link

lecramr commented Aug 29, 2024

I think I also encountered this bug now in real world, to be print conform I have a pretty large canvas, and scale it down for the user to view it better via css transform scale and now this also happens to me.

@asturur
Copy link
Member

asturur commented Aug 29, 2024

This needs to be fixed and tested properly.
What blocks me to do so is that i need so many test cases and devices to do so is an enormous amount of time.
I need the test cases first of all. I would appreciate if someone could build a bunch of html layouts that we can keep around to test those cases.
We need to consider with scrolling, without scrolling, in a modal style div, in a scrolled page.

I usually add a file in the testcases folder, but in this case we need to change the html layout and i don't have a quick way to do that

@YoniZiv
Copy link

YoniZiv commented Oct 17, 2024

Not sure if it's related, but I'm experiencing something that might be related.
I have a fabric editor that's wrapped with a zoom container that I created (the container adjusts the content to fit 16:9 or 9:16 then sets the container's scale (using transform)
Whenever I have a textfield at the bottom of the page and I edit it "enlarges" the container and adds whitespace that shouldn't be there.
I monitored the event with the performance tab and it seems that the initHiddenTextArea calls updateTextareaPosition and I guess that's what's causing the shift

image

image

@zhang759740844
Copy link
Author

Not sure if it's related, but I'm experiencing something that might be related. I have a fabric editor that's wrapped with a zoom container that I created (the container adjusts the content to fit 16:9 or 9:16 then sets the container's scale (using transform) Whenever I have a textfield at the bottom of the page and I edit it "enlarges" the container and adds whitespace that shouldn't be there. I monitored the event with the performance tab and it seems that the initHiddenTextArea calls updateTextareaPosition and I guess that's what's causing the shift

image

image

It seems like the same issue.
Since this PR hasn't been merged, you might try resolving it with the following trick.
You need to check the source code of the fabric version you are using firstly. In my version, updateTextareaPosition calls _calcTextareaPosition. And in this method, it will calculate the position of hiddenTextarea
image

You can create a subclass of a Textbox and then override the _calcTextareaPosition method. Add the code inside the red box to the original method to ensure the hiddenTextArea doesn't exceed the screen.
image
Use this subclass to replace the Textbox

And there are some style adjustments as well, but see if you can roughly solve your problem first.

@zhang759740844
Copy link
Author

This needs to be fixed and tested properly. What blocks me to do so is that i need so many test cases and devices to do so is an enormous amount of time. I need the test cases first of all. I would appreciate if someone could build a bunch of html layouts that we can keep around to test those cases. We need to consider with scrolling, without scrolling, in a modal style div, in a scrolled page.

I usually add a file in the testcases folder, but in this case we need to change the html layout and i don't have a quick way to do that

Sorry, I missed this message. I can help you,but I don't know how to work together with you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants