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

Add an empty entry in combo list when nothing is selected (bug 1773680) #15020

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

calixteman
Copy link
Contributor

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.

r=me, with one more suggestion and passing tests; thank you!

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/8d14a5e52b21352/output.txt

Total script time: 26.02 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Integration Tests: Passed
  • Regression tests: FAILED
  errors: 2
  different ref/snapshot: 42

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

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

Total script time: 28.20 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  errors: 1
  different ref/snapshot: 5

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

Comment on lines 1 to 2
https://bugzilla.mozilla.org/attachment.cgi?id=9280675

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the test results and logs, this document is a duplicate of issue12233.pdf and shouldn't be added.

Hence the manifest-entry below should reference the existing file instead, assuming that we even need to add a new test here (since the existing test-cases may already cover this).

@calixteman
Copy link
Contributor Author

The "regression" in 160F-2019.pdf doesn't make sense. The patch just changed some code in ChoiceWidget and the hello world is in a TextWidget.
The one in annotation-choice-widget-forms.pdf is a bug fix: I opened it in Acrobat and the combo is empty and I rg for the md5 in the wrong directory so my test is useless.

@calixteman
Copy link
Contributor Author

@Snuffleupagus, the regression in 160F-2019 is very likely unrelated to this patch, or maybe I overlooked something, are you still ok to merge it ?

@Snuffleupagus
Copy link
Collaborator

The "regression" in 160F-2019.pdf doesn't make sense. The patch just changed some code in ChoiceWidget and the hello world is in a TextWidget.

Agreed, I also don't understand how that could've happened.

the regression in 160F-2019 is very likely unrelated to this patch, or maybe I overlooked something, are you still ok to merge it ?

Assuming it's even a regression, since comparing with Adobe Reader I'm not sure...


While I'd suggest that it makes sense to try and understand what changed and importantly why here, I suppose that this PR can still land in the meantime :-)

@calixteman
Copy link
Contributor Author

@calixteman calixteman merged commit 6e6d94a into mozilla:master Jun 10, 2022
@calixteman
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@calixteman
Copy link
Contributor Author

The border is white so the reftest is now correct. Probably at some point the stroke color was black and because of wrong g we weren't using the white. So thanks to the patch for the ink stuff, it's now fixed.
I'm sorry about that.

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/6ea1e23602fe397/output.txt

Total script time: 23.39 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

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

Total script time: 21.63 mins

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

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jun 10, 2022

The border is white so the reftest is now correct. Probably at some point the stroke color was black and because of wrong g we weren't using the white. So thanks to the patch for the ink stuff, it's now fixed.

Thanks for figuring this out, since it confirms that the current state is indeed correct now!
(We both missed running the full test-suite in PR #15006 apparently...)

I'm sorry about that.

Don't apologize for fixing an old bug :-)

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.

3 participants