-
Notifications
You must be signed in to change notification settings - Fork 132
Fix checkbox ID generation to preserve brackets in field names #3878
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
Conversation
🦋 Changeset detectedLatest commit: 90914ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-authored-by: jonrohan <[email protected]>
Co-authored-by: jonrohan <[email protected]>
Co-authored-by: jonrohan <[email protected]>
…regex comment Co-authored-by: jonrohan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes checkbox ID generation to preserve brackets and other special characters in field names, addressing issue #3811. Previously, Rails' default sanitization converted brackets to underscores (e.g., permissions[3] became permissions_3), which broke proper HTML semantics for nested form structures.
Changes:
- Added custom ID generation logic in
CheckBox#initializethat preserves special characters - Distinguished between boolean scheme (ID = name) and array scheme (ID = name + "_" + value)
- Updated label
forattributes to match the custom IDs - Added comprehensive tests for both checkbox schemes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
app/lib/primer/forms/check_box.rb |
Implements custom ID generation logic that preserves brackets in names for both boolean and array checkbox schemes |
test/lib/primer/forms/checkbox_input_test.rb |
Adds tests verifying bracket preservation in generated IDs for both boolean and array schemes |
.changeset/brown-wasps-exercise.md |
Documents the change (but description is incorrect) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@primer/view-components": patch | ||
| --- | ||
|
|
||
| Fix generated field ids to remove brackets |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changeset description says "Fix generated field ids to remove brackets" but this is incorrect. The fix actually preserves brackets in generated field IDs, not removes them. The PR description and implementation both confirm that brackets are being preserved (e.g., permissions[3]_foo instead of permissions_3_foo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
| def test_preserves_brackets_in_generated_ids_with_boolean_scheme | ||
| render_in_view_context do | ||
| primer_form_with(url: "/foo") do |f| | ||
| render_inline_form(f) do |check_form| | ||
| check_form.check_box( | ||
| name: "user[settings][email]", | ||
| label: "Email notifications" | ||
| ) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # For boolean scheme, the ID should just be the name with brackets preserved | ||
| assert_selector "input[id='user[settings][email]']" | ||
| assert_selector "label[for='user[settings][email]']" | ||
| end |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a test case to verify that when an explicit ID is provided via the id parameter, it is not overridden by the custom ID generation logic. This would ensure the guard condition on line 15 works correctly and that users can still provide custom IDs when needed.
| "@primer/view-components": patch | ||
| --- | ||
|
|
||
| Fix generated field ids to remove brackets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
What are you trying to accomplish?
Checkbox field IDs were being sanitized by Rails' default behavior, converting brackets to underscores. A checkbox with
name="permissions[3]"andvalue="foo"generatedid="permissions_3_foo"instead ofid="permissions[3]_foo".This PR overrides ID generation in
CheckBoxcomponent to preserve special characters like brackets, maintaining proper HTML semantics for nested form structures.Integration
Risk Assessment
What approach did you choose and why?
Override ID generation in
CheckBox#initialize:long_o)permissions[3]_foo)The distinction matters because array scheme checkboxes share a name but need unique IDs, while boolean scheme checkboxes have unique names.
Rails appends
[]to array scheme names at render time, so we strip trailing[]during ID generation to avoid duplication.Anything you want to highlight for special attention from reviewers?
The label's
forattribute is updated to match the custom ID. Without an explicit ID ininput_arguments, the custom generator runs; with an explicit ID, it's skipped entirely.Accessibility
Merge checklist
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
accounts.google.com/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=5926 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-5898-1fmoyr --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,6134350618444463348,6714943023189237086,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)/opt/google/chrome/chrome /usr/bin/google-chrome --headless --disable-gpu --hide-scrollbars --mute-audio --enable-automation --disable-web-security --disable-session-crashed-bubble --disable-breakpad --disable-sync --no-first-run --use-mock-keychain --keep-alive-for-test --disable-popup-blocking --disable-extensions --disable-component-extensions-with-REDACTED-pages --disable-hang-monitor --disable-features=site-per-process,IsolateOrigins,TranslateUI --disable-translate --disable-REDACTED-networking(dns block)/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=6192 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-6164-bs65w6 --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,10032288820533622983,11681080222740272597,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)clients2.google.com/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=5926 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-5898-1fmoyr --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,6134350618444463348,6714943023189237086,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)/opt/google/chrome/chrome /usr/bin/google-chrome --headless --disable-gpu --hide-scrollbars --mute-audio --enable-automation --disable-web-security --disable-session-crashed-bubble --disable-breakpad --disable-sync --no-first-run --use-mock-keychain --keep-alive-for-test --disable-popup-blocking --disable-extensions --disable-component-extensions-with-REDACTED-pages --disable-hang-monitor --disable-features=site-per-process,IsolateOrigins,TranslateUI --disable-translate --disable-REDACTED-networking(dns block)/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=6192 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-6164-bs65w6 --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,10032288820533622983,11681080222740272597,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)content-autofill.googleapis.com/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=5926 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-5898-1fmoyr --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,6134350618444463348,6714943023189237086,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)/opt/google/chrome/chrome /usr/bin/google-chrome --headless --disable-gpu --hide-scrollbars --mute-audio --enable-automation --disable-web-security --disable-session-crashed-bubble --disable-breakpad --disable-sync --no-first-run --use-mock-keychain --keep-alive-for-test --disable-popup-blocking --disable-extensions --disable-component-extensions-with-REDACTED-pages --disable-hang-monitor --disable-features=site-per-process,IsolateOrigins,TranslateUI --disable-translate --disable-REDACTED-networking(dns block)/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=6192 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-6164-bs65w6 --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,10032288820533622983,11681080222740272597,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)optimizationguide-pa.googleapis.com/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=5926 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-5898-1fmoyr --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,6134350618444463348,6714943023189237086,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)/opt/google/chrome/chrome /usr/bin/google-chrome --headless --disable-gpu --hide-scrollbars --mute-audio --enable-automation --disable-web-security --disable-session-crashed-bubble --disable-breakpad --disable-sync --no-first-run --use-mock-keychain --keep-alive-for-test --disable-popup-blocking --disable-extensions --disable-component-extensions-with-REDACTED-pages --disable-hang-monitor --disable-features=site-per-process,IsolateOrigins,TranslateUI --disable-translate --disable-REDACTED-networking(dns block)safebrowsingohttpgateway.googleapis.com/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=5926 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-5898-1fmoyr --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,6134350618444463348,6714943023189237086,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)/opt/google/chrome/chrome /usr/bin/google-chrome --headless --disable-gpu --hide-scrollbars --mute-audio --enable-automation --disable-web-security --disable-session-crashed-bubble --disable-breakpad --disable-sync --no-first-run --use-mock-keychain --keep-alive-for-test --disable-popup-blocking --disable-extensions --disable-component-extensions-with-REDACTED-pages --disable-hang-monitor --disable-features=site-per-process,IsolateOrigins,TranslateUI --disable-translate --disable-REDACTED-networking(dns block)/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=6192 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-6164-bs65w6 --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,10032288820533622983,11681080222740272597,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)www.google.com/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=5926 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-5898-1fmoyr --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,6134350618444463348,6714943023189237086,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)/opt/google/chrome/chrome /usr/bin/google-chrome --headless --disable-gpu --hide-scrollbars --mute-audio --enable-automation --disable-web-security --disable-session-crashed-bubble --disable-breakpad --disable-sync --no-first-run --use-mock-keychain --keep-alive-for-test --disable-popup-blocking --disable-extensions --disable-component-extensions-with-REDACTED-pages --disable-hang-monitor --disable-features=site-per-process,IsolateOrigins,TranslateUI --disable-translate --disable-REDACTED-networking(dns block)/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --disable-dev-shm-usage --use-angle=swiftshader-webgl --mute-audio --crashpad-handler-pid=6192 --enable-crash-reporter=, --noerrdialogs --user-data-dir=/tmp/ferrum_user_data_dir_20260124-6164-bs65w6 --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,10032288820533622983,11681080222740272597,262144 --enable-features=NetworkService,NetworkServiceInProcess --disable-features=IsolateOrigins,PaintHolding,TranslateUI,site-per-process --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.