fix: allow text in read-only components to be selected#4417
Conversation
227fc00 to
119d85c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4417 +/- ##
=======================================
Coverage 97.41% 97.41%
=======================================
Files 933 933
Lines 29577 29577
Branches 10753 10752 -1
=======================================
Hits 28813 28813
- Misses 716 757 +41
+ Partials 48 7 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Would it be possible to add coverage, maybe with the Selection API? https://developer.mozilla.org/en-US/docs/Web/API/Selection |
|
|
||
| // read-only in-filtering-token (i.e. for property filter readOnlyOperations) | ||
| // should retain standard borders, only the icon changes | ||
| &.readonly:not(&.disabled):not(&.in-filtering-token) { |
There was a problem hiding this comment.
Note: probably a minor use case, but the :not(&.in-filtering-token) excludes from this fix read-only operator selectors in property filter:
![]()
At the same time, we would prefer placeholder text to not be selectable, because that comes with collateral a11y issues (if it is interactive, it will have other requirements such as contrast):
There was a problem hiding this comment.
I've moved the user-select: text to a separate &.readonly:not(&.disabled) rule that also covers filtering tokens. I've also added user-select: none to .placeholder.
8de254f to
2998c7c
Compare
|
I've rebased the branch, added test coverage (see the new "Read-only mode" describe block), and addressed your comments |
| }; | ||
|
|
||
| test( | ||
| 'trigger text is selectable via the Selection API', |
There was a problem hiding this comment.
The test should assert that the text is selectable via user interaction, not via the Selection API. The Selection API can be used for verification.
One way you can select text in tests is by double-clicking, with the caveat that the will only select part of the text (in this case, "2018") when it contains separators such as dashes.
Proposed:
test(
'trigger text is selectable',
setupReadOnlyTest(async (page, browser) => {
const triggerSelector = page.dateRangePickerTrigger;
const triggerText = await page.getTriggerText();
expect(triggerText).toContain('2018-01-09');
const element = browser.$(triggerSelector);
await element.doubleClick();
const selectedText = await browser.execute(() => window.getSelection()?.toString());
expect(selectedText).not.toBeFalsy();
expect(triggerText).toContain(selectedText);
})
- Apply `user-select: text` to all read-only button-triggers (including those inside property-filter tokens, i.e. `readOnlyOperations`), while keeping `form-readonly-element` borders scoped to the non-filtering case. Fixes the gap where read-only property-filter operator text was still unselectable. - Set `user-select: none` on the custom placeholder spans in Select (`.placeholder`) and DateRangePicker (`.label-text`). When these components are read-only without a value, the trigger renders placeholder text; preserving its non-selectable nature avoids accessibility concerns that accompany selectable/interactive text. - Add a DateRangePicker integration test that double-clicks the read-only trigger to simulate a real user selection and verifies via the Selection API that the selected text comes from the trigger's content. Double-click is gated by `user-select`, so this actually exercises the fix (unlike a programmatic Range, which works regardless).
3102a88 to
42c67fd
Compare
ef48a29
Fixes #4411
This PR modifies the styles on
DateRangePicker,FileTokenGroup,SelectandMultiselectto make text selectable.Testing
I've added a read-only toggle to the demo pages and was able to manually test each of these.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.