Support rotating editor layer #15088

calixteman · 2022-06-23T13:52:56Z

As in the annotation layer, use percent instead of pixels as unit;
handle the rotation of the editor layer in allowing editing when rotation
angle is not zero;
the different editors are rotated counterclockwise in order to be usable
when the main page is itself rotated;
add support for saving/printing rotated editors.

Snuffleupagus

r=me, with comments addressed and passing tests; thank you!

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

src/core/document.js

@@ -314,7 +314,6 @@ class Page {
      ...newData.annotations
    );

-    this.xref.resetNewRef();


This was already fixed in PR #15089

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

src/display/annotation_storage.js

@@ -145,6 +145,7 @@ class AnnotationStorage {
      const val = value instanceof AnnotationEditor ? value.serialize() : value;
      clone.set(key, val);
    }
+


Nit: Unrelated change.

Snuffleupagus · 2022-06-24T11:06:19Z

src/display/editor/annotation_editor_layer.js

+
+    this.div.style.width = flipOrientation ? heightStr : widthStr;
+    this.div.style.height = flipOrientation ? widthStr : heightStr;
+    this.div.setAttribute("data-annotation-rotation", rotation);


This looks like too much copy-and-pasting, since I'm assuming that you actually mean the following :-)

Suggested change

this.div.setAttribute("data-annotation-rotation", rotation);

this.div.setAttribute("data-editor-rotation", rotation);

In fact, there is nothing wrong here.
The data-annotation-rotation is a rotation followed by a translation when data-editor-rotation is just a rotation.
The latter is only applied on an editor when the former is on all the containing layer.
So likely, the name data-annotation-rotation is bad, do you have any suggestion ? maybe data-main-rotation ?

So likely, the name data-annotation-rotation is bad, do you have any suggestion ? maybe data-main-rotation ?

Your suggestion works for me, since I'm assuming that you'll be changing the src/display/annotation_layer.js-code accordingly as well?

Also, if we're re-using those CSS-rules for the annotationLayer and the annotationEditorLayer we should probably place those definitions in the https://github.com/mozilla/pdf.js/blob/master/web/pdf_viewer.css file instead.

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

