From 0ecee11a2f8a8814e1997c2d0b4ab0b49da949d0 Mon Sep 17 00:00:00 2001 From: WorldSEnder Date: Wed, 28 Sep 2022 08:32:19 +0200 Subject: [PATCH] Fix portal shifting on reconciliation too often (#2891) * fix portal shifting on reconciliation too often the public vdom api changes to only allow directly setting a Node as sibling (still optional) instead of a NodeRef. This was the intention all along, since the NodeRef was not dynamically tracked, and creating a portal into a subtree already controlled by yew is not supported anway. * fix feature soundness * fix doc tests --- packages/yew/src/dom_bundle/bportal.rs | 55 +++++++++++++++++++-- packages/yew/src/html/mod.rs | 14 +++--- packages/yew/src/virtual_dom/vportal.rs | 11 ++--- website/docs/concepts/html/introduction.mdx | 8 ++- 4 files changed, 68 insertions(+), 20 deletions(-) diff --git a/packages/yew/src/dom_bundle/bportal.rs b/packages/yew/src/dom_bundle/bportal.rs index 496bb9f62..cc98cca8e 100644 --- a/packages/yew/src/dom_bundle/bportal.rs +++ b/packages/yew/src/dom_bundle/bportal.rs @@ -48,6 +48,11 @@ impl Reconcilable for VPortal { inner_sibling, node, } = self; + let inner_sibling = { + let sib_ref = NodeRef::default(); + sib_ref.set(inner_sibling); + sib_ref + }; let inner_root = root.create_subroot(parent.clone(), &host); let (_, inner) = node.attach(&inner_root, parent_scope, &host, inner_sibling.clone()); ( @@ -92,9 +97,11 @@ impl Reconcilable for VPortal { } = self; let old_host = std::mem::replace(&mut portal.host, host); - let old_inner_sibling = std::mem::replace(&mut portal.inner_sibling, inner_sibling); - if old_host != portal.host || old_inner_sibling != portal.inner_sibling { + let should_shift = old_host != portal.host || portal.inner_sibling.get() != inner_sibling; + portal.inner_sibling.set(inner_sibling); + + if should_shift { // Remount the inner node somewhere else instead of diffing // Move the node, but keep the state let inner_sibling = portal.inner_sibling.clone(); @@ -123,12 +130,15 @@ impl BPortal { mod layout_tests { extern crate self as yew; + use gloo::utils::document; use wasm_bindgen_test::{wasm_bindgen_test as test, wasm_bindgen_test_configure}; + use web_sys::HtmlInputElement; use yew::virtual_dom::VPortal; - use crate::html; + use super::*; use crate::tests::layout_tests::{diff_layouts, TestLayout}; use crate::virtual_dom::VNode; + use crate::{create_portal, html}; wasm_bindgen_test_configure!(run_in_browser); @@ -200,4 +210,43 @@ mod layout_tests { diff_layouts(layouts) } + + fn setup_parent_with_portal() -> (BSubtree, AnyScope, Element, Element) { + let scope = AnyScope::test(); + let parent = document().create_element("div").unwrap(); + let portal_host = document().create_element("div").unwrap(); + let root = BSubtree::create_root(&parent); + + let body = document().body().unwrap(); + body.append_child(&parent).unwrap(); + body.append_child(&portal_host).unwrap(); + + (root, scope, parent, portal_host) + } + + #[test] + fn test_no_shift() { + // Portals shouldn't shift (which e.g. causes internal inputs to unfocus) when sibling + // doesn't change. + let (root, scope, parent, portal_host) = setup_parent_with_portal(); + let input_ref = NodeRef::default(); + + let portal = create_portal( + html! { }, + portal_host, + ); + let (_, mut bundle) = portal + .clone() + .attach(&root, &scope, &parent, NodeRef::default()); + + // Focus the input, then reconcile again + let input_el = input_ref.cast::().unwrap(); + input_el.focus().unwrap(); + + let _ = portal.reconcile_node(&root, &scope, &parent, NodeRef::default(), &mut bundle); + + let new_input_el = input_ref.cast::().unwrap(); + assert_eq!(input_el, new_input_el); + assert_eq!(document().active_element(), Some(new_input_el.into())); + } } diff --git a/packages/yew/src/html/mod.rs b/packages/yew/src/html/mod.rs index 68c4f63c5..08c96a576 100644 --- a/packages/yew/src/html/mod.rs +++ b/packages/yew/src/html/mod.rs @@ -124,13 +124,6 @@ impl NodeRef { let node = self.get(); node.map(Into::into).map(INTO::from) } - - /// Place a Node in a reference for later use - pub(crate) fn set(&self, node: Option) { - let mut this = self.0.borrow_mut(); - this.node = node; - this.link = None; - } } #[cfg(feature = "csr")] @@ -156,6 +149,13 @@ mod feat_csr { node_ref.set(Some(node)); node_ref } + + /// Place a Node in a reference for later use + pub(crate) fn set(&self, node: Option) { + let mut this = self.0.borrow_mut(); + this.node = node; + this.link = None; + } } } diff --git a/packages/yew/src/virtual_dom/vportal.rs b/packages/yew/src/virtual_dom/vportal.rs index 1e418a867..280911324 100644 --- a/packages/yew/src/virtual_dom/vportal.rs +++ b/packages/yew/src/virtual_dom/vportal.rs @@ -3,14 +3,13 @@ use web_sys::{Element, Node}; use super::VNode; -use crate::html::NodeRef; #[derive(Debug, Clone)] pub struct VPortal { /// The element under which the content is inserted. pub host: Element, /// The next sibling after the inserted content. Most be a child of `host`. - pub inner_sibling: NodeRef, + pub inner_sibling: Option, /// The inserted node pub node: Box, } @@ -20,7 +19,7 @@ impl VPortal { pub fn new(content: VNode, host: Element) -> Self { Self { host, - inner_sibling: NodeRef::default(), + inner_sibling: None, node: Box::new(content), } } @@ -31,11 +30,7 @@ impl VPortal { pub fn new_before(content: VNode, host: Element, inner_sibling: Option) -> Self { Self { host, - inner_sibling: { - let sib_ref = NodeRef::default(); - sib_ref.set(inner_sibling); - sib_ref - }, + inner_sibling, node: Box::new(content), } } diff --git a/website/docs/concepts/html/introduction.mdx b/website/docs/concepts/html/introduction.mdx index 5af8a4741..3c5dca501 100644 --- a/website/docs/concepts/html/introduction.mdx +++ b/website/docs/concepts/html/introduction.mdx @@ -156,14 +156,18 @@ free to [chime in on this issue](https://github.com/yewstack/yew/issues/1334). Attributes are set on elements in the same way as in normal HTML: ```rust +use yew::prelude::*; + let value = "something"; -html! {
+html! {
}; ``` Properties are specified with `~` before the element name: ```rust -html! { } +use yew::prelude::*; + +html! { }; ``` :::tip