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

Disable the "copy all text" feature when enablePermissions is set (PR 16286 follow-up) #16320

Merged

Conversation

Snuffleupagus
Copy link
Collaborator

When permissions are enabled and the PDF document doesn't have the COPY-flag set, it shouldn't be possible for the user to trigger the "copy all text" feature.

@mozilla mozilla deleted a comment from pdfjsbot Apr 18, 2023
@mozilla mozilla deleted a comment from pdfjsbot Apr 18, 2023
@Snuffleupagus Snuffleupagus marked this pull request as draft April 18, 2023 18:58
@mozilla mozilla deleted a comment from pdfjsbot Apr 18, 2023
…PR 16286 follow-up)

When permissions are enabled and the PDF document doesn't have the COPY-flag set, it shouldn't be possible for the user to trigger the "copy all text" feature.
@Snuffleupagus Snuffleupagus force-pushed the enablePermissions-disable-copy-all branch from e1adc90 to 6bfcc96 Compare April 18, 2023 19:12
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.

@calixteman
Copy link
Contributor

Oh I missed the draft status.

@Snuffleupagus Snuffleupagus marked this pull request as ready for review April 18, 2023 19:15
@Snuffleupagus
Copy link
Collaborator Author

Oh I missed the draft status.

The GENERIC viewer can open more than one document, which I obviously forgot about initially; please check the updated patch.

@mozilla mozilla deleted a comment from pdfjsbot Apr 18, 2023
@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/2252a9f348c769e/output.txt

@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/5ff794d786c6f03/output.txt

@calixteman
Copy link
Contributor

Do you plane to disable copy here too:
https://github.com/mozilla/pdf.js/blob/master/web/text_layer_builder.js#L217 ?

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/5ff794d786c6f03/output.txt

Total script time: 4.16 mins

  • Integration Tests: Passed

@Snuffleupagus
Copy link
Collaborator Author

Do you plane to disable copy here too:
https://github.com/mozilla/pdf.js/blob/master/web/text_layer_builder.js#L217 ?

I don't believe that we need to, since we have CSS rules that prevent user-selection.

pdf.js/web/pdf_viewer.css

Lines 196 to 199 in 3e292dc

.pdfViewer.enablePermissions .textLayer span {
user-select: none !important;
cursor: not-allowed;
}

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/2252a9f348c769e/output.txt

Total script time: 22.53 mins

  • Integration Tests: FAILED

@Snuffleupagus Snuffleupagus merged commit 42faecf into mozilla:master Apr 18, 2023
3 checks passed
@Snuffleupagus Snuffleupagus deleted the enablePermissions-disable-copy-all branch April 18, 2023 20:13
@calixteman
Copy link
Contributor

Do you plane to disable copy here too:
https://github.com/mozilla/pdf.js/blob/master/web/text_layer_builder.js#L217 ?

I don't believe that we need to, since we have CSS rules that prevent user-selection.

pdf.js/web/pdf_viewer.css

Lines 196 to 199 in 3e292dc

.pdfViewer.enablePermissions .textLayer span {
user-select: none !important;
cursor: not-allowed;
}

After having thinking about that, I think it could be useful to be able to select the text (for example when the user wants to show something to someone else) even if the copy is disabled (I personally already have been in such a situation and it was a bit painful). So I'd inclined to remove the CSS rule and disable the copy in the copy listener. Wdyt ?
When the pdf doesn't have the full permissions, maybe we should display an icon in the toolbar (like a lock or something like that) with a tooltip indication what the permissions are. And when the user try to copy when he isn't allowed to, then we could highlight this icon... or something like that.

@Snuffleupagus
Copy link
Collaborator Author

After having thinking about that, I think it could be useful to be able to select the text (for example when the user wants to show something to someone else) even if the copy is disabled (I personally already have been in such a situation and it was a bit painful). So I'd inclined to remove the CSS rule and disable the copy in the copy listener. Wdyt ?

Sure, we could implement it like that without too much trouble I think; I'll submit a patch for this later.

When the pdf doesn't have the full permissions, maybe we should display an icon in the toolbar (like a lock or something like that) with a tooltip indication what the permissions are. And when the user try to copy when he isn't allowed to, then we could highlight this icon... or something like that.

In Adobe Reader there's an additional sidebar-view in protected documents, but given that we ignore permissions by default that feels like a slightly too "heavy" solution in the PDF.js viewer. Your idea about an additional toolbarButton with a tooltip sounds simpler, although we'd probably need a couple of new l10n-strings for that.
Also, this part definitely feels like something where having UX input before we start coding would help.

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.

None yet

3 participants