Annotation deletion popup (bug 1899731) #18900

ryzokuken · 2024-10-14T14:37:59Z

This PR adds a simple popup/snackbar setup and wires it up for annotation removal, so when a user deletes any number of annotations, they get a message about it alongside an undo button that allows them to quickly undo the deletion.

nicolo-ribaudo

This will need tests in the various editors, in https://github.com/mozilla/pdf.js/tree/master/test/integration, or maybe its own new test file that tests that the toast is properly shown in the various cases.

src/display/editor/tools.js

web/toast_manager.js

web/app.js

Snuffleupagus

Is this perhaps related to https://bugzilla.mozilla.org/show_bug.cgi?id=1899731, since there seems to be no mention of that bug here?

Please note that we'll need a "full" design specification before implementing this, and given its current state the code will also require a fair amount of work here (as far as I can tell).

web/message_bar.css

marco-c · 2024-10-14T16:28:56Z

Is this perhaps related to https://bugzilla.mozilla.org/show_bug.cgi?id=1899731, since there seems to be no mention of that bug here?

Please note that we'll need a "full" design specification before implementing this, and given its current state the code will also require a fair amount of work here (as far as I can tell).

I added a link to the Figma specs in the bug. It should be accessible to all of you (let me know if you don't have access, and I'll get you added).

web/viewer.html

src/display/editor/tools.js

web/message_bar.css

web/pdf_viewer.css

calixteman · 2024-10-14T16:45:42Z

web/message_bar.css

+	bottom: 100px;
+	left: 50%;
+	transform: translateX(-50%);
+}


Did you test that everything is rendering fine when switching to HCM mode on Windows ?

nicolo-ribaudo · 2024-10-14T16:50:07Z

@calixteman @Snuffleupagus Please wait to review until when this PR is marked as ready -- I am giving some feedback to @ryzokuken to make sure that it matches correctly what is defined in the figma doc :)

Snuffleupagus

Leaving just a few high-level comments regarding things/patterns that should be improved.

l10n/en-US/viewer.ftl

Snuffleupagus · 2024-10-14T16:50:23Z

src/display/editor/tools.js

+      this.#toastManager.show(
+        undo,
+        editors.length > 1
+          ? `${editors.length} annotations`


This won't be possible to localize properly, since it hard-codes the word "annotations".

Snuffleupagus · 2024-10-14T16:51:43Z

web/message_bar.css

+  --message-bar-icon-color: #0060DF;
+	--message-bar-bg-color: #deeafc;
+	--message-bar-fg-color: #15141a;
+	--text-primary-color: #15141a;


Should this not be using different colours depending on light/dark/HCM mode?

web/pdf_viewer.js

web/toast_manager.js

web/viewer.js

web/app.js

This is a temporary commit on this branch that aims to address some of the review comments from the PR at mozilla#18900.

web/toast_manager.js

ryzokuken · 2024-10-14T23:10:11Z

First off, thanks everyone for all the comments it's nice to have a stronger sense of direction as to where I need to take this patch. I addressed some of the comments y'all made and marked the appropriate suggestions as "resolved" although please feel free to re-raise anything.

To respond to the questions:

Is this perhaps related to bugzilla.mozilla.org/show_bug.cgi?id=1899731, since there seems to be no mention of that bug here?

Please note that we'll need a "full" design specification before implementing this, and given its current state the code will also require a fair amount of work here (as far as I can tell).

Yes it is! Thanks @marco-c for the clarification and sharing the spec. Regarding the code, I hope the bigger pain points have either already been addressed or I'd be hacking on them shortly.

Snuffleupagus

Based on looking at the code, I believe that there's various cases where the notification won't be closed immediately as I believe is intended when:

Restoring the editor with Ctrl+Z (i.e. using the keyboard).
Toggling editing-mode off.
The entire editorLayer is being destroyed (i.e. when closing the current PDF document).

web/toast_manager.js

web/viewer.js

web/app.js

web/toast_manager.js

ryzokuken · 2024-10-15T13:49:33Z

@Snuffleupagus

(I don't know what setInterval has to do with this, since that's not related to the issue I mentioned above.)

Apologies, I misunderstood your comment at first. Applied the fix that you'd ultimately suggested.

I've marked a number of your comments as resolved, please take a look and let me know if I missed something from my latest commit while I work on addressing the remaining issues. Thanks again for your patience.

web/viewer.html

web/message_bar.css

web/viewer.html

web/viewer.js

web/editor_undo_bar.js

l10n/en-US/viewer.ftl

ryzokuken · 2024-10-16T14:43:56Z

@Snuffleupagus thanks for the last slew of suggestions especially, it taught me a lot about the code and fixed the l10n bug. The code should ideally look much more manageable now. While I make more improvements and add tests, please let me know if you have any further comments.

Snuffleupagus

Another thing that I think should be supported is closing the undo-bar with the Esc key, and the easiest solution would be to (after implementing the comments below) also add the following code just after this existing code:

if (this.editorUndoBar?.isOpen) {
  this.editorUndoBar.hide();
  handled = true;
}

web/message_bar.css

web/editor_undo_bar.js

web/app.js

ryzokuken · 2024-10-16T19:59:45Z

I added the ESC key dismiss thingie but note that it's not part of the Figma spec which says:

The message bar should persist on screen until:

User clicks “X” close

User creates a new annotation

User opens Print dialog

User opens Save dialog

User selects an option from the “more” menu

User closes tab

User closes session

Maybe we should get the ESC key condition added there.

Snuffleupagus · 2024-10-16T20:17:46Z

I added the ESC key dismiss thingie but note that it's not part of the Figma spec which says:

The message bar should persist on screen until:

[...]

Note also the other cases mentioned previously in #18900 (review), maybe those should be added to that list as well?

ryzokuken · 2024-10-16T20:20:02Z

Absolutely, I'll mention this to the UX folks.

web/editor_undo_bar.js

ryzokuken · 2024-10-16T20:53:59Z

Good point, I cleaned up the class fields and simplified the constructor a bit.

nicolo-ribaudo · 2024-10-18T10:34:14Z

The first commit also (accidentally) fixes a bug in the existing .messageBar usage, where the X was not vertically aligned in the button:

Snuffleupagus · 2024-10-18T10:38:24Z

web/app.js

+    let editorUndoBar = null;
+    if (appConfig.editorUndoBar) {
+      editorUndoBar = new EditorUndoBar(
+        appConfig.editorUndoBar,
+        eventBus,
+        l10n
+      );
+    }
+    this.editorUndoBar = editorUndoBar;


This can now be simplified, see also the next comment.

Suggested change

let editorUndoBar = null;

if (appConfig.editorUndoBar) {

editorUndoBar = new EditorUndoBar(

appConfig.editorUndoBar,

eventBus,

l10n

);

}

this.editorUndoBar = editorUndoBar;

if (appConfig.editorUndoBar) {

this.editorUndoBar = new EditorUndoBar(

appConfig.editorUndoBar,

eventBus

);

}

Snuffleupagus · 2024-10-18T10:38:56Z

web/app.js

@@ -460,6 +472,7 @@ const PDFViewerApplication = {
      linkService: pdfLinkService,
      downloadManager,
      altTextManager,
+      editorUndoBar,


Suggested change

editorUndoBar,

editorUndoBar: this.editorUndoBar,

Snuffleupagus · 2024-10-18T10:43:08Z

web/editor_undo_bar.js

+    freetext: "pdfjs-editor-undo-bar-message-freetext",
+    stamp: "pdfjs-editor-undo-bar-message-stamp",
+    ink: "pdfjs-editor-undo-bar-message-ink",
+    __multiple: "pdfjs-editor-undo-bar-message-multiple",


Won't a single underscore suffice here?

Snuffleupagus · 2024-10-18T10:46:45Z

web/editor_undo_bar.js

+  async show(undoAction, messageData) {
+    this.hide();
+    this.isOpen = true;
+    this.#controller = new AbortController();
+
+    this.#message.textContent =
+      typeof messageData === "string"
+        ? await this.#l10n.get(EditorUndoBar.#l10nMessages[messageData])
+        : await this.#l10n.get(EditorUndoBar.#l10nMessages.__multiple, {
+            count: messageData,
+          });
+    this.#container.hidden = false;


Why do we need to translate this "manually", since that should only be done as a last resort?
Also, the introduction of await here could lead to intermittent bugs with the bar reshowing after having been hidden elsewhere.

Suggested change

async show(undoAction, messageData) {

this.hide();

this.isOpen = true;

this.#controller = new AbortController();

this.#message.textContent =

typeof messageData === "string"

? await this.#l10n.get(EditorUndoBar.#l10nMessages[messageData])

: await this.#l10n.get(EditorUndoBar.#l10nMessages.__multiple, {

count: messageData,

});

this.#container.hidden = false;

show(undoAction, messageData) {

this.hide();

if (typeof messageData === "string") {

this.#message.setAttribute("data-l10n-id", EditorUndoBar.#l10nMessages[messageData]);

} else {

this.#message.setAttribute("data-l10n-id", EditorUndoBar.#l10nMessages._multiple)

this.#message.setAttribute("data-l10n-args", JSON.stringify({ count: messageData });

}

this.isOpen = true;

his.#container.hidden = false;

this.#controller = new AbortController();

Oh right, thank you!

Snuffleupagus · 2024-10-18T15:13:00Z

src/display/editor/annotation_editor_layer.js

@@ -404,6 +404,9 @@ class AnnotationEditorLayer {
        // Do nothing on right click.
        return;
      }
+
+      this.#uiManager._eventBus.dispatch("annotationstart", { source: this });


This comment applies elsewhere as well.

Is there possibly a more "central" spot where this can be done, since having this code in a bunch of places seems potentially error prone (since it's easy to miss adding a case)?

The event name isn't very specific, as far as I'm concerned. (But please see below.)

Ideally the undo-bar hiding would, in my opinion, be placed within the src/display/editor/-code as much as possible. (With events only used for features that are clearly outside of that folder.)
With that in mind, could we change this to the following instead?

Suggested change

this.#uiManager._eventBus.dispatch("annotationstart", { source: this });

this.#uiManager._editorUndoBar?.hide()

Is there possibly a more "central" spot where this can be done, since having this code in a bunch of places seems potentially error prone (since it's easy to miss adding a case)?

We originally tried looking for a central place but couldn't find a good one. The problem is some of the editors are created when they are "enabled" by clicking on the toolbar (example: the ink one), while others are created only when the user actually starts writing in them (example: the freetext one).

Snuffleupagus · 2024-10-18T15:13:25Z

src/display/editor/tools.js

@@ -804,6 +807,7 @@ class AnnotationEditorUIManager {
      rotation: 0,
    };
    this.isShiftKeyDown = false;
+    this.#editorUndoBar = editorUndoBar;


To support the suggestion above.

Suggested change

this.#editorUndoBar = editorUndoBar;

this._editorUndoBar = editorUndoBar;

Snuffleupagus · 2024-10-18T15:17:57Z

web/editor_undo_bar.js

+    eventBus._on("beforeprint", boundHide);
+    eventBus._on("download", boundHide);
+    eventBus._on("annotationstart", boundHide);
+    eventBus._on("secondarytoolbaraction", boundHide);


While this suggestion would be slight abuse of notation, do we really need a new event just for this or could we use the already existing one instead?

Suggested change

eventBus._on("secondarytoolbaraction", boundHide);

eventBus._on("reporttelemetry", evt => {

if (evt?.details?.type === "buttons") {

boundHide();

}

});

We originally considered this, but yeah it felt a bit too much abusing an event for something that's not meant to. Ideally an event called "reporttelemetry" should not have UI effects other than... reporting telemetry 😅.

Happy to change it if you feel strongly though.

Why not just add a click listener on the window (capture) and check if the target is under the toast bar ?
It's something we do in order to hide the secondary toolbar:
https://github.com/mozilla/pdf.js/blob/master/web/app.js#L2750-L2765

@calixteman about this one, does your comment imply that the toast should be over other UI elements like the toolbar? Or did you mean that we should dismiss the toast if the user clicks another element like in the case of the toolbar?

Snuffleupagus

There's a few additional cases where the undo-bar should also be hidden (which should be possible to implement within the src/display/editor/-code):

When toggling editing off (i.e. setting AnnotationEditorType.NONE).
When destroying the editor.

test/integration/freetext_editor_spec.mjs

test/integration/highlight_editor_spec.mjs

test/integration/ink_editor_spec.mjs

test/integration/stamp_editor_spec.mjs

test/integration/freetext_editor_spec.mjs

test/integration/highlight_editor_spec.mjs

Move .messageBar out of .dialog into its own standalone class in order to reuse as much of it for the upcoming feature for an undo message for annotations.

When a user deletes any number of annotations, they are notified of the action by a popup message with an undo button. Besides that, this change reuses the existing messageBar CSS class from the new alt-text dialog as much as possible.

Fix colors in dark and high contrast modes

Fix l10n

ryzokuken force-pushed the popup-undo-annotation branch from ebf8d04 to ae79bee Compare October 14, 2024 14:45

nicolo-ribaudo reviewed Oct 14, 2024

View reviewed changes

src/display/editor/tools.js Outdated Show resolved Hide resolved

web/toast_manager.js Outdated Show resolved Hide resolved

web/toast_manager.js Outdated Show resolved Hide resolved

web/toast_manager.js Outdated Show resolved Hide resolved

nicolo-ribaudo reviewed Oct 14, 2024

View reviewed changes

web/app.js Outdated Show resolved Hide resolved

Snuffleupagus requested changes Oct 14, 2024

View reviewed changes

nicolo-ribaudo reviewed Oct 14, 2024

View reviewed changes

web/message_bar.css Outdated Show resolved Hide resolved

marco-c changed the title ~~Annotation deletion popup~~ Bug 1899731 - Annotation deletion popup Oct 14, 2024

marco-c changed the title ~~Bug 1899731 - Annotation deletion popup~~ Annotation deletion popup (bug 1899731) Oct 14, 2024

calixteman reviewed Oct 14, 2024

View reviewed changes

web/viewer.html Outdated Show resolved Hide resolved

calixteman reviewed Oct 14, 2024

View reviewed changes

src/display/editor/tools.js Outdated Show resolved Hide resolved

Snuffleupagus requested changes Oct 14, 2024

View reviewed changes

web/message_bar.css Show resolved Hide resolved

web/message_bar.css Outdated Show resolved Hide resolved

web/pdf_viewer.css Outdated Show resolved Hide resolved

calixteman reviewed Oct 14, 2024

View reviewed changes

Snuffleupagus requested changes Oct 14, 2024

View reviewed changes

ryzokuken added a commit to ryzokuken/pdf.js that referenced this pull request Oct 14, 2024

Address few review comments

67822c9

This is a temporary commit on this branch that aims to address some of the review comments from the PR at mozilla#18900.

nicolo-ribaudo reviewed Oct 14, 2024

View reviewed changes

web/toast_manager.js Outdated Show resolved Hide resolved

Snuffleupagus requested changes Oct 15, 2024

View reviewed changes

Snuffleupagus reviewed Oct 15, 2024

View reviewed changes

web/viewer.html Outdated Show resolved Hide resolved

web/message_bar.css Outdated Show resolved Hide resolved

web/message_bar.css Outdated Show resolved Hide resolved

timvandermeij added the editor label Oct 15, 2024

Snuffleupagus reviewed Oct 16, 2024

View reviewed changes

web/message_bar.css Outdated Show resolved Hide resolved

web/editor_undo_bar.js Outdated Show resolved Hide resolved

web/editor_undo_bar.js Show resolved Hide resolved

web/editor_undo_bar.js Outdated Show resolved Hide resolved

web/app.js Outdated Show resolved Hide resolved

Snuffleupagus reviewed Oct 16, 2024

View reviewed changes

web/editor_undo_bar.js Outdated Show resolved Hide resolved

web/editor_undo_bar.js Outdated Show resolved Hide resolved

web/editor_undo_bar.js Outdated Show resolved Hide resolved

ryzokuken force-pushed the popup-undo-annotation branch from a4130d0 to 13d7c53 Compare October 18, 2024 02:16

nicolo-ribaudo force-pushed the popup-undo-annotation branch from 8e291bb to 51d6d9e Compare October 18, 2024 10:33

Snuffleupagus requested changes Oct 18, 2024

View reviewed changes

nicolo-ribaudo force-pushed the popup-undo-annotation branch 2 times, most recently from a0c05d3 to a7b37b7 Compare October 18, 2024 10:57

Snuffleupagus reviewed Oct 18, 2024

View reviewed changes

Snuffleupagus requested changes Oct 19, 2024

View reviewed changes

Snuffleupagus reviewed Oct 19, 2024

View reviewed changes

test/integration/highlight_editor_spec.mjs Outdated Show resolved Hide resolved

Aditi-1400 force-pushed the popup-undo-annotation branch from 48e196f to a1f4aac Compare October 19, 2024 16:44

ryzokuken and others added 7 commits October 21, 2024 13:28

Break messageBar class out

b107fff

Move .messageBar out of .dialog into its own standalone class in order to reuse as much of it for the upcoming feature for an undo message for annotations.

Pop open a message when user deletes an annotation

291b593

When a user deletes any number of annotations, they are notified of the action by a popup message with an undo button. Besides that, this change reuses the existing messageBar CSS class from the new alt-text dialog as much as possible.

fixup! Pop open a message when user deletes an annotation

d26ab1f

Fix colors in dark and high contrast modes

fixup! Pop open a message when user deletes an annotation

95c8b8c

Fix l10n

fixup! Pop open a message when user deletes an annotation

26a5fd8

Add tests for annotation delete popup

5ec02a6

fixup! Pop open a message when user deletes an annotation

a3d26c3

ryzokuken force-pushed the popup-undo-annotation branch from a1f4aac to a3d26c3 Compare October 21, 2024 08:07

ryzokuken added 2 commits October 21, 2024 22:38

fixup! Add tests for annotation delete popup

a893637

fixup! Pop open a message when user deletes an annotation

c1cbf9b

ryzokuken force-pushed the popup-undo-annotation branch from 23c6dfa to c1cbf9b Compare October 21, 2024 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation deletion popup (bug 1899731) #18900

Annotation deletion popup (bug 1899731) #18900

ryzokuken commented Oct 14, 2024

nicolo-ribaudo left a comment

Snuffleupagus left a comment

marco-c commented Oct 14, 2024

calixteman Oct 14, 2024

nicolo-ribaudo commented Oct 14, 2024 •

edited

Loading

Snuffleupagus left a comment

Snuffleupagus Oct 14, 2024 •

edited

Loading

Snuffleupagus Oct 14, 2024

ryzokuken commented Oct 14, 2024

Snuffleupagus left a comment

ryzokuken commented Oct 15, 2024

ryzokuken commented Oct 16, 2024

Snuffleupagus left a comment •

edited

Loading

ryzokuken commented Oct 16, 2024

Snuffleupagus commented Oct 16, 2024

ryzokuken commented Oct 16, 2024

ryzokuken commented Oct 16, 2024

nicolo-ribaudo commented Oct 18, 2024

Snuffleupagus Oct 18, 2024

Snuffleupagus Oct 18, 2024

Snuffleupagus Oct 18, 2024

Snuffleupagus Oct 18, 2024

nicolo-ribaudo Oct 18, 2024

Snuffleupagus Oct 18, 2024

nicolo-ribaudo Oct 18, 2024

Snuffleupagus Oct 18, 2024

Snuffleupagus Oct 18, 2024

nicolo-ribaudo Oct 18, 2024

calixteman Oct 18, 2024

ryzokuken Oct 21, 2024

Snuffleupagus left a comment

	this.#uiManager._eventBus.dispatch("annotationstart", { source: this });
	this.#uiManager._editorUndoBar?.hide()

	this.#editorUndoBar = editorUndoBar;
	this._editorUndoBar = editorUndoBar;

-    eventBus._on("secondarytoolbaraction", boundHide);
+    eventBus._on("reporttelemetry", evt => {
+      if (evt?.details?.type === "buttons") {
+        boundHide();
+      }
+    });

Annotation deletion popup (bug 1899731) #18900

Are you sure you want to change the base?

Annotation deletion popup (bug 1899731) #18900

Conversation

ryzokuken commented Oct 14, 2024

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

marco-c commented Oct 14, 2024

Choose a reason for hiding this comment

nicolo-ribaudo commented Oct 14, 2024 • edited Loading

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Oct 14, 2024

Snuffleupagus left a comment

Choose a reason for hiding this comment

ryzokuken commented Oct 15, 2024

ryzokuken commented Oct 16, 2024

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

ryzokuken commented Oct 16, 2024

Snuffleupagus commented Oct 16, 2024

ryzokuken commented Oct 16, 2024

ryzokuken commented Oct 16, 2024

nicolo-ribaudo commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented Oct 14, 2024 •

edited

Loading

Snuffleupagus Oct 14, 2024 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading