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

PDFScriptingManager: Bind mousedown listener with capture=true #14722

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Mar 28, 2022

PDFScriptingManager uses the mousedown and mouseup listeners to keep track of whether the mouse pointer is pressed in the isDown flag. These listeners were registered to run during the bubbling phase of the event dispatch, which can be interrupted if any of the previous event listeners stopped the event propagation. An example of that is by GrabToPan in web/grab_to_pan.js.

Since the mousedown (and mouseup) listeners of PDFScriptingManager are free of side effects, and the intention is to always run them, it makes most sense to register them with the capture flag.

More details at #14721 (comment)
Fixes #14721

PDFScriptingManager uses the `mousedown` and `mouseup` listeners to keep
track of whether the mouse pointer is pressed in the `isDown` flag.
These listeners were registered to run during the bubbling phase of the
event dispatch, which can be interrupted if any of the previous event
listeners stopped the event propagation. An example of that is by
`GrabToPan` in web/grab_to_pan.js.

Since the mousedown (and mouseup) listeners of PDFScriptingManager are
free of side effects, and the intention is to always run them, it makes
most sense to register them with the capture flag.
@Rob--W Rob--W added the viewer label Mar 28, 2022
@Rob--W Rob--W requested a review from Snuffleupagus March 28, 2022 00:50
@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 28, 2022

What about all of the event listeners attached directly in the AnnotationLayer-code, as mentioned in #14721 (comment), are they not similarly affected?

Also, does this still work even if the handTool is enabled before the PDFScriptingManager-functionality has been fully initialized, by e.g. setting the cursorToolOnLoad preference to 1?

@Rob--W
Copy link
Member Author

Rob--W commented Mar 28, 2022

What about all of the event listeners attached directly in the AnnotationLayer-code, as mentioned in #14721 (comment), are they not similarly affected?

These elements are input elements or links, which are already being recognized as targets where panning should not activate (source is linked in my previous comment).

Also, does this still work even if the handTool is enabled before the PDFScriptingManager-functionality has been fully initialized, by e.g. setting the cursorToolOnLoad preference to 1?

Yes. The time of registration does not matter because PDFScriptingManager registers the event withwindow, whereas GrabToPan registers the event on the container element. The capturing phase of the event starts at window, then descends the DOM tree via document, html, all the way down to the container element, and (if event.stopPropagation() is not callee) then bubbles up again in the reverse order up to window again. Your concern about registration time would be valid indeed if the events were registered on the same target (which they don't), e.g. as in the example at https://stackoverflow.com/q/7398290/#7398447 (bonus: the answer below that post has a visualisation of how event propagation works).

@Snuffleupagus
Copy link
Collaborator

/botio integrationtest

@pdfjsbot
Copy link

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/a7e88adb963a226/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/92b11c3c5ab0091/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/a7e88adb963a226/output.txt

Total script time: 3.97 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/92b11c3c5ab0091/output.txt

Total script time: 7.48 mins

  • Integration Tests: Passed

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.

Let's do this, thank you!

Would it be feasible to add an integration-test for this issue, e.g. in https://github.com/mozilla/pdf.js/blob/master/test/integration/scripting_spec.js, where we first enable the handTool and then ensure that the intended validation runs when the field loses focus?

@Snuffleupagus Snuffleupagus merged commit fffce79 into mozilla:master Apr 15, 2022
@Rob--W
Copy link
Member Author

Rob--W commented Apr 18, 2022

Oops. I missed the question.

Would it be feasible to add an integration-test for this issue, e.g. in https://github.com/mozilla/pdf.js/blob/master/test/integration/scripting_spec.js, where we first enable the handTool and then ensure that the intended validation runs when the field loses focus?

It may be feasible to have a regression test for this issue, by simulating the specific events in a particular order. I don't see any unit tests for the isDown logic, i.e. the one that triggers the logic at

if (this._mouseState.isDown) {
, so having some would be very useful. I haven't written tests with that framework, so if you're willing to write tests I would gladly provide the review (to verify correctness of event order and test logic).

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.

Hand tool prevents custom JavaScript validation
3 participants