[JS] Hide field borders and buttons (#15053) #15054

calixteman · 2022-06-17T13:23:39Z

Since the border belongs to the section containing the HTML
counterpart of an annotation, this section must be hidden when
a JS action requires it;
it wasn't possible to hide a button in using JS.

Snuffleupagus · 2022-06-17T14:15:48Z

src/display/annotation_layer.js

@@ -570,6 +570,7 @@ class LinkAnnotationElement extends AnnotationElement {
  render() {
    const { data, linkService } = this;
    const link = document.createElement("a");
+    link.setAttribute("id", data.id);


Why is this necessary, since all containers already provide that information in a data-annotation-id attribute and I cannot immediately tell where in this patch (or the tests) this is even being used?

We "need" it here:

pdf.js/web/pdf_scripting_manager.js

Line 354 in b9f5a70

const element = document.getElementById(elementId);

Most of (all?) the actions have an impact on the field's HTML counterpart (e.g. textarea/input for a text field) so it's why I originally decided to use the id: my idea was to directly have the element which will receive the change.
And I think it wasn't so bad.
I added the id on the a just to be consistent with what we've for other containers.

Actually, the potentially annoying part of adding ids to these elements is that they become "linkable" through the URL hash in a way that's not supported/intended (see issue #11499 and PR #11503 for some context).

These ids could potentially clash with named destinations, and by adding this to all link elements these issues could very well become more common. I wonder if there's a better way, in general for all annotations, to handle things rather than using ids like this!?

At least from a JS pov, I think we can get rid of it easily: we could select the section with the correct data-annotation-id which will itself update the correct child.
I wonder if it could have a measurable impact on perf: query selector vs getElementById and then go to the child...
Anyway I played /f1040.pdf#208R (an annotation on the bottom of page 2) and the form is scrolled...

I wonder if it could have a measurable impact on perf: query selector vs getElementById and then go to the child...

My gut feeling is that query-selector probably is slower in general :-(

Although we're already using that in some parts of the AnnotationLayer-code, see

pdf.js/src/display/annotation_layer.js

Lines 2506 to 2508 in b9f5a70

const elements = div.querySelectorAll(

`[data-annotation-id="${data.id}"]`

);

and

pdf.js/src/display/annotation_layer.js

Line 2553 in b9f5a70

const element = div.querySelector(`[data-annotation-id="${id}"]`);

Anyway I played /f1040.pdf#208R (an annotation on the bottom of page 2) and the form is scrolled...

Which it obviously shouldn't :-)
As mentioned that may cause some trouble if the document has a "named destination" which happens to be called 208R.

Anyway, we obviously don't have to try and fix all of that right now here in this patch!

I filed a bug:
#15056

Snuffleupagus

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

Snuffleupagus · 2022-06-17T14:49:57Z

src/display/annotation_layer.js

+      this._setDefaultPropertiesFromJS(container.lastChild);
+
+      container.lastChild.addEventListener("updatefromsandbox", jsEvent => {


To make it a bit clearer which element this actually references, how about something like the following (assuming that I'm understanding the code correctly)?

Suggested change

this._setDefaultPropertiesFromJS(container.lastChild);

container.lastChild.addEventListener("updatefromsandbox", jsEvent => {

const linkElement = container.lastChild;

this._setDefaultPropertiesFromJS(linkElement);

linkElement.addEventListener("updatefromsandbox", jsEvent => {

- Since the border belongs to the section containing the HTML counterpart of an annotation, this section must be hidden when a JS action requires it; - it wasn't possible to hide a button in using JS.

calixteman · 2022-06-17T15:39:10Z

/botio unittest

pdfjsbot · 2022-06-17T15:39:12Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-06-17T15:39:13Z

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

pdfjsbot · 2022-06-17T15:42:26Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 3.21 mins

Unit Tests: FAILED

pdfjsbot · 2022-06-17T15:45:40Z

From: Bot.io (Windows)

Success

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

Total script time: 6.47 mins

Unit Tests: Passed

calixteman · 2022-06-17T15:46:57Z

/botio integrationtest

pdfjsbot · 2022-06-17T15:46:59Z

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

pdfjsbot · 2022-06-17T15:46:59Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/53d4ed3ef1a1a1f/output.txt

pdfjsbot · 2022-06-17T15:51:29Z

From: Bot.io (Linux m4)

Success

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

Total script time: 4.49 mins

Integration Tests: Passed

pdfjsbot · 2022-06-17T15:54:43Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/53d4ed3ef1a1a1f/output.txt

Total script time: 7.73 mins

Integration Tests: Passed

calixteman added the form-js label Jun 17, 2022

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

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

Acrobat JavaScript Bugs related with Forms #15053

Closed

Snuffleupagus requested changes Jun 17, 2022

View reviewed changes

Snuffleupagus approved these changes Jun 17, 2022

View reviewed changes

calixteman force-pushed the 15053 branch from 247dc6d to dc5f925 Compare June 17, 2022 15:36

[JS] Hide field borders and buttons (mozilla#15053)

7e3941d

- Since the border belongs to the section containing the HTML counterpart of an annotation, this section must be hidden when a JS action requires it; - it wasn't possible to hide a button in using JS.

calixteman force-pushed the 15053 branch from dc5f925 to 7e3941d Compare June 17, 2022 15:36

calixteman mentioned this pull request Jun 17, 2022

[Annotation] Get rid of use of id in the annotation layer #15056

Closed

calixteman merged commit 31e3427 into mozilla:master Jun 17, 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

[JS] Hide field borders and buttons (#15053) #15054

[JS] Hide field borders and buttons (#15053) #15054

calixteman commented Jun 17, 2022

Snuffleupagus Jun 17, 2022

calixteman Jun 17, 2022

Snuffleupagus Jun 17, 2022 •

edited

Loading

calixteman Jun 17, 2022

Snuffleupagus Jun 17, 2022 •

edited

Loading

calixteman Jun 17, 2022

Snuffleupagus left a comment

Snuffleupagus Jun 17, 2022 •

edited

Loading

calixteman commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

calixteman commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

	const elements = div.querySelectorAll(
	`[data-annotation-id="${data.id}"]`
	);

		this._setDefaultPropertiesFromJS(container.lastChild);

		container.lastChild.addEventListener("updatefromsandbox", jsEvent => {

-      this._setDefaultPropertiesFromJS(container.lastChild);
-      container.lastChild.addEventListener("updatefromsandbox", jsEvent => {
+      const linkElement = container.lastChild;
+      this._setDefaultPropertiesFromJS(linkElement);
+      linkElement.addEventListener("updatefromsandbox", jsEvent => {

[JS] Hide field borders and buttons (#15053) #15054

[JS] Hide field borders and buttons (#15053) #15054

Conversation

calixteman commented Jun 17, 2022

Snuffleupagus Jun 17, 2022

Choose a reason for hiding this comment

calixteman Jun 17, 2022

Choose a reason for hiding this comment

Snuffleupagus Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

calixteman Jun 17, 2022

Choose a reason for hiding this comment

Snuffleupagus Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

calixteman Jun 17, 2022

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

calixteman commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 17, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 17, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jun 17, 2022

From: Bot.io (Windows)

Success

calixteman commented Jun 17, 2022

pdfjsbot commented Jun 17, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 17, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 17, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jun 17, 2022

From: Bot.io (Windows)

Success

Snuffleupagus Jun 17, 2022 •

edited

Loading

Snuffleupagus Jun 17, 2022 •

edited

Loading

Snuffleupagus Jun 17, 2022 •

edited

Loading