Skip to content

Use component ID for input and label to avoid ID collisions#1149

Closed
zeroasterisk wants to merge 1 commit intomainfrom
fix/unique-ids
Closed

Use component ID for input and label to avoid ID collisions#1149
zeroasterisk wants to merge 1 commit intomainfrom
fix/unique-ids

Conversation

@zeroasterisk
Copy link
Copy Markdown
Collaborator

@zeroasterisk zeroasterisk commented Apr 12, 2026

This PR addresses a bug where the input IDs were hardcoded as data in the Lit renderer, which made browser automation difficult.

I made what I believe are reasonable changes, but since Paul wrote the initial version of this, I seriously doubt my changes and worry that his initial version is better. I defer to @paullewis but present this as a possible PR

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces hardcoded 'data' IDs and label associations with dynamic values using this.id in the DateTimeInput and TextField components. The reviewer identified that using this.id directly causes ID collisions with the host element and suggested using a derived ID like ${this.id}-input instead. Additionally, the reviewer noted that the name attribute remains hardcoded and requested the inclusion of tests for these changes.

Comment thread renderers/lit/src/0.8/ui/datetime-input.ts Outdated
Comment thread renderers/lit/src/0.8/ui/datetime-input.ts Outdated
Comment thread renderers/lit/src/0.8/ui/text-field.ts Outdated
Comment thread renderers/lit/src/0.8/ui/text-field.ts Outdated
Copy link
Copy Markdown
Collaborator

@ditman ditman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the ## Unreleased section of the CHANGELOG with a description of this change.

Also, please consider adding this functionality to the other renderers so we don't have diverging capabilities in the basic catalog components!

(Also, also: only land this if it's a super critical need! I'd rather have this fix in the v0.9 stack :))

>
<label
for="data"
for="${this.id}-input"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are you setting this id property? Is it directly in the DOM, or does it come from the agent?

@zeroasterisk
Copy link
Copy Markdown
Collaborator Author

also: only land this if it's a super critical need! I'd rather have this fix in the v0.9 stack :)

That's enough warning for me to mark this as closed. I was nervous about it. We can always come back to it later.

@github-project-automation github-project-automation bot moved this from Todo to Done in A2UI Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants