dev/core#5571 dev/core#5566 Fix regressions on radio options rendering in profiles

[eileenmcnaughton]

Overview

Alternative to #31705

@totten @artfulrobot - this is my preferred approach

Before

This addresses 2 regressions from This change b163ff8#diff-6848a25bfee634154a321db3c9c9125345b96609e62dcf7155ecbdd4d4fe4f64L35 that affect Radio Custom Fields in profiles when the 'options_per_line' is set

That PR improved the mark-up but by trying to alter it post render & missed some edge cases - this moves the markup to pre-render

1. when the field is not required : the 'clear' button is missing for non admin users (and always missing on the uf screen - e.g https://dmaster.localhost:32353/civicrm/profile/create?gid=1&reset=1 - but that is longer standing )

2. when the field is required : the options html renders twice if when rendering the 'required field missing' if not entered

After

Both of the above are fixed when options_per_line is set - as of writing I have not tested when they are not set but expect the issues may persist as they are in a separate copy & paste of the code

Technical Details

I think this is the right approach - but I have only looked at the ones with options_per_line set - I think it would be extended for the others. I suspect the classes should be renamed to 'crm_radio' and 'crm_group' rather than 'radio_with_div' and 'group_with_div' since I think I land on these being classes to add civi-specific divs rather than try to make them flexible enough to force in the relevant divs / html from the outside

Comments

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[vingle]

I suspect the classes should be renamed to 'crm_radio' and 'crm_group' rather than 'radio_with_div' and 'group_with_div'

• 1. Tiny request for it to be crm-radio and crm-option-group (or somesuch) as dashes used more widely than underscores in Civi CSS classnames.

[eileenmcnaughton]

@vingle the classes I was referring to are php classes - no change to the css classes in this PR

[vingle]

@eileenmcnaughton 🤦‍♂️ sorry, that's me not reading the PR properly before piping in! 🤦‍♂️

[totten]

@eileenmcnaughton OK, here's my full test procedure with sample configuration files and results:

https://gist.github.com/totten/0aa0016bec3b87d23f83d6f32c010208

(All results observed as admin user.)

Highlights:

• "Edit via Contact Profile" ==> Mixed
  * Yay: "Optional Contact Radio (OPL2)" gains the clear (X) button
  * Meep: Anything like "Checkbox (OPL2)" no longer wraps
• "Register for Event" ==> Mixed
  * Yay: "Optional Contact Radio (OPL2)" gains the clear (X) button
  * Yay: "Submit Blank: Semi-Required Contact Checkbox (OPL2)" fixes the double-vision
  * Meep: Anything like "Checkbox (OPL2)" no longer wraps
• "View Contact" ==> No change (still buggy)
• "Edit Contribution" ==> No change (still buggy)

I'm guessing the last two screens are rendered through different call-paths and are expected to be unchanged ? Thoughts on wrapping for checkboxes?

[eileenmcnaughton]

@totten on the wrapping I guess the css must have changed in some way - although I thought it didn't in my testing - but I did use a different screen to test.

I agree that the view & edit contact screens are not profile related (I didn't replicate issues with them but that is a different question)

[totten]

(1) OK, when I started reading the code, it felt like it was saying this: The TPL's are too big/messy, which makes them more error-prone, harder to debug, harder to share/reuse. It's easier to understand+fix if we (REF) extract some discrete components to render the radio-groups/checkbox-groups.

That's quite appealing. 👍 There are too many pages that need to be tested because they output the same widget differently. Some (REF) feels quite warranted.

(Tho maybe, as in the opening comment, the names for RadioWithDiv/GroupWithDiv should be tweaked. It's kind of opaque...)

(2) When I start inspecting the generated HTML, here's what I see:

https://gist.github.com/totten/f8944ecc95005547af7ef0f66adbb17a (all examples have options_per_line=2)

The structure of the markup changes:

• For the radio-group, the PR changes from <TABLE> layout to <DIV>s with helper classes (crm-multiple-checkbox-radio-options and crm-option-label-pair).
• For the checkbox-group, the PR changes from <TABLE> layout to bare <INPUT>s and <LABEL>s (no classes to indicate wrapping/layout behavior).

So the HTML-inspection agrees with the visual-testing: the markup for checkboxes has lost support for wrapping.

(3) I feel like I'm generally missing some sense of "true north" around how these are supposed to work - aka missing docs/comments/examples for RadioWithDiv, GroupWithDiv, crm-multiple-checkbox-radio-options, crm-option-label-pair, etc. (I've checked docblocks for the new classes; PR descriptions for this and the earlier #30162; the pattern-library in themetest; and the discussion thread https://lab.civicrm.org/dev/core/-/issues/4985. That last looks rigorous and brilliant about how to make layouts work... but I can't find a summary of the resulting API.)

(4) FWIW, if I use a debugger, I see that a logical radio-group does lead to running RadioWithDiv and GroupWithDiv. But a logical checkbox-group doesn't. It feels like handling of the checkbox-vs-radio groups are partially (but not fully) entangled.

[totten]

We talked a bit, and basically:

1. This is better than status-quo:
   • If the "Clear" (X) button is missing, then that's a hard functional block for a user. And if the validation-message has double-vision, then that's major confusion.
   • If the checkboxes don't quite wrap at the defined options_per_line, then that's a smaller aesthetic issue.
2. We have other drafts/work to do it better, but we'll come back to this soon for another patch-level revision.
3. We're not committing ourselves to specific supporting these new Quickform symbols. That's TBD.
4. For purposes of today (5.81.0), it's better "in than out".

[totten]

🤦