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

XFA - Set storage values to select and option elements #16714

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

TaTo30
Copy link
Contributor

@TaTo30 TaTo30 commented Jul 20, 2023

With XFA Forms, select fields always render with default attributes values and doesn't change if storage has different values, this PR intent to fix that by updating element attributes with storage values if exists and removing the 'selected' attribute (if exists) if the option actually isn't selected.

@Snuffleupagus
Copy link
Collaborator

I've tagged Calixte for review, however a couple of quick observations:

  • The commit message feels a little short and non-descriptive, please keep in mind that all information/context should be included in the commit message as well (and not only in the PR description).
  • Please note that all PRs should include test(s) to ensure that things first of all work as intended and secondly won't accidentally break in the future.

@calixteman
Copy link
Contributor

Do you have an example of pdf which demonstrates the issue you're trying to fix ?

@TaTo30
Copy link
Contributor Author

TaTo30 commented Jul 20, 2023

@Snuffleupagus Thanks for the suggestions.

@calixteman This is the pdf i'm using: xfa_invoice_example.pdf

@TaTo30
Copy link
Contributor Author

TaTo30 commented Jul 20, 2023

I found this issue when i wanted implement the XFA forms in a web application, so i used the npm package pdfjs-dist and tried to play with transformation features like scaling and rotating, i noticed that all fields keeps the stored value but select didn't do it.

This is the current behavior:
current

This is the expected behavior (with the changes i made):
expected

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@calixteman
Copy link
Contributor

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 25.25 mins

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

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

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Please see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits and please also improve the commit message as was mentioned previously.

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

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

Total script time: 39.15 mins

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

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

@TaTo30
Copy link
Contributor Author

TaTo30 commented Jul 21, 2023

@Snuffleupagus Done!

@Snuffleupagus
Copy link
Collaborator

Sorry, but I think that the commit message should mention XFA (as the PR title does) since that's not really clear unless you actually look at the code. (You'd probably guess that it's related to the regular annotationLayer otherwise.)

Please keep in mind that it's very helpful with good/clear commit messages when using the git command-line, such as e.g. during bisecting and other operations.

…a and

removes the 'selected' attribute from option element if it's not actually selected.
@TaTo30
Copy link
Contributor Author

TaTo30 commented Jul 21, 2023

@Snuffleupagus Fixed.

Please keep in mind that it's very helpful with good/clear commit messages when using the git command-line, such as e.g. during bisecting and other operations.

I got it, sorry if i'm kind of newbie using git, all of this are new experiences for me 😅

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Thanks!

@Snuffleupagus Snuffleupagus merged commit abb24f8 into mozilla:master Jul 21, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants