Add margin between radio buttons/checkboxes and their labels/strings #17948

Joewebsta · 2024-04-15T17:39:00Z

Bugzilla Bug 1716806

This PR adds margin between radio buttons/checkboxes and their labels/strings.

Links to images that illustrate the changes can be found below:

Example 1: Before | After
Example 2: Before | After
Example 3: Before | After
Example 4: Before | After

Links to the original PDFs can be found below:

I don't have extensive experience creating PRs, so I'd be happy to receive any feedback you might have.

timvandermeij · 2024-04-16T12:24:59Z

/botio-linux preview

timvandermeij · 2024-04-16T12:27:59Z

/botio test

web/viewer.css

timvandermeij · 2024-04-16T19:17:16Z

I'll trigger a new round of tests. If the change is visible in existing test cases we don't have to do more, but otherwise we'll need to add a (linked or in-repo PDF file) test case to avoid regressions. (We have guidelines on how to do that, but we'll get to that if it actually turns out to be necessary.)

/botio test

moz-tools-bot · 2024-04-16T19:20:05Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/7c48208cbdd4612/output.txt

moz-tools-bot · 2024-04-16T19:20:05Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/7ca844cc39c2e10/output.txt

moz-tools-bot · 2024-04-16T19:47:01Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/7ca844cc39c2e10/output.txt

Total script time: 26.92 mins

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

  different ref/snapshot: 354
  different first/second rendering: 2

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

moz-tools-bot · 2024-04-16T20:01:48Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/7c48208cbdd4612/output.txt

Total script time: 41.71 mins

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

  different ref/snapshot: 177

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

Joewebsta · 2024-04-16T20:48:19Z

I squashed the two commits into one. Thanks for your help and patience on this.

I will keep an eye out for the test results.

timvandermeij · 2024-04-17T11:51:24Z

Fortunately this change is clearly visible in the existing test cases, so we don't have to add new tests, but I do see some unexpected movement. For example in firefox-xfa_bug1718741-page1 (any quite a few others) where the input fields are moved/shortened. Is that indeed unexpected, or is that how e.g. Adobe Reader also renders the PDF (that's usually our reference point for verifying changes)?

I haven't really noticed other unexpected changes, but there are a lot of reference image changes and I haven't looked through all of them yet, so it'd be good if @calixteman also has a look here given the experience with the XFA code.

Joewebsta · 2024-04-17T20:06:21Z

@timvandermeij I reviewed the pdfs and all the observed movement was intended.

I agree that it would be great if @calixteman had the time to review the changes though.

calixteman · 2024-04-17T20:10:38Z

@timvandermeij I reviewed the pdfs and all the observed movement was intended.

I agree that it would be great if @calixteman had the time to review the changes though.

Sorry but I don't see why the movement could have been intended in firefox-xfa_issue14071-page1.
In this case (and in lot of others) we have neither a button nor a checkbox.

Joewebsta · 2024-04-18T18:04:25Z

@calixteman

The requirements outlined in Bugzilla Bug 1716806 specify the need to add spacing between radio buttons (not regular buttons), checkboxes, and text inputs and their associated captions.

This pull request (PR) addresses the bug by targeting the captions associated with any radio button, checkbox, or text input element that is a direct descendant of an element with either the .xfaRight or .xfaLeft class. Margins are then appropriately applied to the left or right of the caption div element.

Regarding PDF firefox-xfa_issue14071:

On Page 1:
- A "Date of Birth" caption associated with a text input element has a right margin applied.
On Page 2:
- Several radio button captions have left-margin applied.
- Several text input captions have right-margin applied.
  - Although the margin here may not be technically necessary, it is required for other forms, such as "Example 3: Before" (also linked in my initial comment above). Without altering the HTML structure, fixing Example 3 without affecting PDFs such as firefox-xfa_issue14071 is not feasible (at least, I couldn't think of a solution).
On Pages 4 and 5:
- Three instances of radio button captions have left-margin applied.

I did attempt to approach the implementation of my CSS styles differently, but couldn't come up with a superior solution.

If these changes are still deemed unacceptable, I will close the PR.

calixteman · 2024-04-18T18:42:22Z

Here's a screenshot from Acrobat:

You can see that the first red box for the year is almost fitting the area corresponding to the year.
The rendering in Firefox is almost the same but with your patch it's now different.
In this specific case there is no reason to change the width of the red box.

calixteman · 2024-04-18T18:54:29Z

Why do you want to close this PR ?
We just want to apply a margin with radio button/checkboxes as your PR title claims it, not with text fields.
I didn't check anything, but I'd say it's possible to write just few lines of CSS in using has selector.

Joewebsta · 2024-04-19T13:41:11Z

I gave the styles another go. The updated styles only affect the margin of checkboxes and radio buttons.

timvandermeij · 2024-04-23T14:00:06Z

web/xfa_layer_builder.css

@@ -157,6 +157,10 @@
  max-height: 100%;
 }

