1843054 - Enable adding image PDF editing feature in Release

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, enhancement)

Description

[Marco Castelluccio [:marco]]

Once we are ready, we should set pdfjs.enableStampEditor to true for Release.

Comment 1

[Marco Castelluccio [:marco]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New PDF editing feature often requested by users.
[Affects Firefox for Android]: No.
[Suggested wording]: Firefox now supports editing PDFs by adding images, in addition to adding text and drawing.
[Links (documentation, blog post, etc)]: N/A.

Comment 2

[kbryant]

I've verbally shared this (with Usama) in a couple meetings, but want to make sure these a11y requirements are officially logged. (Please let me know if this needs to be documented elsewhere, too!)

1. When an image is added to a PDF by users, they need to be able to also add alt-text for that image.
2. When a user edits an image they've added to a PDF, they need to be able to edit the alt-text for that image as well.

Comment 3

[Marco Castelluccio [:marco]]

(In reply to kbryant from comment #2)

I've verbally shared this (with Usama) in a couple meetings, but want to make sure these a11y requirements are officially logged. (Please let me know if this needs to be documented elsewhere, too!)

1. When an image is added to a PDF by users, they need to be able to also add alt-text for that image.
2. When a user edits an image they've added to a PDF, they need to be able to edit the alt-text for that image as well.

Do you think this should be a blocker for releasing the feature?

We can't take inspiration from the state of the art for this because no other PDF editor supports it AFAIK, so we will need UX and accessibility team support to build the alt-text additional functionality.

Comment 4

[Calixte Denizet (:calixte)]

:morgan, could you test this new feature (i.e. adding an image in pdf) in nightly and give us your feedback from an a11y point of view ?

Comment 5

[Marco Castelluccio [:marco]]

I'm talking with Kim about the alt text feature on Tue.

Comment 6

[Morgan Reschenberg [:morgan]]

(In reply to Calixte Denizet (:calixte) from comment #4)

:morgan, could you test this new feature (i.e. adding an image in pdf) in nightly and give us your feedback from an a11y point of view ?

are there instructions for how to do this somewhere? I'm not familiar

Comment 7

[Marco Castelluccio [:marco]]

This is an addition on top of drawing and adding text, the earlier accessibility review happened in bug 1777257.

You can add an image by clicking on the relevant icon in the PDF viewer toolbar (the icon between download and add text, on the top right corner).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Added to the Nightly relnotes. Once the patch lands for this to ride the trains, we can update the note accordingly.

Comment 9

[Morgan Reschenberg [:morgan]]

(In reply to Marco Castelluccio [:marco] from comment #7)

This is an addition on top of drawing and adding text, the earlier accessibility review happened in bug 1777257.

You can add an image by clicking on the relevant icon in the PDF viewer toolbar (the icon between download and add text, on the top right corner).

When I click on the icon, nothing happens. I assume a file-selection dialog is supposed to open?

EDIT: After spending some time on this with :ayeddi, we discovered you have to click the PDF canvas after clicking the image insertion button in order to place an image. This UX makes the feature difficult to discover -- is it possible to open the dialog when the button is clicked and then have the chosen image inserted in the document (in the middle of the active page) where it can then be resized and moved?

The image insertion icon button should remain active while the image insertion mode is active -- otherwise, there's no indication to the user that they're not in "regular" point-and-click mode. It'd be really helpful to have a tooltip or something that describes this feature's usage, especially if we're intent on keeping the ordering of "user clicks on the page and then selects an image". Escape should exit the image insertion mode and remove the active state from the icon button.

Does this feature exist in other browsers/PDF editors? Does our UX match?

I belive Anna has some additional comments about keyboard accessibility and alt text, which I'll defer to her on here.

Comment 10

[Morgan Reschenberg [:morgan]]

Attachment: active_image_button_macos

ah my mistake, it looks like this button does get the active state when image mode is active, the contrast when compared to the neighbouring non-active buttons is extremely low, however (especially on macOS with increase contrast enabled). This needs to be updated.

Comment 11

[Morgan Reschenberg [:morgan]]

Attachment: insert_image_win11_nightsky.gif

On windows in HCM, the active state is more discernible, but there isn't an obvious change when you hover the active button. Also, the icon disappears when transitioning between active an inactive.

Comment 12

[Anna Yeddi [:ayeddi]]

The feature is not accessible with a keyboard. At least I could not find the way to activate the file selection dialog, even when the keyboard focus was on one of the focusable elements (a link) within the PDF - pressing Space/Enter would activate that focusable element. Maybe, while the user is in the image injection mode, we could override the Enter key as it is overwritten for click and tap?

Note: the image injection mode remains active even after the image is injected, which I personally was not expecting, especially considering that the discoverability of the functionality (a need to click on the PDF to proceed with the action) was not clear.

If there is a keyboard shortcut for image injection when the button/mode is On, it should be documented within a reach (i.e. on the tooltip that :morgan has offered earlier).

There is no alt text creation or editing functionality provided. Moreover, an image is injected as <canvas> without any text alternative. MacOS VoiceOver guessed that it was an image, but it is not implied from the code. More information about a11y concerns for Canvas elements can be found on MDN and the resources linked there.

Comment 13

[Marco Castelluccio [:marco]]

We have a few bugs open for some of the concerns you raised:

• Bug 1844952 to support adding alt text. We need to figure out a good UI/UX for this, Adobe Acrobat supports it but it is not really discoverable.
• Bug 1845088 to make "add image" accessible with a keyboard. The other "add text" and "draw" are also not accessible with a keyboard. We'd appreciate your help here: even if we make the buttons activable with a keyboard, how can users position the image/text in the right place?
• Bug 1843293 about making the button directly open the file dialog.

In addition to those, we have bug 1845087 about accessibility. We have marked all of these with the "access" keyword, we hope you can help assign a severity.

Could you file other bugs you found and mark them as blocking bug 1774350?

Comment 14

[kbryant]

I've cc'd Usama on this ticket because there are multiple a11y blockers (even without the alt-text requirement) that he needs to be aware of and (as necessary) help resolve.

Comment 15

[Marco Castelluccio [:marco]]

We (me, Calixte, Usama, and Dasha) met yesterday about the feature in general, the current plan is to target 118 or 119.

Comment 17

[Calixte Denizet (:calixte)]

Attachment: Bug 1843054 - Enable pdfjs.enableStampEditor on Release r=#pdfjs-reviewers

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8679018abbfe
Enable pdfjs.enableStampEditor on Release r=pdfjs-reviewers,marco

Comment 19

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/8679018abbfe

Comment 20

[Calixte Denizet (:calixte)]

Comment on attachment 9355787 [details]
Bug 1843054 - Enable pdfjs.enableStampEditor on Release r=#pdfjs-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Users won't enjoy the new "add image in a pdf" feature.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Self contained change and it has been tested by QA.
• String changes made/needed: No
• Is Android affected?: No

Comment 21

[Dianna Smith [:diannaS]]

Comment on attachment 9355787 [details]
Bug 1843054 - Enable pdfjs.enableStampEditor on Release r=#pdfjs-reviewers

Approved for 119.0b4

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/79e366908649

Comment 23

[Dana Lucaci, Desktop QA]

Verified all the bugs mentioned in here (Bug 1845087, Bug 1845088, Bug 1850797, Bug 1851489, Bug 1790255, Bug 1843293, Bug 1844952) with Firefox 120.0a1 (20231004094640) and Firefox 119.0b4 (20231002091755) on macOS 12 ARM, Win 10 and Ubuntu 22.04.