Skip to content

Add margin to separate radio buttons and checkboxes from their labels #19765

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gpanakkal
Copy link
Contributor

This PR addresses bug 1716806 by adding a 4px margin to the right of checkboxes and radio buttons for .xfaRight elements, or to the left for .xfaLeft elements.

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
@Snuffleupagus
Copy link
Collaborator

Have you actually run npx gulp xfatest successfully with this patch?

Note that this looks effectively like a duplicate of PR #17948, which caused lots of regressions and thus couldn't be merged.

@gpanakkal
Copy link
Contributor Author

When I ran npx gulp xfatest, 228 of the ref test failures were pixel differences, all on Chrome, and all also occurring without the patch (I later saw in #16934 that the bot doesn't run Chrome tests anymore for this reason).

When I ran botxfatest:

  • A couple of integration tests failed on Windows 10. The failing cases were not the same each time, and some fails happened without the patch. No integration failures on Linux, though.
  • 130 ref tests failed. 123 of these fails were solely because the reference image also exhibited this bug. The other 7 involved text getting cut off. I'm working on another approach to fixing this bug in JS instead of touching CSS files, which should hopefully resolve the latter problem. How should I handle the remaining 123 fails?

@calixteman
Copy link
Contributor

/botio xfatest

@moz-tools-bot
Copy link
Collaborator

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/7361414c81b3c63/output.txt

Total script time: 17.97 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 107

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

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

Total script time: 39.38 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 107

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

@gpanakkal
Copy link
Contributor Author

@Snuffleupagus I've been poking around in src/core/xfa/, especially in template.js and layout.js. What's the proper way to modify a component's margin/padding? I've tried several approaches, but none of them worked as expected. For example, initializing a new Margin in the CheckButton constructor always resulted in a margin of 0 regardless of what I set the insets to.

@gpanakkal
Copy link
Contributor Author

@Snuffleupagus I've been poking around in src/core/xfa/, especially in template.js and layout.js. What's the proper way to modify a component's margin/padding? I've tried several approaches, but none of them worked as expected. For example, initializing a new Margin in the CheckButton constructor always resulted in a margin of 0 regardless of what I set the insets to.

@calixteman bumping this - would appreciate some guidance here!

@calixteman
Copy link
Contributor

@gpanakkal did you analyze why, for example the rendering in xfa_bug1739502.pdf, is now wrong ?
You can inspect the pdf content and in particular the XFA stream thanks to https://brendandahl.github.io/pdf.js.utils/xfa/.
Normally the elements should have the dimensions we've in the xml so it's why I'm not sure why we should have 2.5px to give for a margin. Maybe something is wrong when computing the dimensions of the different elements or we could use a flex layout to give some extra space to the margin.
Anyway, it's a low priority bug for us and the fix seems to require more work (at least in term of investigation) than we can provide.
As a side note, const style = toStyle(this, "margin"); fixes a real issue so maybe you could start by making a patch to fix just this one (and others if you find similar ones).

@gpanakkal
Copy link
Contributor Author

gpanakkal commented Apr 29, 2025

@gpanakkal did you analyze why, for example the rendering in xfa_bug1739502.pdf, is now wrong ?

Yes, I analyzed all the reftest failures by hand. Adding space should occur during layout calculations so that adequate room is allocated for the label, but the original approach of adding margin via CSS meant that new margin was added after layout was calculated, so text was cut off in 7 failing pages (xfa_bug1739502-page4, xfa_bug1718741-page5, xfa_bug1717668_4-page3, xfa_bug1717668_4-page4, xfa_issue13597-page1, xfa_issue13633-page1, and xfa_issue13633-page2).

It appears that Acrobat adds up to 2.5px between check buttons and their labels, or less based on how much space is available for the label.

You can inspect the pdf content and in particular the XFA stream thanks to https://brendandahl.github.io/pdf.js.utils/xfa/.

Thanks, this is quite helpful! Looks like Acrobat prunes whitespace around label text; see the second-to-last "Yes" label on page 5 of xfa_bug1718741.pdf, and the "GMDN" label on page 4 of xfa_bug1739502.pdf - both have leading/trailing whitespace in the XFA and when opened in PDF.js, but not in Acrobat.

Normally the elements should have the dimensions we've in the xml so it's why I'm not sure why we should have 2.5px to give for a margin. Maybe something is wrong when computing the dimensions of the different elements or we could use a flex layout to give some extra space to the margin.
Anyway, it's a low priority bug for us and the fix seems to require more work (at least in term of investigation) than we can provide.

I just wanted some general guidance to make sure I wasn't heading in the wrong direction altogether, since I'm not familiar with the codebase. 2.5px margin is just a placeholder solution in the meantime.

From my current understanding, a proper fix requires both:

  1. Caption.[$toHTML] to get the width/height of its value, subtract that from availableSpace to get the upper bound for the margin, and then set the margin on the appropriate side to the lesser of the upper bound or 2.5 pt. Alternatively, adjust flex layout like you suggested.
  2. Leading/trailing whitespace to be trimmed when parsing XFA text elements, at least when inside captions.

I'm on the fence about whether or not I should continue working on this bug. Let me know if these changes sound correct and reasonably easy to implement.

As a side note, const style = toStyle(this, "margin"); fixes a real issue so maybe you could start by making a patch to fix just this one (and others if you find similar ones).

Will do!

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.

None yet

5 participants