Rotate annotations based on the MK::R value (bug 1675139) #15060

calixteman · 2022-06-19T14:44:51Z

it aims to fix: https://bugzilla.mozilla.org/show_bug.cgi?id=1675139;
An annotation can be rotated (counterclockwise);
the rotation can be set in using JS.

Snuffleupagus

Does this perhaps also fix issue #9596?

Snuffleupagus · 2022-06-19T18:19:04Z

src/core/annotation.js

+    this.rotation = 0;
+    if (mk instanceof Dict) {
+      let angle = mk.get("R") || 0;
+      if (angle !== 0) {


Let's ensure that the angle is also an integer, since otherwise none of the code below makes sense.

Suggested change

if (angle !== 0) {

if (Number.isInteger(angle) && angle !== 0) {

Snuffleupagus · 2022-06-19T18:20:47Z

src/core/annotation.js

+        // If the angle is negative then angle % 360 is still negative.
+        angle = (360 + (angle % 360)) % 360;


I'm finding this a bit less clear than what we have in other parts of the code-base, see e.g.

pdf.js/src/display/display_utils.js

Lines 179 to 183 in 45de73b

// Normalize the rotation, by clamping it to the [0, 360) range.

rotation %= 360;

if (rotation < 0) {

rotation += 360;

}

and

pdf.js/web/base_viewer.js

Lines 509 to 513 in 45de73b

// Normalize the rotation, by clamping it to the [0, 360) range.

rotation %= 360;

if (rotation < 0) {

rotation += 360;

}

Snuffleupagus · 2022-06-19T18:25:45Z

src/core/annotation.js

+      case 270:
+        return [0, -1, 1, 0, 0, height];
+      default:
+        return [1, 0, 0, 1, 0, 0];


This should be unreachable, given the if (rotation === 0) { ... } block above, right?
Hence I'd suggest replacing this with:

Suggested change

return [1, 0, 0, 1, 0, 0];

throw new Error("Invalid rotation");

Snuffleupagus · 2022-06-19T18:29:07Z

src/display/annotation_layer.js

+  setRotation(angle, container) {
+    container ||= this.container;


Can't we just do the following?

Suggested change

setRotation(angle, container) {

container ||= this.container;

setRotation(angle, container = this.container) {

src/scripting_api/field.js

Snuffleupagus · 2022-06-19T18:35:11Z

src/scripting_api/field.js

+    if (angle % 90 !== 0) {
+      throw new Error("Invalid rotation: must be a multiple of 90");
+    }
+    this._rotation = (360 + (angle % 360)) % 360;


As above, regarding the normalization of the rotation angle.

test/test_manifest.json

Snuffleupagus

What about the question in #15060 (review) :-)

r=me, with passing tests; thank you!

Snuffleupagus · 2022-06-20T08:29:45Z

src/core/annotation.js

+        // If the angle is negative then angle % 360 is still negative.
+        angle %= 360;
+        angle = angle >= 0 ? angle : 360 + angle;


Can't we just do exactly what's done elsewhere in the code-base, since re-assigning the value when it's non-negative just feels unnecessary?

Suggested change

// If the angle is negative then angle % 360 is still negative.

angle %= 360;

angle = angle >= 0 ? angle : 360 + angle;

// Normalize the rotation, by clamping it to the [0, 360) range.

angle %= 360;

if (angle < 0) {

angle += 360;

}

calixteman · 2022-06-20T09:03:49Z

/botio test

pdfjsbot · 2022-06-20T09:03:50Z

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/1ceb5d3b61383b5/output.txt

pdfjsbot · 2022-06-20T09:03:50Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/13263b2150144ac/output.txt

pdfjsbot · 2022-06-20T09:27:15Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 23.40 mins

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

  different ref/snapshot: 70
  different first/second rendering: 1

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

pdfjsbot · 2022-06-20T09:28:45Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/13263b2150144ac/output.txt

Total script time: 24.89 mins

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

  different ref/snapshot: 26

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

calixteman · 2022-06-20T15:26:41Z

/botio test

pdfjsbot · 2022-06-20T15:26:51Z

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/24f1c535738c29a/output.txt

pdfjsbot · 2022-06-20T15:26:51Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/3de612af212205b/output.txt

pdfjsbot · 2022-06-20T15:51:02Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/24f1c535738c29a/output.txt

Total script time: 24.16 mins

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

  different ref/snapshot: 41
  different first/second rendering: 1

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

pdfjsbot · 2022-06-20T15:52:06Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/3de612af212205b/output.txt

Total script time: 25.24 mins

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

  different ref/snapshot: 10

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

calixteman · 2022-06-21T07:45:55Z

/botio test

pdfjsbot · 2022-06-21T07:45:56Z

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/e737b527aafe572/output.txt

pdfjsbot · 2022-06-21T07:45:56Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-06-21T08:00:32Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 14.58 mins

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

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

pdfjsbot · 2022-06-21T08:14:21Z

From: Bot.io (Windows)

Failed

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

Total script time: 28.40 mins

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

  different ref/snapshot: 4

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

calixteman · 2022-06-21T10:20:45Z

/botio browsertest

pdfjsbot · 2022-06-21T10:20:46Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-06-21T10:20:46Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-06-21T10:42:03Z

From: Bot.io (Windows)

Failed

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

Total script time: 21.26 mins

Regression tests: FAILED

  different ref/snapshot: 2

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

pdfjsbot · 2022-06-21T10:46:28Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 21.89 mins

Regression tests: FAILED

  different ref/snapshot: 25
  different first/second rendering: 1

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

calixteman · 2022-06-21T12:11:38Z

@Snuffleupagus, would you mind to review this patch again ?
I had to make few changes to handle correctly save/print stuff and to support correctly all the different type of fields.

Snuffleupagus · 2022-06-21T13:14:56Z

test/integration/scripting_spec.js

+      await closePages(pages);
+    });
+
+    it("must data-annotation-attribute has the correct value", async () => {


Did you mean something like the following here?

Suggested change

it("must data-annotation-attribute has the correct value", async () => {

it("must check that data-annotation-rotation is correct", async () => {

Snuffleupagus · 2022-06-21T13:22:07Z

src/core/annotation.js

+    if (mk instanceof Dict) {
+      let angle = mk.get("R") || 0;
+      if (Number.isInteger(angle) && angle !== 0) {
+        // If the angle is negative then angle % 360 is still negative.


This comment can probably be removed.

Snuffleupagus · 2022-06-21T13:34:47Z

src/core/annotation.js

+    }
+  }
+
+  getRotationMatrix(annotationStorage) {


Is this necessary for all Annotation-types, or can we perhaps move it into the WidgetAnnotation-class instead?

Snuffleupagus · 2022-06-21T13:35:12Z

src/core/annotation.js

+    if (rotation || this.borderColor || this.backgroundColor) {
+      const MK = new Dict(xref);
+      MK.set("R", rotation);
+      MK.set("BC", getPDFRgbArray(this.borderColor));
+      MK.set("BG", getPDFRgbArray(this.backgroundColor));
+      dict.set("MK", MK);
+    }


This code is repeated multiple times, how about adding a helper-method on the WidgetAnnotation-class that returns a MK-dict when that's necessary?

Also, do we need to populate some of the dictionary entries with empty Arrays (and is that even generally correct)?

Maybe if you add a method like this, on the WidgetAnnotation-class:

_getMKDict(rotation) { const mk = new Dict(null); // `null` can probably be used here, since none of the data below will be a /Ref if (rotation) { mk.set("R", rotation); } if (this.borderColor) { mk.set("BC", Array.from(this.borderColor).map(c => c / 255)); } if (this.backgroundColor) { mk.set("BG", Array.from(this.backgroundColor).map(c => c / 255)); } return mk.size > 0 ? mk : null; }

and then you can simply do (here and elsewhere):

Suggested change

if (rotation || this.borderColor || this.backgroundColor) {

const MK = new Dict(xref);

MK.set("R", rotation);

MK.set("BC", getPDFRgbArray(this.borderColor));

MK.set("BG", getPDFRgbArray(this.backgroundColor));

dict.set("MK", MK);

}

const maybeMK = this._getMKDict(rotation);

if (maybeMK) {

dict.set("MK", maybeMK);

}

Snuffleupagus · 2022-06-21T13:37:27Z

src/core/annotation.js

@@ -721,13 +778,24 @@ class Annotation {
    }
  }

-  getBorderAndBackgroundAppearances() {
+  getBorderAndBackgroundAppearances(annotationStorage) {


This is existing code, but the question above (regarding WidgetAnnotation) applies here as well.

Snuffleupagus · 2022-06-21T13:48:36Z

src/core/annotation.js

+    }
+
+    if (rotation === 0) {
+      return [1, 0, 0, 1, 0, 0];


I don't know how much it matters, but you may want to return IDENTITY_MATRIX here (can be imported from src/shared/util.js)?

Snuffleupagus · 2022-06-21T13:50:00Z

src/core/annotation.js

+    if (rotationMatrix[0] !== 1) {
+      // The matrix isn't the identity one.


I'm not sure how much it matters here, but with a previous comment implemented you could just do the following here:

Suggested change

if (rotationMatrix[0] !== 1) {

// The matrix isn't the identity one.

if (rotationMatrix === IDENTITY_MATRIX) {

Snuffleupagus · 2022-06-21T13:51:21Z

src/core/annotation.js

@@ -2220,6 +2343,17 @@ class ButtonWidgetAnnotation extends WidgetAnnotation {
      : this.uncheckedAppearance;
    if (appearance) {
      const savedAppearance = this.appearance;
+      const savedMatrix = appearance.dict.getArray("Matrix") || [
+        1, 0, 0, 1, 0, 0,


Again, you could maybe just fallback on IDENTITY_MATRIX here?

Snuffleupagus · 2022-06-21T13:52:35Z

src/core/annotation.js

+    if (rotation || this.borderColor || this.backgroundColor) {
+      const MK = new Dict(evaluator.xref);
+      MK.set("R", rotation);
+      MK.set("BC", getPDFRgbArray(this.borderColor));
+      MK.set("BG", getPDFRgbArray(this.backgroundColor));
+      dict.set("MK", MK);
+    }
+


Please see previous comments.

Snuffleupagus · 2022-06-21T13:52:40Z

src/core/annotation.js

+    if (rotation || this.borderColor || this.backgroundColor) {
+      const MK = new Dict(evaluator.xref);
+      MK.set("R", rotation);
+      MK.set("BC", getPDFRgbArray(this.borderColor));
+      MK.set("BG", getPDFRgbArray(this.backgroundColor));
+      dict.set("MK", MK);
+    }
+


Please see previous comments.

calixteman · 2022-06-21T15:19:46Z

/botio test

pdfjsbot · 2022-06-21T15:19:48Z

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/3273fbcaed07a30/output.txt

pdfjsbot · 2022-06-21T15:19:48Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-06-21T15:46:01Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/3273fbcaed07a30/output.txt

Total script time: 26.20 mins

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

  different ref/snapshot: 31

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

pdfjsbot · 2022-06-21T15:48:13Z

From: Bot.io (Windows)

Failed

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

Total script time: 28.40 mins

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

  different ref/snapshot: 2

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

calixteman · 2022-06-21T15:54:05Z

The "regression" is due to a rotation of 270deg on a checkbox and this one is not exactly a square, hence the change, so it's an improvement.

Snuffleupagus · 2022-06-21T15:55:24Z

src/core/annotation.js

@@ -1535,20 +1596,42 @@ class WidgetAnnotation extends Annotation {
    );
  }

+  _getMKDict(rotation) {
+    const mk = new Dict(null); // `null` can probably be used here, since none of the data below will be a /Ref


Please remove the comment, since I only included it to explain my reasoning as part of the review comment.

- it aims to fix: https://bugzilla.mozilla.org/show_bug.cgi?id=1675139; - An annotation can be rotated (counterclockwise); - the rotation can be set in using JS.

calixteman · 2022-06-21T16:03:18Z

/botio makeref

pdfjsbot · 2022-06-21T16:03:20Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/88023865608c3bd/output.txt

pdfjsbot · 2022-06-21T16:03:20Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-06-21T16:35:07Z

From: Bot.io (Linux m4)

Success

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

Total script time: 22.92 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-06-21T16:42:10Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/88023865608c3bd/output.txt

Total script time: 22.33 mins

Lint: Passed
Make references: Passed
Check references: Passed

calixteman added the form-js label Jun 19, 2022

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

calixteman force-pushed the annotation_rotation branch from 14c609a to 73a05e5 Compare June 19, 2022 14:57

Snuffleupagus reviewed Jun 19, 2022

View reviewed changes

test/test_manifest.json Show resolved Hide resolved

calixteman force-pushed the annotation_rotation branch from 73a05e5 to b4e3f79 Compare June 19, 2022 20:04

Snuffleupagus approved these changes Jun 20, 2022

View reviewed changes

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

Acroform Field not rotating with document #9596

Closed

calixteman force-pushed the annotation_rotation branch from b4e3f79 to 3bebf31 Compare June 20, 2022 09:01

calixteman force-pushed the annotation_rotation branch from 3bebf31 to 715d61d Compare June 20, 2022 15:26

calixteman force-pushed the annotation_rotation branch from 715d61d to 4d5ab84 Compare June 21, 2022 07:29

calixteman force-pushed the annotation_rotation branch from 4d5ab84 to 863b4d0 Compare June 21, 2022 08:39

marco-c mentioned this pull request Jun 21, 2022

Acroform Field not rotating with document #9596

Closed

Snuffleupagus reviewed Jun 21, 2022

View reviewed changes

calixteman force-pushed the annotation_rotation branch from 863b4d0 to 34ba918 Compare June 21, 2022 15:19

Snuffleupagus approved these changes Jun 21, 2022

View reviewed changes

Rotate annotations based on the MK::R value (bug 1675139)

cdc58b7

- it aims to fix: https://bugzilla.mozilla.org/show_bug.cgi?id=1675139; - An annotation can be rotated (counterclockwise); - the rotation can be set in using JS.

calixteman force-pushed the annotation_rotation branch from 34ba918 to cdc58b7 Compare June 21, 2022 15:57

calixteman merged commit 8d466f5 into mozilla:master Jun 21, 2022

calixteman mentioned this pull request Dec 6, 2022

Set the dimensions of the various layers at their creation #15770

Merged

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

	if (angle !== 0) {
	if (Number.isInteger(angle) && angle !== 0) {

		// If the angle is negative then angle % 360 is still negative.
		angle = (360 + (angle % 360)) % 360;

	// Normalize the rotation, by clamping it to the [0, 360) range.
	rotation %= 360;
	if (rotation < 0) {
	rotation += 360;
	}

	return [1, 0, 0, 1, 0, 0];
	throw new Error("Invalid rotation");

		setRotation(angle, container) {
		container \|\|= this.container;

	setRotation(angle, container) {
	container \|\|= this.container;
	setRotation(angle, container = this.container) {

	it("must data-annotation-attribute has the correct value", async () => {
	it("must check that data-annotation-rotation is correct", async () => {

		if (rotationMatrix[0] !== 1) {
		// The matrix isn't the identity one.

	if (rotationMatrix[0] !== 1) {
	// The matrix isn't the identity one.
	if (rotationMatrix === IDENTITY_MATRIX) {

Rotate annotations based on the MK::R value (bug 1675139) #15060

Rotate annotations based on the MK::R value (bug 1675139) #15060

Conversation

calixteman commented Jun 19, 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

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

calixteman commented Jun 20, 2022

pdfjsbot commented Jun 20, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 20, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 20, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 20, 2022

From: Bot.io (Windows)

Failed

calixteman commented Jun 20, 2022

pdfjsbot commented Jun 20, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 20, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 20, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 20, 2022

From: Bot.io (Windows)

Failed

calixteman commented Jun 21, 2022

pdfjsbot commented Jun 21, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 21, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 21, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 21, 2022

From: Bot.io (Windows)

Failed

calixteman commented Jun 21, 2022

pdfjsbot commented Jun 21, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 21, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 21, 2022

From: Bot.io (Windows)

Failed

pdfjsbot commented Jun 21, 2022

From: Bot.io (Linux m4)

Failed

calixteman commented Jun 21, 2022

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 21, 2022

pdfjsbot commented Jun 21, 2022