Fix XFA links (bug 1735738) #14153

cathydev · 2021-10-16T18:22:41Z

At the end the anchor tag inside the button wasn't the problem here, after comparing the reference file and this one

https://bug1716758.bmoattachments.org/attachment.cgi?id=9240360

I realized that in the reference file the anchor tag has an inline display and couldn't receive any height. After changing the display to inline-block it started working but since it receives a 100% height the text wasn't showing properly so I added an absolute position and now the reference file it's working in both Firefox nightly and Chrome.

It also solves a bug in this file

https://bug1716758.bmoattachments.org/attachment.cgi?id=9240360

Where the second link in schedule II wasn't working at all, now all links in both files are working just fine.

Thank you @calixteman for explaining me with so much patience!

closes #14139

Snuffleupagus · 2021-10-16T20:22:31Z

Can you please provide a bit more detail in the commit message as well (since it's currently a single sentence), and not just in the PR description?
Generally it's always a very good idea if it's possible to understand what a patch does and why from the commit message too, since that's often most helpful when looking at a patch on the Git command line (e.g. during bisecting).

cathydev · 2021-10-16T20:54:20Z

Can you please provide a bit more detail in the commit message as well (since it's currently a single sentence), and not just in the PR description? Generally it's always a very good idea if it's possible to understand what a patch does and why from the commit message too, since that's often most helpful when looking at a patch on the Git command line (e.g. during bisecting).

Yes, it's no problem. Sorry for that, I'll make sure to be more specific in my commits from now on

Snuffleupagus · 2021-10-17T15:25:20Z

The commit message is still just a single sentence, and doesn't really contain all that much more information. Please imagine that you're looking at this commit locally, without using GitHub, in e.g. one month's time. Will it still help you understand what you changed and (importantly) why the implementation looks the way it does?

In #14153 (comment) you've got the following paragraph, which definitely seems like a good thing to also add to the commit message:

I realized that in the reference file the anchor tag has an inline display and couldn't receive any height. After changing the display to inline-block it started working but since it receives a 100% height the text wasn't showing properly so I added an absolute position and now the reference file it's working in both Firefox nightly and Chrome.

calixteman · 2021-10-18T08:03:56Z

web/xfa_layer_builder.css

@@ -221,6 +221,8 @@
 }

 .xfaLink {
+  display: inline-block;


I think display: inline-block is useless here.
Could you elaborate on why you added it ?

Yes, as I mentioned I added it because the inline display can't receive any height. When changing the display the link start working and the absolute position it's for the text in the links to show properly.

Ok, I see it's still working with just the absolute position. Do you want me to remove the display?

calixteman · 2021-10-19T14:41:57Z

I think we should add a reftest for that to avoid future regressions.
Maybe we could do something similar to what we have for annotations, see:
https://github.com/mozilla/pdf.js/blob/master/test/annotation_layer_builder_overrides.css

@catherinemds, would you mind to do that ?

cathydev · 2021-10-19T16:32:17Z

I think we should add a reftest for that to avoid future regressions. Maybe we could do something similar to what we have for annotations, see: https://github.com/mozilla/pdf.js/blob/master/test/annotation_layer_builder_overrides.css

@catherinemds, would you mind to do that ?

Sure, no problem.

calixteman · 2021-10-25T09:45:31Z

test/test_manifest.json

+      "file": "pdfs/xfa_bug1735738.pdf",
+      "md5": "7aa91f6681798c48e0c9d9836ed30742",
+      "rounds": 1,
+      "type": "eq"


You should add "enableXfa": true else the pdf won't rendered correctly.

Also, are we allowed to include the file or should it be a link test-case instead?

Oh yes, correct.
@catherinemds, please add a file named xfa_bug1735738.pdf.link which contains only the url to the pdf which is in the bugzilla bug and remove from this PR `xfa_bug1735738.pdf`` you added.

calixteman · 2021-10-25T09:50:37Z

@catherinemds, you added a test: nice.
You should run a gulp makeref locally and check the output for this test.
You'll notice that we can't "see" the link on the generated image: it's why you should add a css file for xfa tests (similarly to what it have been done for annotations with https://github.com/mozilla/pdf.js/blob/master/test/annotation_layer_builder_overrides.css) and modify the style for xfaLink in order to be able to "see" it.
If you're a bit impatient, you can remove all the tests from test_manifest.json but the one you added: it should help you to go faster.

cathydev · 2021-10-26T18:13:08Z

Hey @calixteman I submitted the changes even though I could still see the yellow box from the override file after commenting my modifications, I'm struggling to understand what when wrong here, also I wonder if the changes made to the driver.js file are okay

calixteman · 2021-10-26T18:15:55Z

Could you try to see if there is any difference in removing the display:block in th overriden css ?

cathydev · 2021-10-26T18:20:20Z

Could you try to see if there is any difference in removing the display:block in th overriden css ?

Yes, I tried that. The yellow box is almost gone but you can still see a bit of it in a little corner of the button

calixteman · 2021-10-26T19:04:59Z

It's likely due to the box-shadow.
Could you fix the test_manifest file please ? There are a way too much change here ?

cathydev · 2021-10-26T23:00:27Z

@calixteman I noticed something here, the litle corner where the yellow box it's still showing it's not the same xfalink that contains that "www.assuredynamics.com" link.
What I mean it's... The specific xfaLink that wasn't working where I applied my modification has this id "linkbutton12", the one with the yellow box it's "linkbutton48"...I think we could say that my patch has an effect?

Sorry for pushing so many times, I made a mistake and the .gitignore file updated and somehow I pushed it and had to fix it

Snuffleupagus · 2021-10-27T06:46:06Z

test/pdfs/xfa_bug1735738.pdf.link

@@ -0,0 +1 @@
+https://bugzilla.mozilla.org/attachment.cgi?id=9227382


Missing a line-break at the end here.

Your code-editor should be able to help you enforce this, thanks to this file; please see https://editorconfig.org/#pre-installed or https://editorconfig.org/#download

Snuffleupagus · 2021-10-27T06:48:08Z

test/xfa_layer_builder_overrides.css

@@ -0,0 +1,5 @@
+.xfaLink {


This file is missing a license header, please copy the one found e.g. in https://github.com/mozilla/pdf.js/blob/master/test/annotation_layer_builder_overrides.css (obviously with the year adjusted).

calixteman · 2021-10-27T13:24:13Z

test/driver.js

-        .then(async ([cssRules]) => {
-          style.textContent = fontRules + "\n" + cssRules;
+        .then(async ([common, overrides]) => {
+          style.textContent = common + "\n" + overrides;


Why did you remove fontRules ?

Yeah sorry, I was following the example of rasterizeAnnotationLayer and I removed to make it work at first but I fixed it now, I ran gulp makeref again to make sure it still working and everything seems fine

Snuffleupagus · 2021-10-28T07:35:21Z

test/driver.js

+        .then(async ([cssRules, common, overrides]) => {
+          style.textContent =
+            fontRules + "\n" + cssRules + "\n" + common + "\n" + overrides;


Have you successfully tested this, since I don't believe that the style promise will return an Array with three elements?
Based on the styles definition above, I'd expect that the correct thing here would be:

.then(async ([common, overrides]) => { style.textContent = fontRules + "\n" + common + "\n" + overrides;

calixteman · 2021-10-28T14:35:29Z

/botio xfatest

pdfjsbot · 2021-10-28T14:35:30Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2021-10-28T14:35:31Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2021-10-28T14:44:44Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 9.21 mins

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

  errors: 30
  different ref/snapshot: 28

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

pdfjsbot · 2021-10-28T14:51:23Z

From: Bot.io (Windows)

Failed

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

Total script time: 15.87 mins

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

  errors: 30
  different ref/snapshot: 29

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

calixteman · 2021-10-29T11:53:13Z

The reftests are showing links at incorrect positions.
I guess it's fine from a user point of view but it's a bit strange when we look at the images.
I think setting top and left to 0px should fix this small issue. Could you make a try and see if it improves things ?

There were some links not working in some XFA files,I realized that the anchor tag that contains the link has an inline display and couldn't receive any height, solved this by adding a "position: absolute". Tested with two different files in Firefox Nightly and Chrome and now all links are working perfectly fine. Added reftest to avoid future regressions

calixteman · 2021-10-29T15:41:43Z

/botio test

pdfjsbot · 2021-10-29T15:41:44Z

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

pdfjsbot · 2021-10-29T15:41:44Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/114d54c44ff3527/output.txt

pdfjsbot · 2021-10-29T16:04:36Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/93de029e54a7f12/output.txt

Total script time: 22.84 mins

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

  different ref/snapshot: 39

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

pdfjsbot · 2021-10-29T16:25:02Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/114d54c44ff3527/output.txt

Total script time: 43.28 mins

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

  different ref/snapshot: 39

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

calixteman

LGTM, thank you for doing this.

pdfjsbot · 2021-10-29T18:39:39Z

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

pdfjsbot · 2021-10-29T18:39:39Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2021-10-29T19:00:17Z

From: Bot.io (Linux m4)

Success

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

Total script time: 20.61 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2021-10-29T19:20:51Z

From: Bot.io (Windows)

Success

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

Total script time: 39.03 mins

Lint: Passed
Make references: Passed
Check references: Passed

cathydev force-pushed the xfa-link branch from 6a8f65a to 48e2531 Compare October 16, 2021 20:52

cathydev force-pushed the xfa-link branch from 48e2531 to 6c84748 Compare October 17, 2021 17:01

calixteman reviewed Oct 18, 2021

View reviewed changes

cathydev force-pushed the xfa-link branch from 6c84748 to 77c1072 Compare October 18, 2021 15:46

timvandermeij added viewer form-xfa labels Oct 19, 2021

cathydev force-pushed the xfa-link branch from 77c1072 to 9d26790 Compare October 24, 2021 12:20

calixteman reviewed Oct 25, 2021

View reviewed changes

cathydev force-pushed the xfa-link branch from 9d26790 to 5911408 Compare October 26, 2021 18:07

cathydev force-pushed the xfa-link branch 4 times, most recently from 1fdf6b0 to 0ce9a0e Compare October 26, 2021 22:58

Snuffleupagus reviewed Oct 27, 2021

View reviewed changes

cathydev force-pushed the xfa-link branch from 0ce9a0e to 2677267 Compare October 27, 2021 13:21

calixteman reviewed Oct 27, 2021

View reviewed changes

cathydev force-pushed the xfa-link branch from 2677267 to f667258 Compare October 28, 2021 01:56

Snuffleupagus reviewed Oct 28, 2021

View reviewed changes

cathydev force-pushed the xfa-link branch from f667258 to e25b851 Compare October 28, 2021 14:08

cathydev force-pushed the xfa-link branch from e25b851 to db0b3cd Compare October 29, 2021 15:40

calixteman approved these changes Oct 29, 2021

View reviewed changes

calixteman merged commit 2c0bbaf into mozilla:master Oct 29, 2021

mozilla deleted a comment from pdfjsbot Oct 29, 2021

cathydev deleted the xfa-link branch October 29, 2021 18:43

		@@ -0,0 +1 @@
		https://bugzilla.mozilla.org/attachment.cgi?id=9227382

Fix XFA links (bug 1735738) #14153

Fix XFA links (bug 1735738) #14153

Conversation

cathydev commented Oct 16, 2021 • edited Loading

Snuffleupagus commented Oct 16, 2021

cathydev commented Oct 16, 2021

Snuffleupagus commented Oct 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman commented Oct 19, 2021

cathydev commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman commented Oct 25, 2021

cathydev commented Oct 26, 2021

calixteman commented Oct 26, 2021 • edited Loading

cathydev commented Oct 26, 2021

calixteman commented Oct 26, 2021 • edited Loading

cathydev commented Oct 26, 2021

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 Oct 28, 2021

pdfjsbot commented Oct 28, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 28, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 28, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Oct 28, 2021

From: Bot.io (Windows)

Failed

calixteman commented Oct 29, 2021

calixteman commented Oct 29, 2021

pdfjsbot commented Oct 29, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 29, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 29, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Oct 29, 2021

From: Bot.io (Windows)

Failed

calixteman left a comment

Choose a reason for hiding this comment

pdfjsbot commented Oct 29, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 29, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 29, 2021

From: Bot.io (Linux m4)

Success

pdfjsbot commented Oct 29, 2021

From: Bot.io (Windows)

Success

cathydev commented Oct 16, 2021 •

edited

Loading

calixteman commented Oct 26, 2021 •

edited

Loading

calixteman commented Oct 26, 2021 •

edited

Loading