[editor] Add a FreeText editor (#14970) #14976

calixteman · 2022-06-01T08:47:54Z

add a basic UI to edit some text in a pdf;
an editor can be moved, suppressed, cut, copied, pasted, selected;
add an undo/redo manager.

Snuffleupagus

This sure is a lot of code all at once :-)

The following comments are based on a first brief look, and so far I've not really looked at the main part of the patch i.e. src/display/annotation_editor_layer.js (given its size).

Looking at the viewer-integration in particular, there's room for improvement to make this better and easier to work with (especially when adding support for additional annotation-types later).

Snuffleupagus · 2022-06-01T13:59:35Z

src/display/annotation_editor_layer.js

+  #id;
+
+  constructor() {
+    this.#id = 0;


Can't you just directly define #id = 0 above and remove the constructor?

The same (or similar) comment also applies in other spots further down in this file.

Snuffleupagus · 2022-06-01T14:04:43Z

src/display/annotation_storage.js

+
+    const clone = new Map();
+    for (const [key, value] of this._storage) {
+      if ("serialize" in value) {


This feels brittle, i.e. there's nothing stopping a user from manually adding something to the storage that'd fail here; so can we please not do this?

Instead, let's use something like value instanceof AnnotationEditor here (which I think is what the value should be).

Snuffleupagus · 2022-06-01T14:06:53Z

src/display/annotation_editor_layer.js

+    const isMac =
+      typeof navigator !== "undefined" &&
+      (navigator.platform.startsWith("Mac") || navigator.platform === "iPhone");


To make this more maintainable/clearer, please add a method similar to the following one in this class as well:

pdf.js/src/display/annotation_layer.js

Lines 554 to 561 in 1ac33c9

static get platform() {

const platform = typeof navigator !== "undefined" ? navigator.platform : "";

return shadow(this, "platform", {

isWin: platform.includes("Win"),

isMac: platform.includes("Mac"),

});

}

Snuffleupagus · 2022-06-01T14:08:04Z

src/display/display_utils.js

@@ -241,6 +241,7 @@ class PageViewport {
      offsetCanvasX - rotateA * scale * centerX - rotateC * scale * centerY,
      offsetCanvasY - rotateB * scale * centerX - rotateD * scale * centerY,
    ];
+    this.inverseTransform = Util.inverseTransform(this.transform);


Given that this isn't used by default (at least I don't think so), can we use a (shadowed) getter for this instead?

Snuffleupagus · 2022-06-01T14:10:48Z

web/annotation_editor_layer_builder.css

+}
+
+.annotationEditorLayer .freeTextEditor .internal:empty::before {
+  content: "Enter some text...";


This must be properly localizable!
Please refer to how the AnnotationLayer handles localization, and follow a similar pattern for the annotationEditorLayer as well.

Snuffleupagus · 2022-06-01T14:59:16Z

web/viewer.html

@@ -263,6 +263,12 @@
                <span id="numPages" class="toolbarLabel"></span>
              </div>
              <div id="toolbarViewerRight">
+                <div id="editionButtons" role="radiogroup">
+                  <button id="freeTextMode" class="toolbarButton" title="Add some free text" role="radio" aria-checked="false" tabindex="31" data-l10n-id="freetext_mode">
+                    <span data-l10n-id="freetext_mode_label">Free Text</span>


editor_free_text_label and "FreeText Annotation" here.

Snuffleupagus · 2022-06-01T15:01:14Z

web/app.js

+    if (AppOptions.get("annotationEditionMode")) {
+      document.getElementById("freeTextMode").style.visibility = "visible";
+    }
+


Suggested change

if (AppOptions.get("annotationEditionMode")) {

document.getElementById("freeTextMode").style.visibility = "visible";

}

if (AppOptions.get("annotationEditorEnabled")) {

document.getElementById("editorModeButtons").classList.remove("hidden");

}

Snuffleupagus · 2022-06-01T15:02:05Z

web/app.js

@@ -1878,6 +1889,7 @@ const PDFViewerApplication = {
    eventBus._on("namedaction", webViewerNamedAction);
    eventBus._on("presentationmodechanged", webViewerPresentationModeChanged);
    eventBus._on("presentationmode", webViewerPresentationMode);
+    eventBus._on("freetextmode", webViewerFreeTextMode);


Please note the comment in web/toolbar.js about the event name/format.

Snuffleupagus · 2022-06-01T15:05:11Z

web/app.js

@@ -1851,6 +1857,11 @@ const PDFViewerApplication = {
    this.pdfPresentationMode?.request();
  },

+  requestFreeTextMode() {
+    AnnotationEditorLayerBuilder.setEditorType("FreeText");


(While the viewer only really supports being used standalone, it seems a bit weird that the editor-mode is global. However, I guess that this considerably simplifies the overall implementation?)

Snuffleupagus · 2022-06-01T15:13:46Z

web/app.js

+  requestFreeTextMode() {
+    AnnotationEditorLayerBuilder.setEditorType("FreeText");
+    this.appConfig.toolbar.freeTextModeButton.classList.toggle("toggled");
+  },
+


Please handle this in the BaseViewer instead, again similar to how the scroll/spread-modes are implemented.

Basically, let's add a annotationEditorMode getter/setter-pair in the BaseViewer that takes care of updating the editor-mode and then dispatches a annotationeditormodechanged event that the Toolbar listens to and updates the buttons correctly.

Snuffleupagus · 2022-06-02T10:35:03Z

src/display/annotation_editor_layer.js

+ * Base class for editors.
+ */
+class AnnotationEditor {
+  #isInEditMode;


As mentioned previously, I believe that this can be directly initialized here (rather than in the constructor).

Snuffleupagus · 2022-06-02T10:36:16Z

src/display/annotation_editor_layer.js

+  /**
+   * @param {AnnotationEditorParameters} parameters
+   */
+  constructor(parameters) {


Given that this is an abstract base class, how about enforcing that it cannot be used directly with something similar to e.g.

pdf.js/src/core/base_stream.js

Lines 20 to 22 in 1ac33c9

if (this.constructor === BaseStream) {

unreachable("Cannot initialize BaseStream.");

}

Snuffleupagus · 2022-06-02T10:39:42Z

src/display/annotation_editor_layer.js

+   * @returns {AnnotationEditor}
+   */
+  copy() {
+    throw new Error("An editor must be copyable");


In other abstract base classes, we usually use unreachable in these cases (applies both here and below).

Snuffleupagus · 2022-06-02T10:42:15Z

src/display/annotation_editor_layer.js

+    );
+
+    return {
+      type: "FreeText",


As mentioned in the other PR, how about using AnnotationType instead here?

Snuffleupagus · 2022-06-02T10:45:45Z

src/display/annotation_editor_layer.js

+      default:
+        this._currentEditorClass = null;
+    }
+    this._currentEditorType = type;


Won't this allow setting the property to any value, i.e. even one that's completely bogus; does that not matter?

Snuffleupagus · 2022-06-02T10:56:39Z

src/display/annotation_editor_layer.js

+/**
+ * Class to handle undo/redo.
+ * Commands are just saved in a buffer.
+ * If we hit some memory issues we could likely use a circular buffer.


Do you have some idea of the number of commands where this might start to become an issue?

To be defensive here, perhaps we could simply prune the commands from the start of the Array once a large number has been added (e.g. more than 100 commands)?

Snuffleupagus · 2022-06-02T11:04:34Z

src/display/annotation_editor_layer.js

+  static _keyboardManager = new KeyboardManager([
+    [["ctrl+a", "mac+meta+a"], AnnotationEditorLayer.prototype.selectAll],
+    [
+      ["ctrl+shift+A", "mac+alt+meta+æ"],


Do these shortcuts actually make sense?
When using Firefox, on Windows, the "ctrl+shift+A" shortcut will open the about:addons page.

Acrobat has a shortcut to select/deselect all and I thought it was for newly added annotations but I was wrong.
So I'll remove them.

Snuffleupagus

Splitting this into multiple files definitely helps a lot here!

This is looking much better already, and I think that we can probably land this pretty soon, however I've added some more comments as I try to get through all of this (will try to look more during the weekend).

Snuffleupagus · 2022-06-03T13:58:07Z

l10n/en-US/viewer.properties

@@ -249,3 +249,8 @@ password_cancel=Cancel
 printing_not_supported=Warning: Printing is not fully supported by this browser.
 printing_not_ready=Warning: The PDF is not fully loaded for printing.
 web_fonts_disabled=Web fonts are disabled: unable to use embedded PDF fonts.
+
+# Edition


Please use "Editor" here.

Snuffleupagus · 2022-06-03T13:58:34Z

l10n/en-US/viewer.properties

+# Edition
+freetext_default_content=Enter some text...
+editor_free_text.title=Add FreeText Annotation
+editor_free_text_label=FreeText Annotation


Missing new-line here.

Snuffleupagus · 2022-06-03T13:59:59Z

src/display/edition/annotation_editor_layer.js

@@ -0,0 +1,432 @@
+/* Copyright 2022 Mozilla Foundation


For this, and the other new files, please change the directory name to editor/ instead.

Snuffleupagus · 2022-06-03T14:04:42Z

web/base_viewer.js

+    this.#annotationEditorEnabled = options.annotationEditorEnabled === true;
+    if (this.#annotationEditorEnabled) {


I wonder if we actually need this property now, or if we could just do the following here and then just check for this.#annotationEditorUIManager below in the setDocument-method?

if (options.annotationEditorEnabled === true) {

Snuffleupagus · 2022-06-03T14:11:05Z

web/base_viewer.js

+    if (this.#annotationEditorMode === mode) {
+      this.#annotationEditorMode = AnnotationEditorType.NONE;
+    } else {
+      this.#annotationEditorMode = mode;
+    }
+    this.#annotationEditorUIManager.updateMode(this.#annotationEditorMode);
+    this.eventBus.dispatch("annotationeditormodechanged", {
+      source: this,
+      mode: this.#annotationEditorMode,
+    });


Following the Scroll/Spread-mode setters, this should look something like the following:

Suggested change

if (this.#annotationEditorMode === mode) {

this.#annotationEditorMode = AnnotationEditorType.NONE;

} else {

this.#annotationEditorMode = mode;

}

this.#annotationEditorUIManager.updateMode(this.#annotationEditorMode);

this.eventBus.dispatch("annotationeditormodechanged", {

source: this,

mode: this.#annotationEditorMode,

});

if (!this.#annotationEditorUIManager) {

throw new Error(`The AnnotationEditor is not enabled.`);

}

if (this.#annotationEditorMode === mode) {

return; // The AnnotationEditor mode didn't change.

}

if (!isValidAnnotationEditorMode(mode)) {

throw new Error(`Invalid AnnotationEditor mode: ${mode}`);

}

this.#annotationEditorMode = mode;

this.eventBus.dispatch("annotationeditormodechanged", {

source: this,

mode,

});

this.#annotationEditorUIManager.updateMode(mode);

Snuffleupagus · 2022-06-03T14:16:39Z

web/toolbar.js

+    this.editorButtons = [
+      [AnnotationEditorType.FREE_TEXT, options.editorFreeTextButton],
+    ];
+


Please follow the same pattern as in these methods here instead:

pdf.js/web/secondary_toolbar.js

Lines 160 to 162 in 1953967

this.#bindCursorToolsListener(options);

this.#bindScrollModeListener(options);

this.#bindSpreadModeListener(options);

Basically, you want to add a #bindEditorModeListener-method and please make sure that it implements the toggled and aria-checked attributes the same way as e.g.

pdf.js/web/secondary_toolbar.js

Lines 219 to 230 in 1953967

#bindCursorToolsListener({ cursorSelectToolButton, cursorHandToolButton }) {

this.eventBus._on("cursortoolchanged", function ({ tool }) {

const isSelect = tool === CursorTool.SELECT,

isHand = tool === CursorTool.HAND;

cursorSelectToolButton.classList.toggle("toggled", isSelect);

cursorHandToolButton.classList.toggle("toggled", isHand);

cursorSelectToolButton.setAttribute("aria-checked", isSelect);

cursorHandToolButton.setAttribute("aria-checked", isHand);

});

}

Snuffleupagus · 2022-06-03T14:20:04Z

web/viewer.html

@@ -263,30 +263,36 @@
                <span id="numPages" class="toolbarLabel"></span>
              </div>
              <div id="toolbarViewerRight">
-                <button id="presentationMode" class="toolbarButton hiddenLargeView" title="Switch to Presentation Mode" tabindex="31" data-l10n-id="presentation_mode">
+                <div id="editionModeButtons" class="splitToolbarButton hidden" role="radiogroup">
+                  <button id="editorFreeText" class="toolbarButton" title="Add some free text" role="radio" aria-checked="false" tabindex="31" data-l10n-id="editor_free_text">


As mentioned previously, I still firmly believe that we need a button for the (default) "no editing is currently happening"-state to make it clear when editing is not being used (or how to toggle the feature off).

When a mode is enabled, here FreeText, the button is in a toggled state:

and when it's clicked, then it switches to untoggled state and then no more edition is possible.
It is the way it works in Edge (I don't pretend that Edge is a model to follow: I don't have any strong opinions on this kind of stuff and I hope some UX people could help on this kind of stuff).

Anyway, could we considering doing that in a follow-up ?

and when it's clicked, then it switches to untoggled state and then no more edition is possible.

As soon as you add support for at least one more kind of Annotation, this will start becoming really confusing as far as I'm concerned.
Hence why I think that we should fix this now, since it will simplify future changes and would also be consistent with all of our other buttons that control related state (see e.g. the Sidebar buttons, or Scroll/Spread-mode buttons).

One thing that's not particularly nice, in my opinion, about the current implementation is that it makes the BaseViewer-setter look inconsistent/strange when compared to many of the other ones (since they purposely ignore the "value is the same"-case).

Ok, as said I don't have any opinion on UX stuff: usually I rely on people who know.... and you're definitely a better expert than me.
Any idea for an icon ?

I'm not an expert on UX matters either, but I just want to try and make it as easily as possible to work with this w.r.t. any future changes (since I assume INK-annotations may be next in line here, given e.g. the code comments in BaseViewer).

Assuming that the icons are indeed only temporary, and given that all of this is disabled by default, how about maybe adding a copy of the selectTool.svg here?

The ink stuff is ready ;)
Ok for selectTool.

Snuffleupagus · 2022-06-03T14:26:50Z

src/display/edition/freetext.js

+  }
+
+  static setL10n(l10n) {
+    l10n?.get("freetext_default_content").then(msg => {


Please add this new string to the list in https://github.com/mozilla/pdf.js/blob/master/web/l10n_utils.js, to ensure that a fallback value is always available.

Furthermore, is there ever a risk that the localized string becomes available after the attribute has been set below?

Good question: I asked it to myself.
So l10n is queried at the first layer creation, in order to see it (or not) the user need to click on the text editor button then move their mouse somewhere on the page, click and then the div is created with an attribute having (or not) the localized value. My feeling is it's very unlikely possible to have a wrong string. I thought to try to implement something in js to emulate the empty::before behaviour, but I wasn't super excited by the idea... compared the easy solution we've right now.
Anyway, as usual if you have a good idea, I take it !

Would it work if the attribute is always updated asynchronously instead, or does that not work?

Because in that case we could just store the Promise here, rather than the raw string, with something like

this._defaultContentPromise = l10n.get("freetext_default_content");

and then set the attribute with

FreeTextEditor._defaultContentPromise.then(msg => { this.editorDiv.setAttribute("default-content", msg); });

I tested here:
https://jsfiddle.net/ub7tmxdo/13/
and it works.

Snuffleupagus · 2022-06-03T14:29:37Z

test/integration/freetext_editor_spec.js

+
+          const data = "Hello PDF.js World !!";
+          await page.mouse.click(rect.x + 10, rect.y + 10);
+          await page.type("#pdfjs_internal_editor_0 .internal", data);


Here, and elsewhere in the tests, I suggest using the new constant in these strings to avoid having to edit the test-cases if we ever need to update the prefix.

Snuffleupagus · 2022-06-03T14:35:20Z

src/display/edition/freetext.js

+
+  /** @inheritdoc */
+  isEmpty() {
+    return this.editorDiv.innerText.trim() === "";


Here, and elsewhere below, can we perhaps use textContent since I believe that's generally recommended over innerText these days?

Unfortunately, I think innerText is better than textContent mainly because of <br>:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText
I wanted to use a basic textarea but I didn't manage to make it growable so I fall back on a div, hence the <br>...

src/shared/util.js

Snuffleupagus · 2022-06-03T15:54:48Z

web/annotation_editor_layer_builder.js

+    this.pageDiv = options.pageDiv;
+    this.pdfPage = options.pdfPage;
+    this.annotationStorage = options.annotationStorage || null;
+    this.l10n = options.l10n;


Following the web/annotation_layer_builder.js implementation, I believe that you want ... || NullL10n here.

One bonus is that the code in src/display/editor/... can then just assume that the l10n is always available.

Snuffleupagus · 2022-06-03T17:53:43Z

src/shared/util.js

+};
+
+function isValidAnnotationEditorMode(mode) {
+  return mode >= 0 && mode < Object.keys(AnnotationEditorType).length;


It seems generally safer, with any future changes, to use Object.values(AnnotationEditorType).includes(mode) here similar to the pre-existing helper functions.

Also, if we're only using this in the BaseViewer the helper function can possible live in that file for now?

Snuffleupagus

r=me, with the final comments addressed and all tests passing.

Thank you, and sorry about the back-and-forth during review!

Snuffleupagus · 2022-06-04T10:58:52Z

src/shared/util.js

+
+const AnnotationEditorType = {
+  NONE: 0,
+  FREE_TEXT: 1,


To prevent future errors and typos, we should probably use the exact same names/values as in AnnotationType for these ones as well.
Hence, please let's use FREETEXT: 3, instead here.

Snuffleupagus · 2022-06-04T11:00:12Z

web/app.js

+  if (evt.toggle) {
+    PDFViewerApplication.pdfViewer.annotionEditorEnabled = true;
+  } else {
+    PDFViewerApplication.pdfViewer.annotationEditorMode = evt.mode;
+  }


Sorry, but why?
This really ought to be simply PDFViewerApplication.pdfViewer.annotationEditorMode = evt.mode; and nothing else here.

Oh, ok I think I understand what you want.
So to sum up and to be sure that we're ok:

clicking on select:

when off, switch to on and set tool buttons to off

when on, switch to off and set tool buttons to off

when clicking on a tool button:

when off, switch to one and set other tool buttons to off

when on do nothing

Finally, each tool button (FreeText, Ink, ....) is equivalent to a radio button.
Is it correct ?

The idea was actually that all buttons behave as if they were radio-buttons, including the editorNone-one (hence no special handling for that one).

Basically, it's only possible to have one of the None/FreeText/Ink/... buttons toggled at any one time. Does that help?

Snuffleupagus · 2022-06-04T11:01:16Z

web/base_viewer.js

+  get annotionEditorEnabled() {
+    return this.#annotionEditorEnabled;
+  }
+
+  set annotionEditorEnabled(toggle) {
+    this.#annotionEditorEnabled = !this.#annotionEditorEnabled;
+    this.eventBus.dispatch("annotationeditormodechanged", {
+      source: this,
+      mode: this.#annotationEditorMode,
+      enable: this.#annotionEditorEnabled,
+    });
+    this.#annotationEditorUIManager.enableEdition(this.#annotionEditorEnabled);
+  }


Sorry, but why?
Please remove this getter/setter pair.

Snuffleupagus · 2022-06-04T11:01:39Z

web/base_viewer.js

+    if (!this.#annotationEditorUIManager) {
+      throw new Error(`The AnnotationEditor is not enabled.`);
+    }
+
+    if (!this.#annotionEditorEnabled) {
+      return;
+    }
+
+    if (!Object.values(AnnotationEditorType).includes(mode)) {
+      throw new Error(`Invalid AnnotationEditor mode: ${mode}`);
+    }
+
+    // If the mode is the same as before, it means that this mode is disabled
+    // and consequently the mode is NONE.
+    this.#annotationEditorMode =
+      this.#annotationEditorMode === mode ? AnnotationEditorType.NONE : mode;
+    this.eventBus.dispatch("annotationeditormodechanged", {
+      source: this,
+      mode: this.#annotationEditorMode,
+    });
+    this.#annotationEditorUIManager.updateMode(this.#annotationEditorMode);


Please implement this as suggested in #14976 (comment) instead.

Snuffleupagus · 2022-06-04T11:03:32Z

web/images/toolbarButton-editorSelectTool.svg

@@ -0,0 +1,4 @@
+<!-- This Source Code Form is subject to the terms of the Mozilla Public


Please let's call this file toolbarButton-editorNone.svg instead.

Snuffleupagus · 2022-06-04T11:21:18Z

web/viewer.html

+                  <button id="editorFreeText" class="toolbarButton" title="Add some free text" role="radio" aria-checked="false" tabindex="32" data-l10n-id="editor_free_text">
+                    <span data-l10n-id="editor_free_text_label">Free Text Annotation</span>


Suggested change

<button id="editorFreeText" class="toolbarButton" title="Add some free text" role="radio" aria-checked="false" tabindex="32" data-l10n-id="editor_free_text">

<span data-l10n-id="editor_free_text_label">Free Text Annotation</span>

<button id="editorFreeText" class="toolbarButton" title="Add FreeText Annotation" role="radio" aria-checked="false" tabindex="32" data-l10n-id="editor_free_text">

<span data-l10n-id="editor_free_text_label">FreeText Annotation</span>

Snuffleupagus · 2022-06-04T11:24:14Z

web/viewer.html

+                  </button>
+                </div>
+
+                <button id="presentationMode" class="toolbarButton hiddenLargeView" title="Switch to Presentation Mode" tabindex="33" data-l10n-id="presentation_mode">


How many new buttons do you foresee adding?

To avoid changing the blame again, can you please increase this and the following tabindexs to account for additional buttons above such that we don't need to touch them again in just a couple of days?

Snuffleupagus · 2022-06-04T11:24:36Z

web/viewer.js

+      editorFreeTextButton: document.getElementById("editorFreeText"),
+      editorSelectButton: document.getElementById("editorSelect"),


Suggested change

editorFreeTextButton: document.getElementById("editorFreeText"),

editorSelectButton: document.getElementById("editorSelect"),

editorNoneButton: document.getElementById("editorNone"),

editorFreeTextButton: document.getElementById("editorFreeText"),

Snuffleupagus · 2022-06-04T11:27:06Z

web/toolbar.js

+ * @property {HTMLButtonElement} editorFreeTextButton - Button to switch to Free
+ *   Text edition.


Suggested change

* @property {HTMLButtonElement} editorFreeTextButton - Button to switch to Free

* Text edition.

* @property {HTMLButtonElement} editorNoneButton - Button to disable editing.

* @property {HTMLButtonElement} editorFreeTextButton - Button to switch to

* FreeText editing.

Snuffleupagus · 2022-06-04T11:29:33Z

src/display/editor/tools.js

+   * Change the edition mode (FreeText, Ink, ...)
+   * @param {number} mode
+   */
+  updateMode(mode) {


Please make sure that this method handles AnnotationEditorType.NONE, and use that to disable editing.
Most likely, the method below is no longer necessary in that case.

Snuffleupagus · 2022-06-04T12:32:24Z

web/viewer.html

+                  <button id="editorSelect" class="toolbarButton" title="Enter in edit mode" role="radio" aria-checked="false" tabindex="31" data-l10n-id="editor_select">
+                    <span data-l10n-id="editor_select_label">Edit Mode</span>


Suggested change

<button id="editorSelect" class="toolbarButton" title="Enter in edit mode" role="radio" aria-checked="false" tabindex="31" data-l10n-id="editor_select">

<span data-l10n-id="editor_select_label">Edit Mode</span>

<button id="editorNone" class="toolbarButton toggled" title="Disable Annotation Editing" role="radio" aria-checked="false" tabindex="31" data-l10n-id="editor_none">

<span data-l10n-id="editor_none_label">Disable Editing</span>

calixteman · 2022-06-04T14:05:52Z

/botio integrationtest

pdfjsbot · 2022-06-04T14:05:54Z

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/d853ee10b3122b1/output.txt

pdfjsbot · 2022-06-04T14:05:54Z

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/a2f0fd5583b4c56/output.txt

pdfjsbot · 2022-06-04T14:10:27Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/d853ee10b3122b1/output.txt

Total script time: 4.54 mins

Integration Tests: FAILED

pdfjsbot · 2022-06-04T14:13:48Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/a2f0fd5583b4c56/output.txt

Total script time: 7.89 mins

Integration Tests: FAILED

calixteman · 2022-06-04T14:26:12Z

Same error on the two bots and I can't of course reproduce locally...

calixteman · 2022-06-04T15:47:30Z

/botio-windows integrationtest

pdfjsbot · 2022-06-04T15:47:31Z

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/edb23ce17a55f03/output.txt

pdfjsbot · 2022-06-04T15:54:28Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/edb23ce17a55f03/output.txt

Total script time: 6.94 mins

Integration Tests: FAILED

calixteman · 2022-06-04T16:03:46Z

It's an issue with Chrome: I'm fixing.

- add a basic UI to edit some text in a pdf; - an editor can be moved, suppressed, cut, copied, pasted, selected; - add an undo/redo manager.

calixteman · 2022-06-04T16:20:55Z

/botio integrationtest

pdfjsbot · 2022-06-04T16:20:56Z

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/e2020feb72c7228/output.txt

pdfjsbot · 2022-06-04T16:20:56Z

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/c55e90fe291cdea/output.txt

pdfjsbot · 2022-06-04T16:25:33Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/e2020feb72c7228/output.txt

Total script time: 4.61 mins

Integration Tests: Passed

pdfjsbot · 2022-06-04T16:30:22Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/c55e90fe291cdea/output.txt

Total script time: 9.43 mins

Integration Tests: FAILED

calixteman · 2022-06-04T16:32:41Z

/botio unittest

pdfjsbot · 2022-06-04T16:32:42Z

From: Bot.io (Windows)

Received

Command cmd_unittest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/887c7989e838d28/output.txt

pdfjsbot · 2022-06-04T16:32:42Z

From: Bot.io (Linux m4)

Received

Command cmd_unittest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/21374491f164cc0/output.txt

pdfjsbot · 2022-06-04T16:36:00Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/21374491f164cc0/output.txt

Total script time: 3.27 mins

Unit Tests: FAILED

pdfjsbot · 2022-06-04T16:38:10Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/887c7989e838d28/output.txt

Total script time: 5.44 mins

Unit Tests: Passed

timvandermeij · 2022-06-05T09:21:16Z

@timvandermeij Since I believe that you originally added the list of AnnotationType, would you object to adding a custom NONE: 0-entry to it to avoid having to add a new and very similar list here?

To come back to this question, which I originally missed in all the comments here, I did indeed make the original enumeration based on the order in the specification, but I wouldn't mind adding a NONE entry if that means we can avoid having to create a second enumeration that more-or-less tracks the same data. On the other hand we most likely won't support all annotation types for editing, so I'm also not really against only listing the ones we actually support in this new enumeration. In that case it would be good however to maybe use the same numbers in the mapping, which I believe is already done now in a follow-up PR.

calixteman requested a review from Snuffleupagus June 1, 2022 08:47

calixteman linked an issue Jun 1, 2022 that may be closed by this pull request

Add support for writing text #14970

Closed

calixteman force-pushed the editor1 branch 4 times, most recently from 60cfaad to aa0b618 Compare June 1, 2022 09:52

calixteman added the edition label Jun 1, 2022

Snuffleupagus requested changes Jun 1, 2022

View reviewed changes

calixteman changed the title ~~[edition] Add a FreeText editor (#14970)~~ [editor] Add a FreeText editor (#14970) Jun 1, 2022

timvandermeij added editor and removed editor labels Jun 1, 2022

Snuffleupagus requested changes Jun 2, 2022

View reviewed changes

calixteman force-pushed the editor1 branch 2 times, most recently from eef68ae to 90497c3 Compare June 3, 2022 13:37

calixteman requested a review from Snuffleupagus June 3, 2022 14:07

Snuffleupagus requested changes Jun 3, 2022

View reviewed changes

Snuffleupagus reviewed Jun 3, 2022

View reviewed changes

src/shared/util.js Outdated Show resolved Hide resolved

Snuffleupagus reviewed Jun 3, 2022

View reviewed changes

calixteman force-pushed the editor1 branch from 90497c3 to 1442bba Compare June 3, 2022 16:48

calixteman requested a review from Snuffleupagus June 3, 2022 16:55

Snuffleupagus reviewed Jun 3, 2022

View reviewed changes

calixteman force-pushed the editor1 branch from 1442bba to 4087822 Compare June 3, 2022 19:14

calixteman requested a review from Snuffleupagus June 3, 2022 19:14

calixteman force-pushed the editor1 branch from 4087822 to bb28c90 Compare June 3, 2022 19:19

Snuffleupagus approved these changes Jun 4, 2022

View reviewed changes

Snuffleupagus reviewed Jun 4, 2022

View reviewed changes

calixteman force-pushed the editor1 branch from bb28c90 to 30c306f Compare June 4, 2022 13:19

[edition] Add a FreeText editor (mozilla#14970)

be1aa11

- add a basic UI to edit some text in a pdf; - an editor can be moved, suppressed, cut, copied, pasted, selected; - add an undo/redo manager.

calixteman force-pushed the editor1 branch from 30c306f to be1aa11 Compare June 4, 2022 16:20

calixteman merged commit c8ef871 into mozilla:master Jun 4, 2022

calixteman deleted the editor1 branch June 4, 2022 17:02

Snuffleupagus mentioned this pull request Jun 4, 2022

[editor] A couple of small FreeText-related fixes (PR 14976 follow-up) #14985

Merged

marco-c mentioned this pull request Jun 8, 2022

Support moving elements around #14974

Closed

ZeroXClem mentioned this pull request Aug 12, 2024

[Snyk] Upgrade pdfjs-dist from 2.9.359 to 2.16.105 ZeroXClem/metamesa#3

Closed

earthywh mentioned this pull request Sep 24, 2024

[Snyk] Upgrade pdfjs-dist from 2.6.347 to 2.16.105 earthywh/filestash#1

Open

	static get platform() {
	const platform = typeof navigator !== "undefined" ? navigator.platform : "";

	return shadow(this, "platform", {
	isWin: platform.includes("Win"),
	isMac: platform.includes("Mac"),
	});
	}

	if (this.constructor === BaseStream) {
	unreachable("Cannot initialize BaseStream.");
	}

		this.#annotationEditorEnabled = options.annotationEditorEnabled === true;
		if (this.#annotationEditorEnabled) {

	this.#bindCursorToolsListener(options);
	this.#bindScrollModeListener(options);
	this.#bindSpreadModeListener(options);

	#bindCursorToolsListener({ cursorSelectToolButton, cursorHandToolButton }) {
	this.eventBus._on("cursortoolchanged", function ({ tool }) {
	const isSelect = tool === CursorTool.SELECT,
	isHand = tool === CursorTool.HAND;

	cursorSelectToolButton.classList.toggle("toggled", isSelect);
	cursorHandToolButton.classList.toggle("toggled", isHand);

	cursorSelectToolButton.setAttribute("aria-checked", isSelect);
	cursorHandToolButton.setAttribute("aria-checked", isHand);
	});
	}

		@@ -0,0 +1,4 @@
		<!-- This Source Code Form is subject to the terms of the Mozilla Public

		<button id="editorFreeText" class="toolbarButton" title="Add some free text" role="radio" aria-checked="false" tabindex="32" data-l10n-id="editor_free_text">
		<span data-l10n-id="editor_free_text_label">Free Text Annotation</span>

		editorFreeTextButton: document.getElementById("editorFreeText"),
		editorSelectButton: document.getElementById("editorSelect"),

		* @property {HTMLButtonElement} editorFreeTextButton - Button to switch to Free
		* Text edition.

		<button id="editorSelect" class="toolbarButton" title="Enter in edit mode" role="radio" aria-checked="false" tabindex="31" data-l10n-id="editor_select">
		<span data-l10n-id="editor_select_label">Edit Mode</span>

[editor] Add a FreeText editor (#14970) #14976

[editor] Add a FreeText editor (#14970) #14976

Conversation

calixteman commented Jun 1, 2022

Snuffleupagus left a comment • edited Loading

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

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

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 Jun 3, 2022 • edited Loading

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 Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jun 3, 2022 • edited Loading

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 Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman commented Jun 4, 2022

pdfjsbot commented Jun 4, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 4, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 4, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 4, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Jun 3, 2022 •

edited

Loading

Snuffleupagus Jun 3, 2022 •

edited

Loading

Snuffleupagus Jun 3, 2022 •

edited

Loading

Snuffleupagus Jun 3, 2022 •

edited

Loading