Repository navigation
fix(editor): enforce locked attributes, honour i18n.locale, warn on duplicate field names - #509
Merged
Merged
Conversation
…ed attributes The add-attribute dialog rejected disabled names but let a locked one be re-added, overwriting its value, and readonly doesn't stop Space on a checkbox or the arrow keys on a select. Reject locked names in the dialog and in EditPanel#addAttribute, and disable locked checkbox, radio and select controls (text inputs stay readonly). Refs #116 Refs #159
loadResources passed the stored locale unconditionally, so on a first visit it passed null and i18n fell back to en-US. Only override the configured locale when setLang() has stored one.
Only rewrite a hint when its text changes, so its status region is not announced again while another field is edited. Watch every field in the scan, so a name set in code on a field without a name row still refreshes the others. Describe a picklist name control too, and drop an unused return from the i18n locale spec. Refs #331
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved correctness, accessibility-refresh, control-locking, and documentation issues remain.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR fixes editor attribute locking, initial locale precedence, and duplicate field-name warnings.
Changes:
- Enforces locked attributes and control states.
- Honors configured i18n locale on first load.
- Adds duplicate-name detection, accessible hints, tests, and documentation updates.
| File | Reviewed changes |
|---|---|
tests/editor-i18n-locale.spec.js |
Tests locale precedence. |
tests/duplicate-field-names.spec.js |
Tests duplicate-name warnings. |
tests/control-attr-rules.spec.js |
Tests locked control behavior. |
src/lib/js/editor.js |
Applies the configured locale correctly. |
src/lib/js/components/fields/duplicate-names.test.mjs |
Tests duplicate-name utilities. |
src/lib/js/components/fields/duplicate-names.mjs |
Detects duplicate effective field names; normalization needs correction. |
src/lib/js/components/edit-panel/locked-attrs.test.js |
Tests locked attribute guards and rendering. |
src/lib/js/components/edit-panel/edit-panel.js |
Guards attribute additions; canonicalize names before lock checks. |
src/lib/js/components/edit-panel/edit-panel-item.mjs |
Applies locked control states; preserve disabled-attribute enforcement. |
src/lib/js/components/edit-panel/duplicate-name-hint.test.js |
Tests hint lifecycle and accessibility. |
src/lib/js/components/edit-panel/duplicate-name-hint.js |
Renders duplicate-name hints; IDs and locale refresh need updates. |
src/lib/js/common/actions.test.js |
Tests locked attribute validation. |
src/lib/js/common/actions.js |
Rejects locked attribute names. |
docs/options/i18n/README.md |
Documents locale behavior. |
docs/options/controls/README.md |
Documents locked controls; include radios. |
docs/options/config/README.md |
Updates locking guidance; include radios. |
docs/controls/custom-attribute-types.md |
Updates attribute rules; include radios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+298
to
+304
| // readonly does nothing on checkboxes, radios and selects, so a locked one is disabled instead | ||
| if (this.isLocked) { | ||
| const isTextControl = | ||
| baseConfig.tag === 'textarea' || | ||
| (baseConfig.tag === 'input' && !['checkbox', 'radio'].includes(baseConfig.attrs?.type)) | ||
| attrs[isTextControl ? 'readonly' : 'disabled'] = true | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Leaving this as is. attrs.disabled = this.isDisabled was always false at this point: itemInput returns null a few lines up when this.isDisabled is true, and EditPanel#createProps skips disabled props before an EditPanelItem is built. hideDisabled: false doesn't render an editable input for a disabled attribute on either the old or the new code.
Comment on lines
+3
to
+5
| String(name ?? '') | ||
| .trim() | ||
| .replace(/\[\]$/, '') |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 36d84c7. fieldNameKey no longer trims. It only drops the trailing [], the same way userData does, so email and email are now separate keys and two whitespace-only names are flagged.
…s them fieldNameKey trimmed names, so 'email' and ' email ' were flagged as sharing a key though they post under different ones, and two whitespace-only names were ignored though they collide. Refs #331
Collaborator
Author
|
🎉 This PR is included in version 5.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three editor fixes.
Locked attributes stay locked (Refs #116, #159)
EditPanel#addAttributeguards against both, so customactions.add.attrhandlers can't bypass it.readonlydoesn't stop Space on a checkbox or the arrow keys on a select. Locked checkbox, radio and select controls are nowdisabled; locked text inputs stayreadonly.The configured
i18n.localeis used on first loadloadResourcespassed the sessionStorage locale unconditionally. On a first visit that wasnull, which reset the configured locale to en-US.editor.i18n.setLang()still wins for the tab. Otherwise the config'slocaleapplies.Duplicate field names get a warning (Refs #331)
attrs.name, so two fields can end up submitting under one key, most often after a clone.nameattribute (role="status", linked witharia-describedby).duplicateFieldName, with an English fallback until@draggable/formeo-languagesships it (branchfeat/editor-followup-keysin that repo).Testing
npm test: 595 passnpm run lint: cleantests/editor-i18n-locale.spec.jsandtests/duplicate-field-names.spec.js, and new cases intests/control-attr-rules.spec.js