Variables: Improve a11y#1404
Conversation
| return result; | ||
| } | ||
|
|
||
| export const getVariableControlId = (variableType: VariableType, key?: string) => { |
There was a problem hiding this comment.
have we considered making the control id a property on the variable/controller instead of a separate utility method?
right now every consumer needs to call getVariableControlId(type, key) with the right args, and on the grafana side (
#120758) the call sites are already a bit inconsistent (variable.state.key vs variable.id). Also key was added to AdHocFiltersController purely to feed into this function, feels like the controller could just expose the final controlId directly.
something like:
getControlId(): string {
return getVariableControlId(this.state.type, this.state.key);
}
id={model.getControlId()}
WDYT?
There was a problem hiding this comment.
I added it to the adhoc controller here and removed the key prop, but I think adding it to all variable models (or introducing a base class just fort his api) might be a bit of an overkill for an accessibility change? 🤔
I agree the call sites are inconsistent, and looking at them again, the changes in OptionsPicker.tsx, PickerRenderer.tsx and TextBoxVariablePicker.tsx might not make sense since they're legacy non-scenes components, so they probably don't need to use scenes utils. I reverted those changes in a new commit it that PR. I tested with scenes disabled and the new changes and it works fine as well
| } | ||
|
|
||
| const getKeyComboboxElement = () => getAllByRole(screen.getByTestId('AdHocFilter-'), 'combobox')[0]; | ||
| const getAdHocInputElement = () => screen.getByPlaceholderText('Filter by label values'); |
There was a problem hiding this comment.
the new UI changes this placeholder to I think + label = value, I think that is why tests are failing
mdvictor
left a comment
mdvictor
left a comment
There was a problem hiding this comment.
LGTM! I've mainly focused on the adhoc code changes, although it looks good overall. Tested the adhoc filters var and with a custom var
idastambuk
added
release
|
🚀 PR was released in |
3 participants
Right now, we're not announcing variable labels to screen readers. This PR:
getVariableControlIdfunction that gets the id for the variable that will match the htmlFor. This function is used in core grafana PR that completes this featuregetVariableControlIdTo test:
This is how it works after the fix, with the adjustment for ad hoc filters:
Screen.Recording.2026-03-20.at.15.29.12.mov