src/display/editor/editor.js

  }

  /**
   * Translate the editor position within its parent.
   * @param {number} x
   * @param {number} y
+   * @param {boolean} isScreen.


This parameter doesn't exist below, please remove it from the JSDoc.

Snuffleupagus · 2022-06-24T11:08:37Z

src/display/editor/editor.js

@@ -131,21 +127,51 @@ class AnnotationEditor {
   * @param {number} x
   * @param {number} y


Please update the JSDoc to also list the new parameters.

Snuffleupagus · 2022-06-24T11:12:15Z

src/display/editor/ink.js

+      case 270:
+        return [height, 0, width, height];
+    }
+    return [0, 0, width, height];


In other switch-statements, for example in screenToPageTranslation, you've used the following format:

Suggested change

case 270:

return [height, 0, width, height];

}

return [0, 0, width, height];

case 270:

return [height, 0, width, height];

default:

return [0, 0, width, height];

Snuffleupagus · 2022-06-24T11:19:11Z

web/annotation_editor_layer_builder.css

@@ -88,6 +98,7 @@
  outline: var(--hover-outline);
 }

+.annotationEditorLayer .inkEditor.disabled:focus,


This line looks identical to the one below, did you mean the following?

Suggested change

.annotationEditorLayer .inkEditor.disabled:focus,

.annotationEditorLayer .freeTextEditor.disabled:focus,

calixteman · 2022-06-24T16:12:31Z

/botio test

pdfjsbot · 2022-06-24T16:12:33Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/96536a0bb76291a/output.txt

pdfjsbot · 2022-06-24T16:12:33Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/9bef82b31a7d175/output.txt

pdfjsbot · 2022-06-24T16:38:52Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/96536a0bb76291a/output.txt

Total script time: 26.30 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 24

Image differences available at: http://54.241.84.105:8877/96536a0bb76291a/reftest-analyzer.html#web=eq.log

pdfjsbot · 2022-06-24T16:41:35Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/9bef82b31a7d175/output.txt

Total script time: 29.02 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/9bef82b31a7d175/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2022-06-24T17:14:10Z

src/display/editor/editor.js

@@ -130,22 +126,53 @@ class AnnotationEditor {
   * Set the editor position within its parent.
   * @param {number} x
   * @param {number} y
+   * @param {number} tx - x-translation in screen coordinates.
+   * @param {number} ty - xy-translation in screen coordinates.


Typo?

Suggested change

* @param {number} ty - xy-translation in screen coordinates.

* @param {number} ty - y-translation in screen coordinates.

Snuffleupagus · 2022-06-24T17:14:32Z

src/display/editor/editor.js

  }

  /**
   * Translate the editor position within its parent.
+   * @param {number} x - x-translation in screen coordinates.
+   * @param {number} y - xy-translation in screen coordinates.


Typo?

Suggested change

* @param {number} y - xy-translation in screen coordinates.

* @param {number} y - y-translation in screen coordinates.

- As in the annotation layer, use percent instead of pixels as unit; - handle the rotation of the editor layer in allowing editing when rotation angle is not zero; - the different editors are rotated counterclockwise in order to be usable when the main page is itself rotated; - add support for saving/printing rotated editors.

Snuffleupagus · 2022-06-25T08:35:26Z

web/annotation_layer_builder.css

-[data-annotation-rotation="90"] {
-  transform: rotate(90deg) translateY(-100%);
-}
-[data-annotation-rotation="180"] {
-  transform: rotate(180deg) translate(-100%, -100%);
-}
-[data-annotation-rotation="270"] {
-  transform: rotate(270deg) translateX(-100%);
-}
-


To ensure that moving this code won't cause any issues now, or in the future, I've submitted PR #15101 which should help avoid any problems.

calixteman · 2022-06-25T13:44:01Z

/botio test

pdfjsbot · 2022-06-25T13:44:03Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/1a191ba09fbe6da/output.txt

pdfjsbot · 2022-06-25T13:44:03Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/6c98ec426c59d97/output.txt

pdfjsbot · 2022-06-25T14:10:21Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/1a191ba09fbe6da/output.txt

Total script time: 26.30 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 21

Image differences available at: http://54.241.84.105:8877/1a191ba09fbe6da/reftest-analyzer.html#web=eq.log

pdfjsbot · 2022-06-25T14:12:36Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/6c98ec426c59d97/output.txt

Total script time: 28.54 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: Passed

calixteman · 2022-06-25T14:18:19Z

/botio makeref

pdfjsbot · 2022-06-25T14:18:20Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/6b871846394b1a5/output.txt

pdfjsbot · 2022-06-25T14:18:20Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/59a37956d8becec/output.txt

pdfjsbot · 2022-06-25T14:41:09Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/6b871846394b1a5/output.txt

Total script time: 22.80 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-06-25T14:43:59Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/59a37956d8becec/output.txt

Total script time: 22.40 mins

Lint: Passed
Make references: Passed
Check references: Passed

calixteman added the editor label Jun 23, 2022

calixteman requested a review from Snuffleupagus June 23, 2022 13:52

Snuffleupagus approved these changes Jun 24, 2022

View reviewed changes

calixteman force-pushed the editor_rotation branch from 2761f06 to 0830d4b Compare June 24, 2022 16:12

Snuffleupagus reviewed Jun 24, 2022

View reviewed changes

calixteman force-pushed the editor_rotation branch from 0830d4b to 0c420f5 Compare June 24, 2022 18:02

Snuffleupagus reviewed Jun 25, 2022

View reviewed changes

calixteman merged commit 23fcdab into mozilla:master Jun 25, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support rotating editor layer #15088

Support rotating editor layer #15088

calixteman commented Jun 23, 2022

Snuffleupagus left a comment

Snuffleupagus Jun 24, 2022

Snuffleupagus Jun 24, 2022

Snuffleupagus Jun 24, 2022

calixteman Jun 24, 2022 •

edited

Loading

Snuffleupagus Jun 24, 2022

Snuffleupagus Jun 24, 2022

Snuffleupagus Jun 24, 2022

Snuffleupagus Jun 24, 2022

Snuffleupagus Jun 24, 2022

calixteman commented Jun 24, 2022

pdfjsbot commented Jun 24, 2022

pdfjsbot commented Jun 24, 2022

pdfjsbot commented Jun 24, 2022

pdfjsbot commented Jun 24, 2022

Snuffleupagus Jun 24, 2022

Snuffleupagus Jun 24, 2022

Snuffleupagus Jun 25, 2022

calixteman commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

calixteman commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

	this.div.setAttribute("data-annotation-rotation", rotation);
	this.div.setAttribute("data-editor-rotation", rotation);

		@@ -131,21 +127,51 @@ class AnnotationEditor {
		* @param {number} x
		* @param {number} y

	.annotationEditorLayer .inkEditor.disabled:focus,
	.annotationEditorLayer .freeTextEditor.disabled:focus,

	* @param {number} ty - xy-translation in screen coordinates.
	* @param {number} ty - y-translation in screen coordinates.

	* @param {number} y - xy-translation in screen coordinates.
	* @param {number} y - y-translation in screen coordinates.

Support rotating editor layer #15088

Support rotating editor layer #15088

Conversation

calixteman commented Jun 23, 2022

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

calixteman Jun 24, 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

calixteman commented Jun 24, 2022

pdfjsbot commented Jun 24, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 24, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 24, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 24, 2022

From: Bot.io (Windows)

Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 25, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 25, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 25, 2022

From: Bot.io (Windows)

Success

calixteman commented Jun 25, 2022

pdfjsbot commented Jun 25, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 25, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 25, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jun 25, 2022

From: Bot.io (Windows)

Success

calixteman Jun 24, 2022 •

edited

Loading