fix(core): correct read only property#90
Open
coryrylan wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR renames the host property from ChangesReadonly Property Rename Across Components
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/core/src/internal/controllers/state-selected.controller.test.ts (1)
105-118: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a transition test for anchor-selected → readOnly=true.
Please add a case that first sets selected anchor state, waits stable, then toggles
readOnly = trueand assertsaria-currentis removed. This guards the runtime transition path.Also applies to: 120-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/internal/controllers/state-selected.controller.test.ts` around lines 105 - 118, Add a new test that exercises the runtime transition from an anchor-selected state into readOnly: create an anchor element `a`, append it to `element`, set `element.selected = true`, add the internal anchor state via `element._internals.states.add('anchor')`, call `element.requestUpdate()` and `await elementIsStable(element)` to stabilize, then toggle `element.readOnly = true`, await stability again, and assert that `a.getAttribute('aria-current')` is null (and that `element._internals.ariaSelected` is null and `element.matches(':state(selected)')` is false) to verify `aria-current` is removed; add the same transition-style test for the other affected block noted around the 120-131 region.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/internal/controllers/state-selected.controller.ts`:
- Around line 30-33: The read-only branch exits without clearing any
anchor-specific aria state, so when this.host.readOnly is true you must also
clear any previously set anchor aria-current state: add code in the same block
(where this.host._internals!.ariaSelected is set to null and
this.host._internals!.states.delete('selected') is called) to clear the anchor
aria-current state—e.g. set this.host._internals!.ariaCurrent = null (or remove
the attribute on the stored anchor element, e.g.
this.host._internals!.anchor?.removeAttribute('aria-current')) so no stale
aria-current remains when switching to read-only.
---
Outside diff comments:
In `@projects/core/src/internal/controllers/state-selected.controller.test.ts`:
- Around line 105-118: Add a new test that exercises the runtime transition from
an anchor-selected state into readOnly: create an anchor element `a`, append it
to `element`, set `element.selected = true`, add the internal anchor state via
`element._internals.states.add('anchor')`, call `element.requestUpdate()` and
`await elementIsStable(element)` to stabilize, then toggle `element.readOnly =
true`, await stability again, and assert that `a.getAttribute('aria-current')`
is null (and that `element._internals.ariaSelected` is null and
`element.matches(':state(selected)')` is false) to verify `aria-current` is
removed; add the same transition-style test for the other affected block noted
around the 120-131 region.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b64ac6e0-5d5b-450a-ad84-3211122224f6
📒 Files selected for processing (26)
projects/core/src/internal/base/button.test.tsprojects/core/src/internal/base/button.tsprojects/core/src/internal/controllers/state-current.controller.test.tsprojects/core/src/internal/controllers/state-current.controller.tsprojects/core/src/internal/controllers/state-disabled.controller.test.tsprojects/core/src/internal/controllers/state-disabled.controller.tsprojects/core/src/internal/controllers/state-expanded.controller.test.tsprojects/core/src/internal/controllers/state-expanded.controller.tsprojects/core/src/internal/controllers/state-pressed.controller.test.tsprojects/core/src/internal/controllers/state-pressed.controller.tsprojects/core/src/internal/controllers/state-selected.controller.test.tsprojects/core/src/internal/controllers/state-selected.controller.tsprojects/core/src/internal/controllers/type-anchor.controller.test.tsprojects/core/src/internal/controllers/type-anchor.controller.tsprojects/core/src/internal/controllers/type-button.controller.test.tsprojects/core/src/internal/controllers/type-button.controller.tsprojects/core/src/internal/controllers/type-command.controller.test.tsprojects/core/src/internal/controllers/type-command.controller.tsprojects/core/src/internal/controllers/type-interest.controller.test.tsprojects/core/src/internal/controllers/type-interest.controller.tsprojects/core/src/internal/controllers/type-submit.controller.test.tsprojects/core/src/internal/controllers/type-submit.controller.tsprojects/core/src/internal/types/index.tsprojects/internals/eslint/src/local/reserved-property-names.jsprojects/internals/eslint/src/local/reserved-property-names.test.jsprojects/site/src/docs/elements/tag.md
coryrylan
force-pushed
the
topic-readonly-fix
branch
from
45df4e3 to
4b0aef6
Compare
coryrylan
commented
May 18, 2026
|
|
||
| it('should remove aria-disabled if readonly', async () => { | ||
| element.readonly = true; | ||
| element.readOnly = true; |
Collaborator
Author
There was a problem hiding this comment.
The native input form apis use readOnly for properties and readonly for attributes. This aligns our APIs to the platform read only APis. This updates all of our internal references to use readOnly but is backwards compatible with our pre-existing readonly property. The attribute behavior remains the same as all lowercase readonly. This is important to align on as we expand the form control capabilities.
| } | ||
|
|
||
| set readonly(value: boolean) { | ||
| this.toggleAttribute('readonly', value); |
Collaborator
There was a problem hiding this comment.
Minor nit:
There are two subtle ways this aliasing approach changes the deprecated readonly property vs its previous behavior:
- timing: synchronous reflection in the setter vs Lit's batched reflection in the in
update()lifecycle method - changed property tracking:
changedPropertieswon't include the deprecated property name in its set (with its oldValue).
The second is likely fine since we're supposed to be the only ones extending this internal base class, and would align any changedProperties optimizations around the new property name.
Did you run into an issue with just having the old property setter setting the new property (rather than toggling the attribute)?
Collaborator
Author
There was a problem hiding this comment.
I didn't see any differences but I'll double check before merging. Good callout, didn't think about the shift there.
johnyanarella
approved these changes
May 18, 2026
coryrylan
force-pushed
the
topic-readonly-fix
branch
from
4b0aef6 to
d2f7aa3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
projects/core/src/internal/controllers/state-selected.controller.ts (1)
30-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear anchor
aria-currentwhen switching to read-only.When
readOnlybecomes true after the anchor branch (lines 36-40) has already setaria-current="page", the early return at line 33 leaves the stalearia-currentattribute on the anchor element.🧹 Proposed fix to clear stale aria-current
if (this.host.readOnly) { this.host._internals!.ariaSelected = null; this.host._internals!.states.delete('selected'); + this.host.querySelector('a')?.removeAttribute('aria-current'); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/internal/controllers/state-selected.controller.ts` around lines 30 - 33, When handling the readOnly early-return in state-selected.controller (the block that uses this.host.readOnly and sets this.host._internals!.ariaSelected = null and this.host._internals!.states.delete('selected')), also clear any stale aria-current on the anchor: call this.host._internals!.anchor?.removeAttribute('aria-current') (or set it to null) before returning so any previously set aria-current="page" is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@projects/core/src/internal/controllers/state-selected.controller.ts`:
- Around line 30-33: When handling the readOnly early-return in
state-selected.controller (the block that uses this.host.readOnly and sets
this.host._internals!.ariaSelected = null and
this.host._internals!.states.delete('selected')), also clear any stale
aria-current on the anchor: call
this.host._internals!.anchor?.removeAttribute('aria-current') (or set it to
null) before returning so any previously set aria-current="page" is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5c347825-3cab-405c-a7f4-db5e9672134c
📒 Files selected for processing (26)
projects/core/src/internal/base/button.test.tsprojects/core/src/internal/base/button.tsprojects/core/src/internal/controllers/state-current.controller.test.tsprojects/core/src/internal/controllers/state-current.controller.tsprojects/core/src/internal/controllers/state-disabled.controller.test.tsprojects/core/src/internal/controllers/state-disabled.controller.tsprojects/core/src/internal/controllers/state-expanded.controller.test.tsprojects/core/src/internal/controllers/state-expanded.controller.tsprojects/core/src/internal/controllers/state-pressed.controller.test.tsprojects/core/src/internal/controllers/state-pressed.controller.tsprojects/core/src/internal/controllers/state-selected.controller.test.tsprojects/core/src/internal/controllers/state-selected.controller.tsprojects/core/src/internal/controllers/type-anchor.controller.test.tsprojects/core/src/internal/controllers/type-anchor.controller.tsprojects/core/src/internal/controllers/type-button.controller.test.tsprojects/core/src/internal/controllers/type-button.controller.tsprojects/core/src/internal/controllers/type-command.controller.test.tsprojects/core/src/internal/controllers/type-command.controller.tsprojects/core/src/internal/controllers/type-interest.controller.test.tsprojects/core/src/internal/controllers/type-interest.controller.tsprojects/core/src/internal/controllers/type-submit.controller.test.tsprojects/core/src/internal/controllers/type-submit.controller.tsprojects/core/src/internal/types/index.tsprojects/internals/eslint/src/local/reserved-property-names.jsprojects/internals/eslint/src/local/reserved-property-names.test.jsprojects/site/src/docs/elements/tag.md
- fix readonly to readOnly to align with native input type readOnly property Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-readonly-fix
branch
from
d2f7aa3 to
3e2e571
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
projects/core/src/internal/controllers/state-selected.controller.ts (1)
30-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear stale anchor
aria-currentwhen entering read-only mode.At Line 30, the early return skips cleanup of any previously set anchor
aria-current, so switching from selected-anchor state to read-only can leave stalearia-current="page".Suggested fix
if (this.host.readOnly) { this.host._internals!.ariaSelected = null; this.host._internals!.states.delete('selected'); + this.host.querySelector('a')?.removeAttribute('aria-current'); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/internal/controllers/state-selected.controller.ts` around lines 30 - 33, The early return when this.host.readOnly skips clearing any anchor aria-current, leaving stale aria-current="page"; update the read-only branch in state-selected.controller by also clearing the anchor current state—e.g., set this.host._internals!.ariaCurrent (or the internals property that tracks aria-current) to null/undefined and remove any 'current'/'aria-current' state before returning, alongside the existing this.host._internals!.ariaSelected = null and this.host._internals!.states.delete('selected').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@projects/core/src/internal/controllers/state-selected.controller.ts`:
- Around line 30-33: The early return when this.host.readOnly skips clearing any
anchor aria-current, leaving stale aria-current="page"; update the read-only
branch in state-selected.controller by also clearing the anchor current
state—e.g., set this.host._internals!.ariaCurrent (or the internals property
that tracks aria-current) to null/undefined and remove any
'current'/'aria-current' state before returning, alongside the existing
this.host._internals!.ariaSelected = null and
this.host._internals!.states.delete('selected').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 80bfb338-fe80-4161-9c65-53ddbcf64268
📒 Files selected for processing (26)
projects/core/src/internal/base/button.test.tsprojects/core/src/internal/base/button.tsprojects/core/src/internal/controllers/state-current.controller.test.tsprojects/core/src/internal/controllers/state-current.controller.tsprojects/core/src/internal/controllers/state-disabled.controller.test.tsprojects/core/src/internal/controllers/state-disabled.controller.tsprojects/core/src/internal/controllers/state-expanded.controller.test.tsprojects/core/src/internal/controllers/state-expanded.controller.tsprojects/core/src/internal/controllers/state-pressed.controller.test.tsprojects/core/src/internal/controllers/state-pressed.controller.tsprojects/core/src/internal/controllers/state-selected.controller.test.tsprojects/core/src/internal/controllers/state-selected.controller.tsprojects/core/src/internal/controllers/type-anchor.controller.test.tsprojects/core/src/internal/controllers/type-anchor.controller.tsprojects/core/src/internal/controllers/type-button.controller.test.tsprojects/core/src/internal/controllers/type-button.controller.tsprojects/core/src/internal/controllers/type-command.controller.test.tsprojects/core/src/internal/controllers/type-command.controller.tsprojects/core/src/internal/controllers/type-interest.controller.test.tsprojects/core/src/internal/controllers/type-interest.controller.tsprojects/core/src/internal/controllers/type-submit.controller.test.tsprojects/core/src/internal/controllers/type-submit.controller.tsprojects/core/src/internal/types/index.tsprojects/internals/eslint/src/local/reserved-property-names.jsprojects/internals/eslint/src/local/reserved-property-names.test.jsprojects/site/src/docs/elements/tag.md
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
2 participants
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.
Summary by CodeRabbit
New Features
Deprecation
Documentation
Tests
Chores