fix(flow-client): set text node content before it is attached - #25683
fix(flow-client): set text node content before it is attached#25683totally-not-ai[bot] wants to merge 2 commits into
Conversation
A text node was created empty and got its content from the reactive flush, which runs after the node has been inserted into the DOM and after its parent element has been attached. A web component that resolves the state of its slotted content while it renders, such as vaadin-badge since 25.2.7, then sees an empty text node, and changing the data of a text node that is already assigned to a slot fires no slotchange to correct that. The content is now applied when the text node is bound, the same way element attributes, properties and styles already are. Fixes #25679
The GWT case only checked the text after the bind returned, which would still pass if the content were applied after the insertion. Both cases now record the text of every inserted child as it is at the moment of the insertion.
|
I'm wondering if this is a trivial bug fix or a fundamental behavior change in how flow and the client side interacts which has to be done at the next major.. locking at a comment from @web-padawan -- this might could affect a lot of components for better or worst 😅 |
|
|
@knoobie Fair question. Scope first, then why it matters less than it looks. What changes at runtime. Only the moment the initial text of a text node is written: from "the next reactive flush" to "when the node is bound". Both happen inside the same synchronous handling of the same server message, so no component can see the text later than it does today — only earlier, or at the same point. Later text updates still go through the flush. No protocol change, no API change, no new configuration. Why it is a consistency fix rather than a new rule. Attributes, properties, styles and class names are already applied while the element is bound ( The regression is already fixed on the web-components side. So this is not on the critical path for the reported bug and does not need to ride a patch release. What it still buys is that a component which measures itself while it renders sees the final text — the reason I have left the code as it is, since I think the ordering belongs in the client engine rather than in every component. But it is a client-side timing change that touches every application, so if you would rather not move that in a patch line, holding this for the next major is fine by me — nothing in it needs to be split up to land later. One triage note: #25679 was closed as a duplicate of vaadin/web-components#12663, so the link on this PR should be "Related to" rather than "Fixes". |
|
This might be a good change or might not, I will let @Legioth decide that. It is not a fix for the linked issue though. A web component should not have timing issues so the web-components fix is probably the good fix for badge |
|
@Artur- Agreed — the web-components fix is the right fix for the badge, and this is not a fix for the linked issue. #25679 was closed as a duplicate of vaadin/web-components#12663, and the badge fix shipped in What is left here should be judged on its own: attributes, properties, styles and class names are already applied while an element is bound, so an element reaches the DOM with them in place. The initial text of a text node was the only content written afterwards, in the reactive flush that follows. This makes both happen at the same point, so a component that measures itself while it renders sees the final text instead of an empty text node. @Legioth over to you. Happy to close this if moving that timing in the client engine is not something you want. |
2 participants
Summary
A text node was created empty and only got its text later, when the reactive flush ran — after the node was already in the DOM. Now the text is set while the node is bound, so it is never inserted empty.
What changed
TextBindingStrategy(both the Java/GWT and the TypeScript client) now callscomputation.recompute()right after registering the reactive computation, so the text is applied duringbind()instead of during the later flush. This matches how element attributes, properties and styles are already handled.vaadin-badgesince 25.2.7), which used to see an empty text node. Changing the data of a text node that is already assigned to a slot fires noslotchange, so there was no later chance to correct it. No public API changes; only the timing of the text update moves earlier.Fixes #25679
Test summary
appendChild), in both the TS and the GWT clientbind()returns, without anyReactive.flush()SimpleElementBindingStrategyChildrenTests→ "inserts a text-node child with its text already in place": rows 1, 2GwtBasicElementBinderTest.testAddTextNodeBeforeBind: rows 1, 2TextBindingStrategyTests→ "reactively binds the text property to the DOM node and unbinds on unregister" (extra assert added): rows 2, 3Not tested on purpose: the real
vaadin-badgeslot behaviour, since it lives outside this repository; the client tests reproduce the same insertion order instead.