Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 17, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/display/annotation_layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ class AnnotationElement {
return shadow(this, "_commonActions", {
display: event => {
const hidden = event.detail.display % 2 === 1;
event.target.style.visibility = hidden ? "hidden" : "visible";
this.container.style.visibility = hidden ? "hidden" : "visible";
this.annotationStorage.setValue(this.data.id, {
hidden,
print: event.detail.display === 0 || event.detail.display === 3,
Expand All @@ -321,7 +321,7 @@ class AnnotationElement {
});
},
hidden: event => {
event.target.style.visibility = event.detail.hidden
this.container.style.visibility = event.detail.hidden
? "hidden"
: "visible";
this.annotationStorage.setValue(this.data.id, {
Expand Down Expand Up @@ -570,6 +570,7 @@ class LinkAnnotationElement extends AnnotationElement {
render() {
const { data, linkService } = this;
const link = document.createElement("a");
link.setAttribute("id", data.id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

@Snuffleupagus Snuffleupagus Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed a bug:
#15056

let isBound = false;

if (data.url) {
Expand Down Expand Up @@ -1429,7 +1430,14 @@ class PushButtonWidgetAnnotationElement extends LinkAnnotationElement {
container.title = this.data.alternativeText;
}

this._setDefaultPropertiesFromJS(container);
if (this.enableScripting && this.hasJSActions) {
const linkElement = container.lastChild;
this._setDefaultPropertiesFromJS(linkElement);

linkElement.addEventListener("updatefromsandbox", jsEvent => {
this._dispatchEventFromSandbox({}, jsEvent);
});
}

return container;
}
Expand Down
52 changes: 52 additions & 0 deletions test/integration/scripting_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1336,4 +1336,56 @@ describe("Interaction", () => {
);
});
});

describe("in issue15053.pdf", () => {
let pages;

beforeAll(async () => {
pages = await loadAndWait("issue15053.pdf", "#\\34 4R");
});

afterAll(async () => {
await closePages(pages);
});

it("must check that a button and text field with a border are hidden", async () => {
await Promise.all(
pages.map(async ([browserName, page]) => {
await page.waitForFunction(
"window.PDFViewerApplication.scriptingReady === true"
);

let visibility = await page.$eval(
"[data-annotation-id='35R']",
el => getComputedStyle(el).visibility
);
expect(visibility)
.withContext(`In ${browserName}`)
.toEqual("visible");

visibility = await page.$eval(
"[data-annotation-id='51R']",
el => getComputedStyle(el).visibility
);
expect(visibility)
.withContext(`In ${browserName}`)
.toEqual("visible");

await page.click("#\\34 4R");

visibility = await page.$eval(
"[data-annotation-id='35R']",
el => getComputedStyle(el).visibility
);
expect(visibility).withContext(`In ${browserName}`).toEqual("hidden");

visibility = await page.$eval(
"[data-annotation-id='51R']",
el => getComputedStyle(el).visibility
);
expect(visibility).withContext(`In ${browserName}`).toEqual("hidden");
})
);
});
});
});
1 change: 1 addition & 0 deletions test/pdfs/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,3 +527,4 @@
!issue14705.pdf
!bug1771477.pdf
!bug1724918.pdf
!issue15053.pdf
Binary file added test/pdfs/issue15053.pdf
View file
Open in desktop
Binary file not shown.