+.xfaRight > :is(.xfaRadio, .xfaCheckbox) {


Don't we need a rule for .xfaLeft as well? Moreover, in the Bugzilla ticket the margin directions are reversed, so .xfaRight sets a margin-left but here we set a margin-right; is that correct?

The styles in the bugzilla ticket target the .xfaCaption and .xfaCaptionForCheckButton classes which leads to the unexpected html element movement we observed last week (See calixteman's comment re: xfa_issue14071.pdf).

I took an alternative approach by targeting the .xfaRadio and .xfaCheckbox classes instead and as a result had to swap the margin left/right properties.

The xfaRight styles below work correctly and are included in the PR:

.xfaRight > :is(.xfaRadio, .xfaCheckbox) { margin-right: 4px; }

The xfaLeft styles below do not work correctly and were not included in the PR (I apologize, I should have explained my rationale for this). These styles once again lead to unexpected HTML element movement.

.xfaLeft > :is(.xfaRadio, .xfaCheckbox) { margin-left: 4px; }

See the attached examples of incorrect element movement. The green background was add by me to highlight the element shift.

Any suggestions on how to proceed?

@Joewebsta Can you share a link to the pdf you used ?

The images above were taken from the following test pdfs:

xfa_issue13994

xfa_issue14150

xfa_issue13213

Some additional examples are:

xfa_bug1716816.pdf

xfa_bug1716980.pdf

xfa_bug1718735.pdf

xfa_dhl_shipment.pdf

xfa_hsbc_closure.pdf

xfa_issue13597.pdf

timvandermeij · 2024-04-24T12:29:04Z

/botio test

moz-tools-bot · 2024-04-24T12:29:07Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/019065b41c17c79/output.txt

moz-tools-bot · 2024-04-24T12:29:07Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/47deb7aa1bcec96/output.txt

calixteman · 2024-04-24T12:35:56Z

As @timvandermeij pointed it, the xfaLeft and xfaRight are symmetric, so making a change in one property should imply a change in the other: it'd be very strange to have an asymmetry here.
Instead of moving the caption, I think the right way to do (which needs to be verified with the tests) is add some left/right margin to the input under xfaLeft /xfaRight.

moz-tools-bot · 2024-04-24T12:56:06Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/019065b41c17c79/output.txt

Total script time: 26.97 mins

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

  different ref/snapshot: 275
  different first/second rendering: 4

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

moz-tools-bot · 2024-04-24T13:11:53Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/47deb7aa1bcec96/output.txt

Total script time: 42.75 mins

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

  different ref/snapshot: 132

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

Joewebsta · 2024-04-24T16:59:57Z

I've previously tried applying margins to the checkbox and radio button inputs (more specifically the .xfaRadio and .xfaCheckbox classes, but these modifications did not yield the desired outcome.

Despite a couple of attempts, I've been unable to resolve this issue satisfactorily. @calixteman and @timvandermeij, thank you for help and patience, but I believe this bug exceeds my current ability to address.

calixteman · 2024-04-24T17:09:41Z

Did you try something like (and its equivalent for xfaLeft):

.xfaRight > input {
 margin-right: 4px;
}

?

calixteman · 2024-04-24T17:10:00Z

That said, don't give up :)

Joewebsta · 2024-04-25T19:17:26Z

@calixteman Thanks for your suggestion.

I applied the styles below locally, but still observed the element shifting unfortunately.

.xfaRight > input {
  margin-right: 4px;
}

.xfaLeft > input {
  margin-left: 4px;
}

timvandermeij added the viewer label Apr 16, 2024

Snuffleupagus added form-xfa and removed viewer labels Apr 16, 2024

Snuffleupagus requested changes Apr 16, 2024

View reviewed changes

web/viewer.css Outdated Show resolved Hide resolved

Joewebsta requested a review from Snuffleupagus April 16, 2024 17:23

Snuffleupagus removed their request for review April 16, 2024 18:04

mozilla deleted a comment from moz-tools-bot Apr 16, 2024

Joewebsta force-pushed the xfa-radio-checkbox-spacing branch from cdf6d22 to eca2352 Compare April 16, 2024 20:45

Joewebsta closed this Apr 18, 2024

Add margin between radio buttons/checkboxes and their labels/strings

92c61f3

Joewebsta reopened this Apr 19, 2024

Joewebsta force-pushed the xfa-radio-checkbox-spacing branch from eca2352 to 92c61f3 Compare April 19, 2024 13:39

timvandermeij reviewed Apr 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add margin between radio buttons/checkboxes and their labels/strings #17948

Add margin between radio buttons/checkboxes and their labels/strings #17948

Joewebsta commented Apr 15, 2024

timvandermeij commented Apr 16, 2024

timvandermeij commented Apr 16, 2024

timvandermeij commented Apr 16, 2024 •

edited

moz-tools-bot commented Apr 16, 2024

moz-tools-bot commented Apr 16, 2024

moz-tools-bot commented Apr 16, 2024

moz-tools-bot commented Apr 16, 2024

Joewebsta commented Apr 16, 2024

timvandermeij commented Apr 17, 2024 •

edited

Joewebsta commented Apr 17, 2024

calixteman commented Apr 17, 2024

Joewebsta commented Apr 18, 2024 •

edited

calixteman commented Apr 18, 2024

calixteman commented Apr 18, 2024

Joewebsta commented Apr 19, 2024

timvandermeij Apr 23, 2024

Joewebsta Apr 23, 2024 •

edited

calixteman Apr 23, 2024

Joewebsta Apr 23, 2024

timvandermeij commented Apr 24, 2024

moz-tools-bot commented Apr 24, 2024

moz-tools-bot commented Apr 24, 2024

calixteman commented Apr 24, 2024 •

edited

moz-tools-bot commented Apr 24, 2024

moz-tools-bot commented Apr 24, 2024

Joewebsta commented Apr 24, 2024

calixteman commented Apr 24, 2024

calixteman commented Apr 24, 2024

Joewebsta commented Apr 25, 2024

Add margin between radio buttons/checkboxes and their labels/strings #17948

Are you sure you want to change the base?

Add margin between radio buttons/checkboxes and their labels/strings #17948

Conversation

Joewebsta commented Apr 15, 2024

timvandermeij commented Apr 16, 2024

timvandermeij commented Apr 16, 2024

timvandermeij commented Apr 16, 2024 • edited

moz-tools-bot commented Apr 16, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Apr 16, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Apr 16, 2024

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Apr 16, 2024

From: Bot.io (Windows)

Failed

Joewebsta commented Apr 16, 2024

timvandermeij commented Apr 17, 2024 • edited

Joewebsta commented Apr 17, 2024

calixteman commented Apr 17, 2024

Joewebsta commented Apr 18, 2024 • edited

calixteman commented Apr 18, 2024

calixteman commented Apr 18, 2024

Joewebsta commented Apr 19, 2024

timvandermeij Apr 23, 2024

Choose a reason for hiding this comment

Joewebsta Apr 23, 2024 • edited

Choose a reason for hiding this comment

calixteman Apr 23, 2024

Choose a reason for hiding this comment

Joewebsta Apr 23, 2024

Choose a reason for hiding this comment

timvandermeij commented Apr 24, 2024

moz-tools-bot commented Apr 24, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Apr 24, 2024

From: Bot.io (Windows)

Received

calixteman commented Apr 24, 2024 • edited

moz-tools-bot commented Apr 24, 2024

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Apr 24, 2024

From: Bot.io (Windows)

Failed

Joewebsta commented Apr 24, 2024

calixteman commented Apr 24, 2024

calixteman commented Apr 24, 2024

Joewebsta commented Apr 25, 2024

timvandermeij commented Apr 16, 2024 •

edited

timvandermeij commented Apr 17, 2024 •

edited

Joewebsta commented Apr 18, 2024 •

edited

Joewebsta Apr 23, 2024 •

edited

calixteman commented Apr 24, 2024 •

edited