Skip to content
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

Make anchor behave like native anchor when contenteditable #1684

Merged
merged 16 commits into from
Dec 11, 2023

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Nov 22, 2023

Pull Request

🀨 Rationale

Fixes #1502

πŸ‘©β€πŸ’» Implementation

By design, the content-editable state is not inherited across the shadow boundary, but that is really what we want. To mimic this, I introduced a new wrapper div* around the native anchor element that we can set contenteditable on, so that the native anchor behaves as we want. Ideally, we would set it based on the value of isContentEditable of the host nimble-anchor. However, reacting to changes in the host's isContentEditable state is a problem. isContentEditable is a read-only property that is implemented by walking the ancestry chain until an elmement with contenteditable set is found. There is no practical way to detect changes to this value. We will instead require that clients explicitly set contenteditable on the nimble-anchor whenever it is inside a content-editable area. We can reflect the value of that attribute to the wrapper div.

*Note that the new wrapper div is necessary, because if we instead set contenteditable directly on the native anchor, it behaves differently (than when it is contained by a content-editable element): it remains focusable.

πŸ§ͺ Testing

Manually tested in Storybook, and wrote new unit tests.

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@m-akinc m-akinc requested a review from atmgrifter00 November 22, 2023 19:05
@m-akinc
Copy link
Contributor Author

m-akinc commented Nov 22, 2023

@atmgrifter00 could I get a buddy on this?

@m-akinc m-akinc marked this pull request as ready for review November 29, 2023 00:07
@m-akinc m-akinc requested a review from msmithNI as a code owner November 30, 2023 18:36
@m-akinc m-akinc requested a review from rajsite November 30, 2023 18:37
@m-akinc m-akinc requested a review from rajsite December 7, 2023 18:08
@m-akinc m-akinc requested a review from rajsite December 7, 2023 21:20
@rajsite rajsite self-requested a review December 7, 2023 23:48
@rajsite
Copy link
Member

rajsite commented Dec 11, 2023

Just a thought that occurred. In the future if we find ourselves having to do more with contenteditable we should treat it like lang and dir as a DesignToken that has to be configured on theme provider / DesignToken for nimble to be aware.

@m-akinc m-akinc enabled auto-merge (squash) December 11, 2023 18:59
@m-akinc m-akinc merged commit 487c344 into main Dec 11, 2023
11 checks passed
@m-akinc m-akinc deleted the users/makinc/contenteditable-anchor branch December 11, 2023 19:15
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.

Nimble anchor inside "contenteditable" div is clickable
5 participants