User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/115.0

Steps to reproduce:

Added a copy event listener to the built-in PDF viewer, expecting it to get called when a copy event happened, as was the behavior in prior versions of Firefox.

To reproduce:

1. Open a PDF document in firefox: https://browse.arxiv.org/pdf/1908.06809v2.pdf
2. Open the Firefox console.
3. Execute the following statement: document.addEventListener('copy', function(e){ console.debug("copy event", e) });
4. Select some text in the PDF document, right-click the selection and click "Copy". Alternatively press CTRL-C instead of using the context menu.

Actual results:

Nothing

Expected results:

The text "copy event" with the details of the event should have been logged to the firefox console, at debug level.

Note: Normal web pages that do not use the Firefox PDF viewer, do properly emit the event on copy, so the issue seems to relate to some recent change in the PDF viewer implementation itself. A few versions back (prior to 115.esr3) the PDF viewer did emit the copy event. Unfortunately I'm not sure exactly of the update this broke, so some bisecting would seem in order.

The Bugbug bot thinks this bug should belong to the 'Firefox::PDF Viewer' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

It's because of this patch:
https://github.com/mozilla/pdf.js/pull/16200
and especially:
https://github.com/mozilla/pdf.js/pull/16200/files#diff-579f2fb8055052aa97906d5fc75aeee84aa7dd9f8efcca24d8a5460d828e1de8R223-R224
We can probably remove these two lines and just capture the event to be sure to pass the correct value to the copy event.

Good (and quick) find!

There was nothing in the PR discussion that even touched on why the author decided to stop the event propagation, so either that was accidentally included in the PR (e.g. forgotten local experimentation not pruned before pushing the feature branch), or there was a specific intent behind those two lines. As long as the regression suite passes with those two lines removed, it'd seem safe. A new test to verify copy event isn't broken in the future might be useful, so as to squash this entire vector of bugs :)

Edit: after looking over the code some more, and checking how many other places pdf.js itself subscribes to the copy-event, it seems to be exactly zero (prior to the PR you linked). As such the copy event seems intended for users of the library. What I need it for is pretty much the same reason as the author of your linked PR, but for a different use case; I need to do other pre-processing than the author's arabic specific use case.

I have an external program that reacts to the clipboard data getting changed, and I'm not happy at all with that PR, since it breaks that program as well. It would get two events, one for the normalized arabic stuff of the linked PR, and then another event with the user-controlled changes expected.

I.e. the flow of events would be:

1. User selects some text.
2. User triggers copy.
3. The arabic PR copy event listener modifies the selected text and puts it on the clipboard.
4. External programs monitoring the clipboard would get the arabic normalized data.
5. The user's own copy event listener for "copy" would get triggered, putting its variant of the selected text on the clipboard.
6. External programs monitoring the clipboard would get another change event, now containing the result of the user-modified data (sans the arabic "fix", since the arabic fix is only added to the clipboard data, which is over-written by subsequent writes to the clipboard).

So to me it seems the PR is a poor solution to the problem described in the PR itself, as it robs users of the "copy" event from adjusting the selected content. The more I look at that code, the clearer it becomes that the PR author hadn't sufficiently reflected on the implications of that change, and just decided to rob users of the ability to react on the copy event by stopping the event propagation all together; monopolizing the arabic normalization pre-processing to be the only reaction to the copy event possible, closing down copy event subscriptions for all other use cases.

Now, as to your suggestion of just deleting those two lines, I think it makes sense. For the vast majority of users, all text copying would go through the author's arabic text normalization code, and that's what would end up on the clipboard, as expected by the author. For very specialized (and unusual) use cases, like mine, I'd not get the arabic fix (which is OK by me, since I've never needed it anyway), and would just have to ignore the first of two event triggers in my programs. I can live with this trade-off until the PR is properly implemented. At least it would give me a way to get the text to a different program, which is all I really need (even if it comes with some warts for now).