Fix Shift or Ctrl Key cause the drawn line is selected with additional line and can't be removed

[siddharthisrani]

Fixes #17829

Description

This PR addresses the issue where pressing Shift or Ctrl while drawing would select previous drawings and create additional lines that couldn't be removed. The solution implements the ability to draw straight lines when Shift or Ctrl is pressed, while preventing unintended interactions with existing drawings.

Approach

• Modified canvasPointerdown to detect Shift/Ctrl and set a flag for straight line drawing
• Updated #draw method to handle straight line drawing separately from freehand drawing
• Adjusted #stopDrawing to properly finalize straight lines

Testing

I've tested this change by:

• Drawing freehand lines without modifier keys
• Drawing straight lines with Shift and Ctrl keys
• Verifying that previous drawings are not selected when starting a new drawing
• Ensuring that all drawn lines (freehand and straight) can be properly deleted

Video Demo

InShot_20240725_164749209.mp4

Please review and let me know if any further changes are needed.

[siddharthisrani]

Hi @timvandermeij ,

I wanted to follow up on the pull request I submitted for the Shift/Ctrl key drawing issue (#17829). The changes should fix the problem with selecting previous drawings and ensure consistent drawing behavior with the Shift and Ctrl keys.

I've tested everything and included a video demo. Could you please review the PR ?

Thank you!

[timvandermeij]

I have triggered the CI for this patch and it found some linting issues, a few of which could indicate bugs. Please fix those first by running npx gulp lint --fix.

Moreover, such a change in ink editor behavior will need an integration test in https://github.com/mozilla/pdf.js/blob/master/test/integration/ink_editor_spec.mjs to automatically verify that the new behavior works and so it can't accidentally regress in future refactoring. You can use the existing tests there as inspiration for how to write the new test.

Once the above is in place we can review this further. Thanks.

[siddharthisrani]

Hi @timvandermeij ,

Thank you for your feedback. I will address the linting issues and add the necessary integration tests as requested. I'll update the PR once these changes are made.

[Snuffleupagus]

Unfortunately there's a number of issues with this patch, based on a quick look:

• It fails linting.
• It fails a bunch of existing integration tests; please don't submit patches that fail linting/testing.
• It doesn't include any new tests.
• It contains various commented-out code, which is not allowed. Most likely some of those cases are also related to the failing tests.
• It's somewhat difficult, at least to me, to understand the changes being made here.
• Please remember that when you add code comments they should be complete sentences, i.e. end with a period.
• Also, please remember to write good commit messages (look at the git history to see how those are usually written).

[siddharthisrani]

Hii @Snuffleupagus,

Thank you for your feedback.
I have fixed the linting issues and removed the commented-out code. I appreciate your guidance and will ensure to address all other points mentioned in future submissions, including adding new tests, ensuring all tests pass and maintaining good commit message practices.

[siddharthisrani]

Hii @timvandermeij,

I've fixed the linting issues by running npx gulp lint --fix. However, I'm encountering errors when trying to add the integration test for the ink editor behavior.

Could you please provide some guidance on how to properly write and implement the new test in test/integration/ink_editor_spec.mjs? I've looked at the existing tests, but I'm still having trouble ensuring that the new behavior is correctly verified.

Thanks for your help.

[timvandermeij]

The integration tests basically perform the same actions that a user would take in the UI and allow us to make assertions about the state of the viewer. In this case, I would expect an integration test to:

• Open the viewer.
• Open a PDF file.
• Draw a straight line using the Ctrl and Shift keys
• Assert that the straight line is visible, of the right size and positioned at the right coordinates.

This integration test should fail on the current master branch and pass on your branch.

You can use something like https://github.com/mozilla/pdf.js/blob/master/test/integration/ink_editor_spec.mjs#L157-L170 to draw the line using mouse actions, just like a user would in the browser, and then something like https://github.com/mozilla/pdf.js/blob/master/test/integration/ink_editor_spec.mjs#L129-L133 to get the ink annotation rectangle to make assertions about its size and coordinates.

The Puppeteer documentation, which is the browser testing framework we use, is a very useful reference when writing integration tests to know which actions are possible; please see https://pptr.dev/api/puppeteer.mouse.click and the other sections in the sidebar to find out what you can do with simulating mouse actions, keyboard actions, et cetera.

[siddharthisrani]

Hi @timvandermeij ,

Thanks a lot for the detailed instructions!

I'll go ahead and use the provided examples to simulate the drawing actions and validate the ink annotation. I'll refer to the Puppeteer documentation for additional guidance on simulating user interactions.