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

[api-minor] Set font size and color for text widget annotations #12828

Conversation

dhufnagel
Copy link
Contributor

@dhufnagel dhufnagel commented Jan 7, 2021

The display layer for annotation did not consider the font size in input fields.

I added the parsing and assigning of the font size of the input field. Aside from my own issues of PDFs not beeing correctly rendered on Screen, this fixes the too large rendered text fields in issue #7367 as well as the too small rendered text field in issue #11199 .

I did not change any rendering steps, as the font size seems to be considered there. One remaining issue are fields with font size "auto" which corresponds to size 0.

Fixes #7367.
Fixes #11199.

@dhufnagel
Copy link
Contributor Author

If #12831 gets merged, i would rewrite my changes to use the defaultAppearance parser and font-size calculations for auto-size provided in that PR

@dhufnagel dhufnagel marked this pull request as draft January 10, 2021 21:50
@dhufnagel dhufnagel force-pushed the feature/annotation_layer_display_fontsize branch from 8a11a30 to 013e935 Compare January 10, 2021 22:03
@dhufnagel dhufnagel force-pushed the feature/annotation_layer_display_fontsize branch from 013e935 to ee49880 Compare January 21, 2021 21:34
@dhufnagel
Copy link
Contributor Author

I've rewritten my old commit and now use the parsed default appearance data which was introduced by @calixteman in #12831 .
As this is my first PR, maybe someone can guide me how to go on from now? Just wait till someone is going to review this PR or is there anything to do now?

@dhufnagel dhufnagel marked this pull request as ready for review January 21, 2021 21:39
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

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

Looks good to me with these comments addressed, and I like how this uses the new parser.

src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/core/annotation.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
dhufnagel added a commit to dhufnagel/pdf.js that referenced this pull request Jan 22, 2021
@dhufnagel dhufnagel force-pushed the feature/annotation_layer_display_fontsize branch from ee49880 to aefd111 Compare January 22, 2021 11:49
test/test_manifest.json Show resolved Hide resolved
src/display/annotation_layer.js Outdated Show resolved Hide resolved
test/test_manifest.json Outdated Show resolved Hide resolved
@Snuffleupagus
Copy link
Collaborator

@dhufnagel dhufnagel force-pushed the feature/annotation_layer_display_fontsize branch 2 times, most recently from 7c5bf99 to 6765bf9 Compare January 22, 2021 15:55
@timvandermeij
Copy link
Contributor

Looks good to me once the merge conflict is resolved (happened because we just merged #12886 that also touched the same code, but simplified it). We'll run a preview and the tests after this is done.

use the default appearance to set the font size and color of a text
annotation widget
@dhufnagel dhufnagel force-pushed the feature/annotation_layer_display_fontsize branch from 6765bf9 to c5083cd Compare January 22, 2021 22:12
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/8c584538f995328/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/8c584538f995328/output.txt

Total script time: 4.23 mins

Published

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/948062b78ea4be7/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/6b557d91a88d37c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/948062b78ea4be7/output.txt

Total script time: 26.87 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/6b557d91a88d37c/output.txt

Total script time: 27.89 mins

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

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

@timvandermeij timvandermeij changed the title Feature/annotation layer display fontsize #11199 #7367 Set font size and color for input fields Jan 23, 2021
@timvandermeij timvandermeij changed the title Set font size and color for input fields Set font size and color for text widget annotations Jan 23, 2021
@timvandermeij timvandermeij changed the title Set font size and color for text widget annotations [api-minor] Set font size and color for text widget annotations Jan 23, 2021
@timvandermeij timvandermeij merged commit 25b84ce into mozilla:master Jan 23, 2021
@timvandermeij
Copy link
Contributor

Thank you for fixing this!

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/81a576ae607bb45/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/933a9f427d3c2c0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/933a9f427d3c2c0/output.txt

Total script time: 24.85 mins

  • Lint: Passed
  • Make references: FAILED

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/81a576ae607bb45/output.txt

Total script time: 25.94 mins

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

@timvandermeij
Copy link
Contributor

/botio-linux makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/20cf4a0032200ae/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/20cf4a0032200ae/output.txt

Total script time: 24.90 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.

Form Field doesn't have the correct font size Form field text values too large
4 participants