Use separate signals to prevent executing connects/disconnects in the Choices controller #8203
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5d477d3 to
e4bf155
Compare
e4bf155 to
92d9d80
Compare
92d9d80 to
4199b22
Compare
leofeyer
approved these changes
Mar 19, 2025
|
Thank you @m-vo. |
leofeyer
pushed a commit
that referenced
this pull request
May 13, 2025
Description ----------- Fixes #8248 ### Overview We had numerous attempts at integrating Choices with our mutation observer based handling of components (#7901, #8075, #8203, …). The main issue is, that Choices wraps/unwraps the underlying select element by removing and readding it to the DOM, which is again triggering mutations. On top there are some global event-listeners. This all screams "race condition". This PR therefore goes down a completely different route. The strategy basically looks like this: * On `connect`, create a sibling div element (the _host_) and hide the original element (add `.hidden` class) * Create a separate DOM (shadow root) with a clone of the original select element inside the host and initialize Choices on that element * When there are changes (`change` event on Choices-owned select), update the original element. * On `disconnect`/`beforeCache`, remove the host and show the original element again (remove `.hidden` class) Now, no unwanted mutations occur anymore. In fact, the potentially hundreds of added Choices DOM nodes are also not part of the original DOM which should also make queries etc. on parent elements cheaper. ### Implementation details Choices was a contao-component, now we install it as a npm package (88a718c). We therefore do not load the respective css/js files in `be_main` anymore. Due to isolation, the shadow DOM styles cannot target classes on the parent (custom properties such as our color presets penetrate the boundary, though). There is the `:host` selector, however, that targets the host element. In order to allow styling depending on the current color scheme, I moved the color scheme controller to the `html` element and made a small change, so that you now can have multiple outlet targets where the classes are applied (d7f298e). The host elements are now such targets. Because global styles do not penetrate the boundary, we are only applying them to the shadow roots themselves. With webpack that can be done quite nicely: We use an annotated import to get the loader output and then create a `CssStyleSheet` object from it, that is shared across all controller instances (b076c53). Speaking of styles… There were the choices default styles, our adjustments from the contao-component and the base styles (`backend.css`) - all of them overwriting each other and sometimes only working with a very specific specificity (say that 3 times in a row). Now that styles are isolated, I moved them in one file (`assets/component/choices.pcss` importing `choices.js/public/assets/styles/choices.css`) and refactored them. Even with a lot of testing, it is very likely that I broke something while doing so. So please have a close look at this when reviewing. ### `contao--choices:create` event Because the controller is now basically rewritten, I also added an event that lets you alter the config before the Choices instance is created. If we want to go with this one-line-addition, this PR will also supersede #8115 and #8154. Usage: ```js selectElement.addEventLister('contao--choices:init', (event) => { const config = event.details.config; config.foo = bar; // alter config // If you need to target the select node, the Choices // instance is created on, use `event.details.select`. }); ``` Commits ------- 88a718c remove Choices contao-component and install it as a npm package d7f298e move contao--color-scheme controller to the html element and allow ha… b076c53 rewrite contao--choices controller with shadow isolation and refactor… efdb4d1 fix filter layout b564d49 build assets bd80975 simplify import 798f69a allow defining a config value via a data attribute f810e21 rename custom properties to a generic name 1f938ea build assets f907a1d fix border, remove unnecessary properties, reorder f367ac8 normalize line height 392cded fix class name 9adfc31 fix specificity 087b59b fix .is-open selector + simplify d0fef2b fix tl_search panel 0a317ca make this friendly for upstream merging by applying FC CS 2b62096 build assets 4df465b Add widths for the `shadow-root` instances for choices 4ff6c23 Adjust the font-size for .choices list items d166609 Only add margin to initialized choices controllers that are not withi… Co-authored-by: zoglo <55794780+zoglo@users.noreply.github.com>
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8192
We need separate signals for when mutations are triggering subsequent connects/disconnects while adding or removing Choices elements. In the scenario in #8192 we are basically directly removing and readding an initialized Choices element in a single microtask. With the previous logic, the guard would still be up when readding, effectively stopping the connect
@zoglo Could you double check if this fixes the issue (and Choices is behaving correctly everywhere)?
Sidenote: I still dislike how Choices is altering the DOM (wrapping the element instead of putting its things aside) - maybe we should create an issue/PR there, to improve the situation. There are probably other frameworks (Vue? Anyone using mutation observers?) that are affected by this as well.