Skip to content

[Editor] Don't scroll when drawing (issue 17327) #19338

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

Jump to bottom
Merged
merged 1 commit into from
Jan 23, 2025
Merged

[Editor] Don't scroll when drawing (issue 17327) #19338

merged 1 commit into from
Jan 23, 2025

Conversation

avdoseferovic
Copy link
Contributor

This patch is taken from #17688 which fixes the issue for Chrome. I'd need more info from @calixteman if this patch is fine.

After

Screen.Recording.2025-01-17.at.13.47.54.mp4

Before

Screen.Recording.2025-01-17.at.13.49.00.mp4

@calixteman
Copy link
Contributor

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/a19122a9171aabb/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/0b0060456946c74/output.txt

Snuffleupagus
Snuffleupagus requested changes Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The commit message doesn't include a reference to an issue (which it should), so I'm guessing that this is related to #17327?

If so, this is now the third time that that issue is being fixed. Hence this really needs a test-case to ensure that this won't regress yet again.

@avdoseferovic avdoseferovic force-pushed the fix/ink-editor-jumping branch from 4cbcfb5 to 5fef0b8 Compare January 17, 2025 14:23
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/0b0060456946c74/output.txt

Total script time: 10.77 mins

  • Integration Tests: FAILED

@avdoseferovic
Copy link
Contributor Author

The commit message doesn't include a reference to an issue (which it should), so I'm guessing that this is related to #17327?

If so, this is now the third time that that issue is being fixed. Hence this really needs a test-case to ensure that this won't regress yet again.

hey @Snuffleupagus thanks for the review. I have changed the commit message to include the issue number #17688.

I'd be happy to write the test myself, do you think it should be done within this PR or does it need a separate one?

@calixteman
Copy link
Contributor

It should be possible to write an integration test which check the position of the first page, click on drawing button, draw something and the check that the position of the page is the same.
@avdoseferovic would you mind to add a test in https://github.com/mozilla/pdf.js/blob/master/test/integration/ink_editor_spec.mjs ?

@avdoseferovic
Copy link
Contributor Author

It should be possible to write an integration test which check the position of the first page, click on drawing button, draw something and the check that the position of the page is the same. @avdoseferovic would you mind to add a test in https://github.com/mozilla/pdf.js/blob/master/test/integration/ink_editor_spec.mjs ?

Yeah, I'd be glad to. thanks for the hint!

@Snuffleupagus
Copy link
Collaborator

I have changed the commit message to include the issue number #17688.

But, that's a PR. It really seems more appropriate to reference issue #17327.

@avdoseferovic avdoseferovic force-pushed the fix/ink-editor-jumping branch from 5fef0b8 to 0fc5641 Compare January 17, 2025 14:40
@avdoseferovic
Copy link
Contributor Author

I have changed the commit message to include the issue number #17688.

But, that's a PR. It really seems more appropriate to reference issue #17327.

sorry, clumsy me. should be fine now

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/a19122a9171aabb/output.txt

Total script time: 26.19 mins

  • Integration Tests: FAILED

@avdoseferovic avdoseferovic force-pushed the fix/ink-editor-jumping branch from 0fc5641 to 8b1e22e Compare January 17, 2025 14:56
@avdoseferovic
Copy link
Contributor Author

the test case is passing locally: macOS on firefox:
.TEST-PASSED | must check that the page position remains the same after drawing

I decided to use Math.round because the x and y coordinates are integers on Firefox and floats on chrome

@avdoseferovic avdoseferovic force-pushed the fix/ink-editor-jumping branch from 8b1e22e to 144de2d Compare January 17, 2025 15:05
calixteman
calixteman reviewed Jan 17, 2025
View reviewed changes
@calixteman
Copy link
Contributor

the test case is passing locally: macOS on firefox: .TEST-PASSED | must check that the page position remains the same after drawing

I decided to use Math.round because the x and y coordinates are integers on Firefox and floats on chrome

Aren't the values before/after drawing the same (float or not) ?

@avdoseferovic
Copy link
Contributor Author

the test case is passing locally: macOS on firefox: .TEST-PASSED | must check that the page position remains the same after drawing
I decided to use Math.round because the x and y coordinates are integers on Firefox and floats on chrome

Aren't the values before/after drawing the same (float or not) ?

good point. I've removed the Math.round as its unnecessary, especially if we programatically move the drawing cursor by a specific x or y.

@Snuffleupagus
Copy link
Collaborator

Please squash the commits, since we don't use separate fixup commits in this project.

@avdoseferovic avdoseferovic force-pushed the fix/ink-editor-jumping branch from 7704746 to 78f612f Compare January 18, 2025 09:12
@Snuffleupagus
Copy link
Collaborator

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/9466667843b396c/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/e7dd3329c3ad771/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/9466667843b396c/output.txt

Total script time: 10.76 mins

  • Integration Tests: FAILED

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/e7dd3329c3ad771/output.txt

Total script time: 26.82 mins

  • Integration Tests: FAILED

@Snuffleupagus Snuffleupagus changed the title fix: don't scroll when drawing [Editor] Don't scroll when drawing (issue 17327) Jan 21, 2025
calixteman
calixteman approved these changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@calixteman calixteman merged commit f798bad into mozilla:master Jan 23, 2025
9 checks passed
@avdoseferovic avdoseferovic deleted the fix/ink-editor-jumping branch March 17, 2025 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus requested changes

@calixteman calixteman calixteman approved these changes

Assignees
No one assigned
Labels
editor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@avdoseferovic @calixteman @moz-tools-bot @Snuffleupagus