Skip to content

fix: ensure isReadonly applies to all non-literal date segments #7969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Persists
Copy link
Contributor

Found During Testing

When isReadonly is set, all segments should be non-editable.

For RAC components, this ensures that when isReadonly is applied to DateField or TimeField, the non-literal DateSegments have isReadonly=true and isEditable=false

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@Persists Persists changed the title fix: force data-readonly when datefield is readonly Ensure isReadonly applies to all non-literal DateSegments Mar 20, 2025
@Persists Persists changed the title Ensure isReadonly applies to all non-literal DateSegments fix: ensure isReadonly applies to all non-literal DateSegments Mar 20, 2025
@Persists Persists changed the title fix: ensure isReadonly applies to all non-literal DateSegments fix: ensure isReadonly applies to all non-literal date segments Mar 20, 2025
@Persists Persists marked this pull request as draft March 20, 2025 14:46
@Persists Persists force-pushed the fix-readonly-datesegment branch from 08e945b to 6a6319c Compare March 20, 2025 14:47
@Persists Persists force-pushed the fix-readonly-datesegment branch from 6a6319c to 3ed43ed Compare March 20, 2025 14:48
@Persists Persists marked this pull request as ready for review March 20, 2025 14:48
@snowystinger
Copy link
Member

@nwidynski
Copy link
Contributor

nwidynski commented Mar 20, 2025

@snowystinger Here is the reproduction: https://codesandbox.io/p/devbox/floral-wind-2fx6pc?workspaceId=ws_nU6d6H4r59ATf4jbZ5rYc

You can see that only the literal elements carry the data-readonly attribute (red background). This is because isEditable in the state does not take isReadOnly into consideration, as we do in the aria layer:

let isEditable = !state.isDisabled && !state.isReadOnly && segment.isEditable;

This causes a mismatch between the data-* props and the aria props, e.g. contenteditable. We can either take isReadOnly into consideration while building the state or possibly just adjust the RAC layer.

To be honest i feel like both isDisabled and isReadOnly should be considered right in the state, but that probably has downstream implications.

@snowystinger
Copy link
Member

I'm not sure why the literals have readonly at all, the control is disabled, and the literals aren't interactive to being with. In aria, disabled almost the same as not existing. So it seems to me that readonly is unnecessary and shouldn't even be appearing there unless the control has isReadOnly
Also, those are literals, they are readonly to start with and the data-attribute won't be read.

Maybe we should back up, what is the use case you're trying to build?

@nwidynski
Copy link
Contributor

nwidynski commented Mar 21, 2025

@snowystinger No use case besides styling being difficult right now for isReadOnly, as its neither applied to all segments nor on the parent field. Our component test suite prompted us with this issue.

@LFDanLu
Copy link
Member

LFDanLu commented Mar 21, 2025

@nwidynski Just to clarify, the expectation here would be that data-readonly would only appear on the segments when isReadOnly is set on the DateField but NOT when isDisabled is applied correct?

@nwidynski
Copy link
Contributor

nwidynski commented Mar 21, 2025

@LFDanLu Yep. We had expected data-readonly to appear on ALL segments when isReadOnly is set on the field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants