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.

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

Annotation & XFA: Scale the font size in choicelist using zoom factor (bug 1715996) #13868

Merged
merged 1 commit into from
Aug 5, 2021

Conversation

calixteman
Copy link
Contributor

  • this is an accessibility issue which could be painful for some people with visual disabilities.

src/core/xfa/template.js Show resolved Hide resolved
@@ -694,6 +694,8 @@ class BaseViewer {
}

_setScaleUpdatePages(newScale, newValue, noScroll = false, preset = false) {
document.documentElement.style.setProperty("--zoom-factor", newScale);
Copy link
Collaborator

Choose a reason for hiding this comment

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

First of all, please move this down to just after the if (isSameScale(...)) { ... } block below, since there's no good reason to update this unconditionally as far as I can tell.

Secondly, let's add this._doc = document.documentElement; after e.g.

and utilize that instead there.


Also, this won't work when PDFPageView is being used standalone. Ideally we should probably move this code into PDFPageView.update instead, but I'm not sure if repeatedly setting the CSS variable (to the same value) is an issue performance-wise?
However, I'd be OK with deferring this point for now in the interest of getting it fixed in a timely manner.

web/viewer.css Outdated
@@ -16,6 +16,8 @@
@import url(pdf_viewer.css);

:root {
--zoom-factor: 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be placed in pdf_viewer.css instead, since otherwise you're breaking the standalone viewer components.

@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/afddeed44d80c81/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/e126d74dc1b3bb4/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/e126d74dc1b3bb4/output.txt

Total script time: 33.20 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  errors: 612
  different ref/snapshot: 9
  different first/second rendering: 2

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

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/afddeed44d80c81/output.txt

Total script time: 34.85 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 8
  different first/second rendering: 2

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

@Snuffleupagus
Copy link
Collaborator

There seem to be one regression in chrome-annotation-choice-widget-forms-page1.

@calixteman
Copy link
Contributor Author

Yep, I forgot to set the font size only for combo choicelist.

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/9769fd7e81c6223/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/a011d5bca46d56a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/9769fd7e81c6223/output.txt

Total script time: 34.77 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 13
  different first/second rendering: 1

Image differences available at: http://54.67.70.0:8877/9769fd7e81c6223/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Aug 4, 2021

Yep, I forgot to set the font size only for combo choicelist.

It looks like you forgot to push the changes?

Also, a couple of the integrationtests fail on all platforms/browsers, so those tests may need some updates as well?

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/a011d5bca46d56a/output.txt

Total script time: 39.96 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 8
  different first/second rendering: 1

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

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/0bcf82be5692316/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/e1f5eb540e1a61a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/0bcf82be5692316/output.txt

Total script time: 26.13 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  errors: 639
  different ref/snapshot: 7
  different first/second rendering: 2

Image differences available at: http://54.67.70.0:8877/0bcf82be5692316/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio-linux test

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/2161bac77c3f2f8/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/e1f5eb540e1a61a/output.txt

Total script time: 38.91 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 7
  different first/second rendering: 1

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

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/2161bac77c3f2f8/output.txt

Total script time: 33.46 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 10
  different first/second rendering: 2

Image differences available at: http://54.67.70.0:8877/2161bac77c3f2f8/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/94f1f691698295b/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/94f1f691698295b/output.txt

Total script time: 5.14 mins

Published

@Snuffleupagus
Copy link
Collaborator

Testing the preview, with the annotation-choice-widget.pdf file from the test-suite, there's now a large of amount of "padding" in the dropdowns (i.e. similar to the previously failing ref-tests above); please see the following screen-shot:

image

… (bug 1715996)

  - this is an accessibility issue which could be painful for some people with visual disabilities.
@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/e17dd845fc3c2c1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/14537e4295eec8b/output.txt

@calixteman
Copy link
Contributor Author

Testing the preview, with the annotation-choice-widget.pdf file from the test-suite, there's now a large of amount of "padding" in the dropdowns (i.e. similar to the previously failing ref-tests above); please see the following screen-shot:

I thought adding font-size stuff on an optgroup will help to reduce the number of setProperty but I misunderstood what optgroup is and it's why some extra blanks were added at the beginning of the line.
So finally, I added font-size stuff on each option, if you've a good idea to avoid that, please propose.

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/e17dd845fc3c2c1/output.txt

Total script time: 26.23 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  errors: 639
  different ref/snapshot: 11
  different first/second rendering: 1

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

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/14537e4295eec8b/output.txt

Total script time: 32.46 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  errors: 612
  different ref/snapshot: 12
  different first/second rendering: 1

Image differences available at: http://3.101.106.178:8877/14537e4295eec8b/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/b4d0638614dda26/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/0a89046e18bda03/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/0a89046e18bda03/output.txt

Total script time: 33.49 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 16
  different first/second rendering: 1

Image differences available at: http://54.67.70.0:8877/0a89046e18bda03/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/b4d0638614dda26/output.txt

Total script time: 39.11 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 11
  different first/second rendering: 1

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

@calixteman
Copy link
Contributor Author

The differences are due to the fact we use the fontsize from the pdf.

@calixteman
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/00bcf1864098081/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/00bcf1864098081/output.txt

Total script time: 5.13 mins

Published

@Snuffleupagus
Copy link
Collaborator

So finally, I added font-size stuff on each option, if you've a good idea to avoid that, please propose.

Given that documents likely won't contain more than perhaps a handful of these form elements per page, and that we unload pages that aren't visible, I'm not sure if this really is a going to be a big problem.
Unfortunately I've not got a better idea off the top of my head, so let's do this for now (and I'll try to fix the standalone PDFPageView case in a follow-up).

@Snuffleupagus Snuffleupagus merged commit 4ad65c8 into mozilla:master Aug 5, 2021
@Snuffleupagus
Copy link
Collaborator

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Aug 5, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/ec51d41b2f106da/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 5, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/3b5b2ff0aff2b0f/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 5, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/ec51d41b2f106da/output.txt

Total script time: 30.38 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

pdfjsbot commented Aug 5, 2021

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/3b5b2ff0aff2b0f/output.txt

Total script time: 36.84 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants