[4.0] Update choices.js and fix a bug with detaching

[Fedik]

Pull Request for Issue #23448 and #23402 (comment)

Summary of Changes

Update choices.js and fix issue caused by moving the element in DOM tree.

Before testing, run npm install

Testing Instructions

Please read #23448

@Hackwar please test it, it should fix your issue also.

Documentation Changes Required

none

side note: @dgrammatiko I would suggest to review joomla-tab.js behavior, it should not move content around on resize. It mess up DOM tree, and will cause many more bugs in future.

[dgrammatiko]

I would suggest to review joomla-tab.js

That code is really bad, I know! But Is exposing all the wrong things that we did in the custom elements, so don't expect an update before all the custom elements are correctly implemented.

It mess up DOM tree

There is no rule that prohibits anyone to move nodes around the tree. Actually this behaviour reveals our bad implementation of custom elements... On the practical side, I guess the idea to transform tabs to accordion should be dropped for the production code, although it's a great accessibility win, for the reasons you also mention: future bugs (eg devs bad scripts)

This PR is taking this component one step close to production but there are 2 more things that needs to be patched here:

• You should be able to create the element without any content and append the content later one (mutation observer) so the custom element satisfy:

x = document.createElement('joomla-field-fancy-select');

document.body.appendChild(x)

x.innerHTML = `<select><option value="zzzz" selected>oooo</option>`

• The choices.js should be integral part of the custom element code, relying on an external script to be already available is wrong in this context. Custom elements by nature are async, in Joomla we're doing something weird loading them after the rest of the scripts (lazily). This behaviour works right now but once you change the order (for whatever reason) then the custom element here will be broken...

[dgrammatiko]

Actually this part is correct. You need to destroy the current instance and recreate it. The missing part here is that you need a value attribute in the custom element that keeps the value (json). Also if you do it this way I guess you can skip the select element altogether and just create hidden input with the value so PHP is also happy...

[Fedik]

There is no rule that prohibits anyone to move nodes around the tree...

There one golden rule: "Do not mess the DOM"

Actually this behaviour reveals our bad implementation of custom elements...

It not only about CE. Joomla! forms are heavy already (can have 100+ inputs), moving nodes just force a browser re-evaluate everything. Additionally Developers can have a custom scripts (even more fancy than CE), that no one know how they will behavior if you change DOM in way tabs.js doing.

Instead of moving content, the tabs.js can just move the "navigation buttons", or even render them twice and switch by media query in CSS, that even more flexible. Also, example when you have just a 2 tab, you do not need to switch them to accordion. It should not be forced and hardcoded to 920px.

You should be able to create the element without ...

A <select> already required, you just will get an error about. So I do not see a problem.
This is correct use:

x = document.createElement('joomla-field-fancy-select');
x.innerHTML = '<select><option value="zzzz" selected>oooo</option>';
document.body.appendChild(x);

It can be change later on demand, when someone really really need it.

The choices.js should be integral part of the custom element code...

Injecting choicies.js (or any othe blablba.js) in to CE also wrong

[dgrammatiko]

Injecting choicies.js (or any othe blablba.js) in to CE also wrong

It's the second best choice since we cannot use imports which is the right way.

This is correct use:

x = document.createElement('joomla-field-fancy-select');
x.innerHTML = '<select><option value="zzzz" selected>oooo</option>';
document.body.appendChild(x);

I'm afraid that the DOM can also work the other way around (create an empty element and inject the contents later on). You can force people to do it your way but you're not complying with the specs...

It should not be forced and hardcoded to 920px

Tabs shouldn't really change to accordion, that was a suggestion from the a11y group that I rushed to code. Anyways tabs already have been rewritten without this part, once the rest of the custom elements in this repo have been corrected I'll pull that code in

[Fedik]

It's the second best choice since we cannot use imports

I have changed, will be safe now, without injecting and without "while" loops

not complying with the specs...

I am who do the specs here 😄
So it require <select> to be preset. And any direct manipulation of CE children not allowed.
It like a single CE with shadow DOM, but without shadow DOM 😉

[dgrammatiko]

And any direct manipulation of CE children not allowed.

Not really, check this:
Setting up a mutation observer:

joomla-cms/build/media/webcomponents/js/editor-none/editor-none.js

Lines 18 to 21 in e887207

	// Watch for children changes.
	// eslint-disable-next-line no-return-assign
	new MutationObserver(() => this.childrenChange())
	.observe(this, { childList: true });

and then care only for a specific element (in that case textarea)

joomla-cms/build/media/webcomponents/js/editor-none/editor-none.js

Lines 98 to 111 in e887207

	/**
	* Called when element's child list changes
	*/
	childrenChange() {
	// Ensure the first child is an input with a textarea type.
	if (this.firstElementChild
	&& this.firstElementChild.tagName
	&& this.firstElementChild.tagName.toLowerCase() === 'textarea'
	&& this.firstElementChild.getAttribute('id')) {
	this.editor = this.firstElementChild;
	this.unregisterEditor();
	this.registerEditor();
	}
	}

But anyways this is good enough for the time, let's patch the rest as well...

[wilsonge]

Please update the package lock

[Fedik]

done

[Fedik]

@dgrammatiko but you know the cost of MutationObserver ? 😉
Especially when choices.js (or other script that used with CE, eg editors scripts etc) doing a changes itself.

From my point of view it doesn't worth to bother about it 😉

[Fedik]

this is broken now, need full redo

[dgrammatiko]

@Fedik oops sorry, copying the js to a new PR should be easy tho