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

(WIP)Add text wrapping to FreeText editors (issue 18191) #19345

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

Conversation

avdoseferovic
Copy link
Contributor

I'm aware that there is already an PR open with the similar promise, but the implementation is different.

Note: please read the gist comments https://gist.github.com/bennadel/033e0158f47bff9e066016f99567ebba
This solution might face performance issues with very large text being added.

Fixes: #18191

Before
Screen.Recording.2025-01-18.at.23.24.58.mov
After
Screen.Recording.2025-01-18.at.23.26.22.mov

@Snuffleupagus Snuffleupagus changed the title feat: add text wraping to freetext [18191] Add text wrapping to FreeText editors (issue 18191) Jan 19, 2025
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Please improve the commit message (see the commit history of the project for examples of how we usually write them):

  • Note the updated PR title, which is thus also a good "title" for the commit message.
  • Generally you want more than a single line in the commit message, such that relevant context is available also on the Git command line.

Comment on lines 694 to 696
// Not sure why we need the -16px but it's neccessary to prevent
// text shifting when user finishes typing.
this.div.style.maxWidth = `calc(100% - ${this.div.style.left} - 16px)`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks too "magical", and really needs to be properly understood (and improved).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea behind this is to subtract the left position (which is already a percentage) from 100% and that way we always ensure that the text editor will not go out of bounds(and will be forced to wrap into the next line).

I still need to find why the 16px is needed, maybe the internal padding of the editor also has effect on the max-width property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or better visually explained, we end up with this: (this is only when you click on the right edge)
Screenshot 2025-01-19 at 20 30 38

src/display/editor/freetext.js Outdated Show resolved Hide resolved
@avdoseferovic avdoseferovic force-pushed the feat/add-text-wraping-to-freetext branch from cbb1889 to cd76eca Compare January 19, 2025 19:23
@calixteman
Copy link
Contributor

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/58c61e5873c6787/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/58c61e5873c6787/output.txt

Total script time: 0.86 mins

Published

Text wrapping is ensured by preventing the FreeText editor from going
out of bounds.

- max-width is a formula of (100% of the parentElement - the left % of the editor element)
- this ensures that clicking on the very right edge of the document does
- not create the editor out of bounds.
@avdoseferovic avdoseferovic force-pushed the feat/add-text-wraping-to-freetext branch from cd76eca to 0208f0b Compare January 23, 2025 23:47
@avdoseferovic
Copy link
Contributor Author

@calixteman please do ignore this for now, my initial approach only worked when pages were not rotated. I tried extending it, but the resulting text after print/save is messed up

@avdoseferovic avdoseferovic changed the title Add text wrapping to FreeText editors (issue 18191) (WIP)Add text wrapping to FreeText editors (issue 18191) Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add wrapping/resizing for Freetext annotations
4 participants