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

Don't steal the copy event (bug 1857823) #17099

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

Conversation

calixteman
Copy link
Contributor

There were no specific reasons to stop the propagation of the event and it can be useful for some third parties to catch the event to let them do their own stuff.

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.

There were no specific reasons to stop the propagation of the event [...]

Yes there was, since that allows us to prevent copying when PDF permissions are enabled; something that this patch unfortunately breaks!

More generally, do we actually need/want to "support" the use-case outlined in bug 1857823?
(The comments there seem to think that the changes were solely for arabic text, which unfortunately isn't really accurate, since it's a much broader change affecting copy-pasting in general e.g. ligatures and special symbols as well.)


To reproduce the problem, try changing the value of

enablePermissions: {
/** @type {boolean} */
value: false,
kind: OptionKind.VIEWER + OptionKind.PREFERENCE,
},
and then open the PDF document from https://bugzilla.mozilla.org/show_bug.cgi?id=792816#c0 in the viewer and note that copying is now incorrectly possible with this patch.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Oct 9, 2023

Furthermore, when I open https://github.com/mozilla/pdf.js/blob/master/test/pdfs/copy_paste_ligatures.pdf locally in the viewer after applying this patch, and manually select the text then copying is also "broken" without any normalization being done.

The existing integration-test for that PDF document uses select all, hence the code-path modified in this patch is never entered (as far as I can tell); hence I'd like to reiterate my question above if this is actually a desirable change in general?

@calixteman
Copy link
Contributor Author

Furthermore, when I open https://github.com/mozilla/pdf.js/blob/master/test/pdfs/copy_paste_ligatures.pdf locally in the viewer after applying this patch, and manually select the text then copying is also "broken" without any normalization being done.

The existing integration-test for that PDF document uses select all, hence the code-path modified in this patch is never entered (as far as I can tell); hence I'd like to reiterate my question above if this is actually a desirable change in general?

I don't see any good reasons to potentially "break" any web extensions which could run on top of pdf.js, so yes I think we should do that.

There were no specific reasons to stop the propagation of the event
and it can be useful for some third parties to catch the event to let
them do their own stuff.
@Snuffleupagus
Copy link
Collaborator

I don't see any good reasons to potentially "break" any web extensions which could run on top of pdf.js,

As far as I know, unless things have changed recently, web-extensions aren't allowed to run in the built-in Firefox PDF Viewer.

@calixteman
Copy link
Contributor Author

I don't see any good reasons to potentially "break" any web extensions which could run on top of pdf.js,

As far as I know, unless things have changed recently, web-extensions aren't allowed to run in the built-in Firefox PDF Viewer.

Interesting, I didn't know that, would you have any pointer about that ?

@Snuffleupagus
Copy link
Collaborator

I don't see any good reasons to potentially "break" any web extensions which could run on top of pdf.js,

As far as I know, unless things have changed recently, web-extensions aren't allowed to run in the built-in Firefox PDF Viewer.

Interesting, I didn't know that, would you have any pointer about that ?

It could obviously have changed since then, or I could be misunderstanding how things work (since I don't know much about web-extensions), but the only thing I'm aware of is https://bugzilla.mozilla.org/show_bug.cgi?id=1454760

@calixteman
Copy link
Contributor Author

Thank you for sharing this.
So I've no other argument than I don't see any reasons to not propagate the event.

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.

To ensure that we don't break the permission-handling, can you please also add two new tests (with select-all respectively manual-selection) that check that copying is prevented as expected?

The following PDF should be usable for that purpose: https://github.com/mozilla/pdf.js/blob/master/test/pdfs/issue9972-3.pdf

await closePages(pages);
});

it("must check that the ligatures have been removed when the text has been copied", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have a test-case in this file with the same exact name, which we've seen recently can be annoying when trying to track down intermittent issues; so please change this one slightly.

Comment on lines +255 to +256
if (browserName === "chrome") {
// In Firefox the copy event hasn't the correct value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a TODO with a link to a relevant upstream bug-report, either in Firefox or Puppeteer (whichever is appropriate), so that we can eventually enable this test everywhere?

await promise;
});

const promise = waitForEvent(page, "copy", 300000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is ten times larger than the Jasmine-timeout, is that intentional?

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Oct 9, 2023

Following-up on #17099 (review), we probably have to add a little bit of code to support testing the permissions stuff. The following is untested, however I do believe that this diff could work:

diff --git a/test/integration/test_utils.js b/test/integration/test_utils.js
index bc79c60d0..54f11daa5 100644
--- a/test/integration/test_utils.js
+++ b/test/integration/test_utils.js
@@ -13,7 +13,7 @@
  * limitations under the License.
  */
 
-exports.loadAndWait = (filename, selector, zoom, pageSetup) =>
+exports.loadAndWait = (filename, selector, zoom, pageSetup, hashOptions = "") =>
   Promise.all(
     global.integrationSessions.map(async session => {
       const page = await session.browser.newPage();
@@ -34,9 +34,17 @@ exports.loadAndWait = (filename, selector, zoom, pageSetup) =>
       });
 
       let url = `${global.integrationBaseUrl}?file=/test/pdfs/${filename}`;
+      const hash = [];
       if (zoom) {
-        url += `#zoom=${zoom}`;
+        hash.push(`zoom=${zoom}`);
       }
+      if (hashOptions) {
+        hash.push(hashOptions);
+      }
+      if (hash.length) {
+        url += `#${hash.join("&")}`;
+      }
+
       await page.goto(url);
       if (pageSetup) {
         await pageSetup(page);
diff --git a/web/app.js b/web/app.js
index d5675e578..9b6c5bc3f 100644
--- a/web/app.js
+++ b/web/app.js
@@ -346,6 +346,12 @@ const PDFViewerApplication = {
     if (params.has("disablehistory")) {
       AppOptions.set("disableHistory", params.get("disablehistory") === "true");
     }
+    if (params.has("enablepermissions")) {
+      AppOptions.set(
+        "enablePermissions",
+        params.get("enablepermissions") === "true"
+      );
+    }
     if (params.has("verbosity")) {
       AppOptions.set("verbosity", params.get("verbosity") | 0);
     }
diff --git a/web/app_options.js b/web/app_options.js
index e461f3910..82711b931 100644
--- a/web/app_options.js
+++ b/web/app_options.js
@@ -161,7 +161,7 @@ const defaultOptions = {
   },
   pdfBugEnabled: {
     /** @type {boolean} */
-    value: typeof PDFJSDev === "undefined",
+    value: typeof PDFJSDev === "undefined" || PDFJSDev.test("TESTING"),
     kind: OptionKind.VIEWER + OptionKind.PREFERENCE,
   },
   printResolution: {

Which you'd invoke, from the integration-test, something like:

beforeAll(async () => {
  pages = await loadAndWait(
    "issue9972-3.pdf",
    "#hiddenCopyElement",
    100,
    async page => {
      await page.waitForFunction(
        () => !!window.PDFViewerApplication.eventBus
      );
      await waitForTextLayer(page);
    },
    "enablePermissions=true",
  );
});

@jojje
Copy link

jojje commented Oct 10, 2023

As far as I know, unless things have changed recently, web-extensions aren't allowed to run in the built-in Firefox PDF Viewer.

Unfortunately so, which is why I'm forced to use a bookmarklet to make the pdf viewer extensible for my use cases.

The specific use case behind the bug report this change is trying to address is a tool for visually impaired people. They can select part of a document for TTS synthesis, and custom code strips text content that makes absolutely no sense reading aloud (such as all the gazillion document references to xyz et al, ), before passing the data on to the clipboard and TTS applications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants