Add tests to confirm an input in the semi coupled form work with autocomplete. If it turns out that doesn't currently work... this issue should provide the fix as well.
Create a test-only module that uses a form_alter to add an autocomplete input to the props form and add e2e tests to confirm it works.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | autocomplete-test-passes-headed.png | 639.23 KB | bnjmnm |
| #10 | Screenshot 2025-01-28 at 2.56.47 PM.png | 40.86 KB | bnjmnm |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
bnjmnmComment #3
wim leersComment #4
bnjmnmComment #5
bnjmnmAdded a test module in the MR and found that the autocomplete inputs (which had been proven to work in the past but never got test coverage) no longer work due to the input now using the Radix TextField
component. This component intercepts a few props, including className and applies them to the input's parent<div>instead of the<input/>and Drupal's autocomplete initializes based on the presence of.form-autocompleteon an<input/>element.Comment #6
bnjmnmBetween this issue and #3481719: Get Semi-coupled Checkbox form elements to work with the State API , if we want to continue using Radix we need to have a way to customize the attributes sent to the input elements within the component. I filed an issue with primitives about doing this with
Checkbox, and if that is welcomed I'll do the same in Radix themes for theTextFieldcomponentComment #7
effulgentsia commentedThis is a blocker for #3499279: Make link widget autocomplete work (for uri and uri-reference props).
Comment #8
effulgentsia commentedComment #9
effulgentsia commentedIf Radix doesn't accept the PR, is there a way we can accomplish it (without forking Radix) via
ref? Does Radix already forward refs to the form element we'd want our attributes on?Comment #10
bnjmnmIt can be done with a ref!
Updated MR on the way...
Comment #12
bnjmnmComment #13
effulgentsia commentedI think the MR looks great. Should we upgrade to React 19 and take advantage of the nicer syntax for refs?
Comment #14
wim leers#13: Let's not expand the scope here. 🙏 Opened #3510610: [PP-1] After upgrading to React 19, refactor forwardRefs to ref props for that ☺️
Some discoverability and consistency remarks, plus I don't understand why test is being changed. I bet it's all trivial for @bnjmnm to address 👍
Comment #15
bnjmnmFeedback addressed and the Cypress memory issue was seemingly addressed by making the test module name shorter (???)
Comment #16
wim leers#15: WTF?!?! 🤪
Self-addressed remaining nits. Now needs FE review.
Comment #18
bnjmnm@balintbrews approved this but it looks like we still need a semi-coupled owner to approve too> @hooroomoo seems like the best candidate for that
Comment #19
bnjmnmAfter a few days of e2e tests passing reliably, the autocomplete test introduced here began encountering the unpredictable+ambiguous "
We detected that the Electron Renderer process just crashed.". So we can continue working without false-red-x's popping up, the autocomplete e2e test is bypassed on headless and the screenshot below shows that it is passing fine on headed.Comment #23
bnjmnmMerged. Thanks for the reviews everybody!