[FIX] html_editor: update font size selector on set tag in toolbar

[Zynton]

In website builder:

1. Select text
2. Change heading type in the toolbar

-> the font size selector in the toolbar is blank:

This is because the font size input is in an iframe, and a chain of events triggered by changing the heading type (setTag) leads to the iframe's document to be re-rendered. This only happens in the website builder because of a call to updateContainers as a step_added handler that triggers the change_current_options_containers_listeners.

These listeners are triggered because setTag replaced the block which was the target of the BuilderOptionsPlugin with another. The target is not connected anymore so it was assumed we would always need to reset the containers, but the target didn't really change, it was simply replaced to change its tag. So this allows going through the checks to see if updating the containers is needed before triggering the listeners. Since in this case it isn't, they won't be and the iframe will remain intact.

[robodoo]

This PR targets the un-managed branch odoo-dev/odoo:master-mysterious-egg-next, it needs to be retargeted before it can be merged.

[dmo-odoo]

@robodoo r+

[robodoo]

Branch master-mysterious-egg-next is not within my remit, imma just ignore it.

[dmo-odoo]

@robodoo r-

[robodoo]

Branch master-mysterious-egg-next is not within my remit, imma just ignore it.

[dmo-odoo]

Didn't realize it wasn't for master

[robinlej]

It looks good, I'll wait for the tests to go green.

One small issue:

• Select some paragraph or header that is at the default font size for that style (so paragraph -> 16px, h2 -> 40px)
  (Alternatively: set some selection to header 2 and then double click on it to select it again)
• Open the "Select font style" dropdown and select the current style
  => the font-size input is empty

[Zynton]

This is just linting.

[Zynton]

@robinlej Thanks for reviewing.

I fixed the issue you raised and in doing so I changed approaches entirely (because it couldn't be solved with the other approach).

I added a note in the commit message on future work that might be needed because I lack proper context to know exactly what is or isn't expected in terms of re-rendering of Owl components in the context of html_builder.

Since it touches on some editor specific stuff, I thought it would be best for one of our own to review it too. @sébastien (sge) and I discussed it IRL, I made some more changes, and he approved the approach.

[Zynton]

@robinlej Do you know who wrote updateContainers or at least the following section:

odoo/addons/html_builder/static/src/core/builder_options_plugin.js

Lines 115 to 121 in 2e9b9d5

	if (!this.target || !this.target.isConnected) {
	this.lastContainers = this.lastContainers.filter((c) => c.element.isConnected);
	this.target = this.lastContainers.at(-1)?.element;
	this.dependencies.history.setStepExtra("optionSelection", this.target);
	this.dispatchTo("change_current_options_containers_listeners", this.lastContainers);
	return;
	}

?

If we were to remove return, setStepExtra and dispatchTo would be called anyway, unless we enter if (areSameOptions). Is there a specific reason for skipping these checks when we lost the target?

Removing

odoo/addons/html_builder/static/src/core/builder_options_plugin.js

Lines 118 to 120 in 2e9b9d5

	this.dependencies.history.setStepExtra("optionSelection", this.target);
	this.dispatchTo("change_current_options_containers_listeners", this.lastContainers);
	return;

so that the checks happen normally fixes the bug (because the option didn't change, only the reference to the element), so I could remove everything in this PR and simply remove these three lines instead.

So was there a reason for it or can these lines be removed?

cc @sebgeelen

[robinlej]

Many hands have been involved around updateContainers and many squashes happened in between, so hard to tell. That said, the basic logic around that (returning early if no target) seems to date back from the beginning of the refactoring. But it has moved a lot in the meantime.

From the look of it, it seems mainly there to avoid frequent computations while we can just rely on the last known containers. We could test to see if there's a performance cost to removing it.

[Zynton]

@robinlej So I applied that fix, and I added a test for safety (passes with the fix, doesn't otherwise). I tested manually, with many (nested) snippets and didn't notice any obvious performance cost.

Edit: one test failed ("useDomState callback shouldn't be called when the editingElement is removed") and revealed a special case so instead of removing the lines I put them in an if so they're only executed if the target is still not found in the DOM.

[Zynton]

Closed in favor of odoo#211807