Eliminate wrapper elements (#602)

* Factor out all DOM-structure modifying code into an abstraction. Currently, implementation is still backed by the same underlying DOM APIs

* Implement LogicalElement properly (store hierarchy in own property; use comment nodes as containers)

* In LogicalElement, be explicit about unsupported scenarios

* Update E2E tests to stop assuming existence of wrapper elements

* Where supported, store private properties using opaque symbols as keys

src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/BrowserRenderer.ts

@@ -4,13 +4,14 @@ import { getTreeFramePtr, renderTreeFrame, FrameType, RenderTreeFramePointer } f
 import { platform } from '../Environment';
 import { EventDelegator } from './EventDelegator';
 import { EventForDotNet, UIEventArgs } from './EventForDotNet';
+import { LogicalElement, toLogicalElement, insertLogicalChild, removeLogicalChild, getLogicalParent, getLogicalChild, createAndInsertLogicalContainer, isSvgElement } from './LogicalElements';
 const selectValuePropname = '_blazorSelectValue';
 let raiseEventMethod: MethodHandle;
 let renderComponentMethod: MethodHandle;
 
 export class BrowserRenderer {
   private eventDelegator: EventDelegator;
-  private childComponentLocations: { [componentId: number]: Element } = {};
+  private childComponentLocations: { [componentId: number]: LogicalElement } = {};
 
   constructor(private browserRendererId: number) {
     this.eventDelegator = new EventDelegator((event, componentId, eventHandlerId, eventArgs) => {
@@ -19,7 +20,7 @@ export class BrowserRenderer {
   }
 
   public attachRootComponentToElement(componentId: number, element: Element) {
-    this.attachComponentToElement(componentId, element);
+    this.attachComponentToElement(componentId, toLogicalElement(element));
   }
 
   public updateComponent(componentId: number, edits: System_Array<RenderTreeEditPointer>, editsOffset: number, editsLength: number, referenceFrames: System_Array<RenderTreeFramePointer>) {
@@ -39,11 +40,11 @@ export class BrowserRenderer {
     this.eventDelegator.removeListener(eventHandlerId);
   }
 
-  private attachComponentToElement(componentId: number, element: Element) {
+  private attachComponentToElement(componentId: number, element: LogicalElement) {
     this.childComponentLocations[componentId] = element;
   }
 
-  private applyEdits(componentId: number, parent: Element, childIndex: number, edits: System_Array<RenderTreeEditPointer>, editsOffset: number, editsLength: number, referenceFrames: System_Array<RenderTreeFramePointer>) {
+  private applyEdits(componentId: number, parent: LogicalElement, childIndex: number, edits: System_Array<RenderTreeEditPointer>, editsOffset: number, editsLength: number, referenceFrames: System_Array<RenderTreeFramePointer>) {
     let currentDepth = 0;
     let childIndexAtCurrentDepth = childIndex;
     const maxEditIndexExcl = editsOffset + editsLength;
@@ -60,41 +61,55 @@ export class BrowserRenderer {
         }
         case EditType.removeFrame: {
           const siblingIndex = renderTreeEdit.siblingIndex(edit);
-          removeNodeFromDOM(parent, childIndexAtCurrentDepth + siblingIndex);
+          removeLogicalChild(parent, childIndexAtCurrentDepth + siblingIndex);
           break;
         }
         case EditType.setAttribute: {
           const frameIndex = renderTreeEdit.newTreeIndex(edit);
           const frame = getTreeFramePtr(referenceFrames, frameIndex);
           const siblingIndex = renderTreeEdit.siblingIndex(edit);
-          const element = parent.childNodes[childIndexAtCurrentDepth + siblingIndex] as HTMLElement;
+          const element = getLogicalChild(parent, childIndexAtCurrentDepth + siblingIndex);
-          this.applyAttribute(componentId, element, frame);
+          if (element instanceof HTMLElement) {
+            this.applyAttribute(componentId, element, frame);
+          } else {
+            throw new Error(`Cannot set attribute on non-element child`);
+          }
           break;
         }
         case EditType.removeAttribute: {
           // Note that we don't have to dispose the info we track about event handlers here, because the
           // disposed event handler IDs are delivered separately (in the 'disposedEventHandlerIds' array)
           const siblingIndex = renderTreeEdit.siblingIndex(edit);
-          removeAttributeFromDOM(parent, childIndexAtCurrentDepth + siblingIndex, renderTreeEdit.removedAttributeName(edit)!);
+          const element = getLogicalChild(parent, childIndexAtCurrentDepth + siblingIndex);
+          if (element instanceof HTMLElement) {
+            const attributeName = renderTreeEdit.removedAttributeName(edit)!;
+            element.removeAttribute(attributeName);
+          } else {
+            throw new Error(`Cannot remove attribute from non-element child`);
+          }
           break;
         }
         case EditType.updateText: {
           const frameIndex = renderTreeEdit.newTreeIndex(edit);
           const frame = getTreeFramePtr(referenceFrames, frameIndex);
           const siblingIndex = renderTreeEdit.siblingIndex(edit);
-          const domTextNode = parent.childNodes[childIndexAtCurrentDepth + siblingIndex] as Text;
+          const textNode = getLogicalChild(parent, childIndexAtCurrentDepth + siblingIndex);
-          domTextNode.textContent = renderTreeFrame.textContent(frame);
+          if (textNode instanceof Text) {
+            textNode.textContent = renderTreeFrame.textContent(frame);
+          } else {
+            throw new Error(`Cannot set text content on non-text child`);
+          }
           break;
         }
         case EditType.stepIn: {
           const siblingIndex = renderTreeEdit.siblingIndex(edit);
-          parent = parent.childNodes[childIndexAtCurrentDepth + siblingIndex] as HTMLElement;
+          parent = getLogicalChild(parent, childIndexAtCurrentDepth + siblingIndex);
           currentDepth++;
           childIndexAtCurrentDepth = 0;
           break;
         }
         case EditType.stepOut: {
-          parent = parent.parentElement!;
+          parent = getLogicalParent(parent)!;
           currentDepth--;
           childIndexAtCurrentDepth = currentDepth === 0 ? childIndex : 0; // The childIndex is only ever nonzero at zero depth
           break;
@@ -107,7 +122,7 @@ export class BrowserRenderer {
     }
   }
 
-  private insertFrame(componentId: number, parent: Element, childIndex: number, frames: System_Array<RenderTreeFramePointer>, frame: RenderTreeFramePointer, frameIndex: number): number {
+  private insertFrame(componentId: number, parent: LogicalElement, childIndex: number, frames: System_Array<RenderTreeFramePointer>, frame: RenderTreeFramePointer, frameIndex: number): number {
     const frameType = renderTreeFrame.frameType(frame);
     switch (frameType) {
       case FrameType.element:
@@ -129,48 +144,31 @@ export class BrowserRenderer {
     }
   }
 
-  private insertElement(componentId: number, parent: Element, childIndex: number, frames: System_Array<RenderTreeFramePointer>, frame: RenderTreeFramePointer, frameIndex: number) {
+  private insertElement(componentId: number, parent: LogicalElement, childIndex: number, frames: System_Array<RenderTreeFramePointer>, frame: RenderTreeFramePointer, frameIndex: number) {
     const tagName = renderTreeFrame.elementName(frame)!;
-    const newDomElement = tagName === 'svg' || parent.namespaceURI === 'http://www.w3.org/2000/svg' ?
+    const newDomElementRaw = tagName === 'svg' || isSvgElement(parent) ?
       document.createElementNS('http://www.w3.org/2000/svg', tagName) :
       document.createElement(tagName);
-    insertNodeIntoDOM(newDomElement, parent, childIndex);
+    const newElement = toLogicalElement(newDomElementRaw);
+    insertLogicalChild(newDomElementRaw, parent, childIndex);
 
     // Apply attributes
     const descendantsEndIndexExcl = frameIndex + renderTreeFrame.subtreeLength(frame);
     for (let descendantIndex = frameIndex + 1; descendantIndex < descendantsEndIndexExcl; descendantIndex++) {
       const descendantFrame = getTreeFramePtr(frames, descendantIndex);
       if (renderTreeFrame.frameType(descendantFrame) === FrameType.attribute) {
-        this.applyAttribute(componentId, newDomElement, descendantFrame);
+        this.applyAttribute(componentId, newDomElementRaw, descendantFrame);
       } else {
         // As soon as we see a non-attribute child, all the subsequent child frames are
         // not attributes, so bail out and insert the remnants recursively
-        this.insertFrameRange(componentId, newDomElement, 0, frames, descendantIndex, descendantsEndIndexExcl);
+        this.insertFrameRange(componentId, newElement, 0, frames, descendantIndex, descendantsEndIndexExcl);
         break;
       }
     }
   }
 
-  private insertComponent(parent: Element, childIndex: number, frame: RenderTreeFramePointer) {
+  private insertComponent(parent: LogicalElement, childIndex: number, frame: RenderTreeFramePointer) {
-    // Currently, to support O(1) lookups from render tree frames to DOM nodes, we rely on
+    const containerElement = createAndInsertLogicalContainer(parent, childIndex);
-    // each child component existing as a single top-level element in the DOM. To guarantee
-    // that, we wrap child components in these 'blazor-component' wrappers.
-    // To improve on this in the future:
-    // - If we can statically detect that a given component always produces a single top-level
-    //   element anyway, then don't wrap it in a further nonstandard element
-    // - If we really want to support child components producing multiple top-level frames and
-    //   not being wrapped in a container at all, then every time a component is refreshed in
-    //   the DOM, we could update an array on the parent element that specifies how many DOM
-    //   nodes correspond to each of its render tree frames. Then when that parent wants to
-    //   locate the first DOM node for a render tree frame, it can sum all the frame counts for
-    //   all the preceding render trees frames. It's O(N), but where N is the number of siblings
-    //   (counting child components as a single item), so N will rarely if ever be large.
-    //   We could even keep track of whether all the child components happen to have exactly 1
-    //   top level frames, and in that case, there's no need to sum as we can do direct lookups.
-    const containerElement = parent.namespaceURI === 'http://www.w3.org/2000/svg' ?
-      document.createElementNS('http://www.w3.org/2000/svg', 'g') :
-      document.createElement('blazor-component');
-    insertNodeIntoDOM(containerElement, parent, childIndex);
 
     // All we have to do is associate the child component ID with its location. We don't actually
     // do any rendering here, because the diff for the child will appear later in the render batch.
@@ -178,10 +176,10 @@ export class BrowserRenderer {
     this.attachComponentToElement(childComponentId, containerElement);
   }
 
-  private insertText(parent: Element, childIndex: number, textFrame: RenderTreeFramePointer) {
+  private insertText(parent: LogicalElement, childIndex: number, textFrame: RenderTreeFramePointer) {
     const textContent = renderTreeFrame.textContent(textFrame)!;
-    const newDomTextNode = document.createTextNode(textContent);
+    const newTextNode = document.createTextNode(textContent);
-    insertNodeIntoDOM(newDomTextNode, parent, childIndex);
+    insertLogicalChild(newTextNode, parent, childIndex);
   }
 
   private applyAttribute(componentId: number, toDomElement: Element, attributeFrame: RenderTreeFramePointer) {
@@ -246,7 +244,7 @@ export class BrowserRenderer {
     }
   }
 
-  private insertFrameRange(componentId: number, parent: Element, childIndex: number, frames: System_Array<RenderTreeFramePointer>, startIndex: number, endIndexExcl: number): number {
+  private insertFrameRange(componentId: number, parent: LogicalElement, childIndex: number, frames: System_Array<RenderTreeFramePointer>, startIndex: number, endIndexExcl: number): number {
     const origChildIndex = childIndex;
     for (let index = startIndex; index < endIndexExcl; index++) {
       const frame = getTreeFramePtr(frames, index);
@@ -268,23 +266,6 @@ function isCheckbox(element: Element) {
   return element.tagName === 'INPUT' && element.getAttribute('type') === 'checkbox';
 }
 
-function insertNodeIntoDOM(node: Node, parent: Element, childIndex: number) {
-  if (childIndex >= parent.childNodes.length) {
-    parent.appendChild(node);
-  } else {
-    parent.insertBefore(node, parent.childNodes[childIndex]);
-  }
-}
-
-function removeNodeFromDOM(parent: Element, childIndex: number) {
-  parent.removeChild(parent.childNodes[childIndex]);
-}
-
-function removeAttributeFromDOM(parent: Element, childIndex: number, attributeName: string) {
-  const element = parent.childNodes[childIndex] as Element;
-  element.removeAttribute(attributeName);
-}
-
 function raiseEvent(event: Event, browserRendererId: number, componentId: number, eventHandlerId: number, eventArgs: EventForDotNet<UIEventArgs>) {
   event.preventDefault();
 

src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/LogicalElements.ts

@@ -0,0 +1,148 @@
+/*
+  A LogicalElement plays the same role as an Element instance from the point of view of the
+  API consumer. Inserting and removing logical elements updates the browser DOM just the same.
+
+  The difference is that, unlike regular DOM mutation APIs, the LogicalElement APIs don't use
+  the underlying DOM structure as the data storage for the element hierarchy. Instead, the
+  LogicalElement APIs take care of tracking hierarchical relationships separately. The point
+  of this is to permit a logical tree structure in which parent/child relationships don't
+  have to be materialized in terms of DOM element parent/child relationships. And the reason
+  why we want that is so that hierarchies of Blazor components can be tracked even when those
+  components' render output need not be a single literal DOM element.
+
+  Consumers of the API don't need to know about the implementation, but how it's done is:
+  - Each LogicalElement is materialized in the DOM as either:
+    - A Node instance, for actual Node instances inserted using 'insertLogicalChild' or
+      for Element instances promoted to LogicalElement via 'toLogicalElement'
+    - A Comment instance, for 'logical container' instances inserted using 'createAndInsertLogicalContainer'
+  - Then, on that instance (i.e., the Node or Comment), we store an array of 'logical children'
+    instances, e.g.,
+      [firstChild, secondChild, thirdChild, ...]
+    ... plus we store a reference to the 'logical parent' (if any)
+  - The 'logical children' array means we can look up in O(1):
+    - The number of logical children (not currently implemented because not required, but trivial)
+    - The logical child at any given index
+  - Whenever a logical child is added or removed, we update the parent's array of logical children
+*/
+
+const logicalChildrenPropname = createSymbolOrFallback('_blazorLogicalChildren');
+const logicalParentPropname = createSymbolOrFallback('_blazorLogicalParent');
+
+export function toLogicalElement(element: Element) {
+  if (element.childNodes.length > 0) {
+    throw new Error('New logical elements must start empty');
+  }
+
+  element[logicalChildrenPropname] = [];
+  return element as any as LogicalElement;
+}
+
+export function createAndInsertLogicalContainer(parent: LogicalElement, childIndex: number): LogicalElement {
+  const containerElement = document.createComment('!');
+  insertLogicalChild(containerElement, parent, childIndex);
+  return containerElement as any as LogicalElement;
+}
+
+export function insertLogicalChild(child: Node, parent: LogicalElement, childIndex: number) {
+  const childAsLogicalElement = child as any as LogicalElement;
+  if (child instanceof Comment) {
+    const existingGrandchildren = getLogicalChildrenArray(childAsLogicalElement);
+    if (existingGrandchildren && getLogicalChildrenArray(childAsLogicalElement).length > 0) {
+      // There's nothing to stop us implementing support for this scenario, and it's not difficult
+      // (after inserting 'child' itself, also iterate through its logical children and physically
+      // put them as following-siblings in the DOM). However there's no scenario that requires it
+      // presently, so if we did implement it there'd be no good way to have tests for it.
+      throw new Error('Not implemented: inserting non-empty logical container');
+    }
+  }
+
+  if (getLogicalParent(childAsLogicalElement)) {
+    // Likewise, we could easily support this scenario too (in this 'if' block, just splice
+    // out 'child' from the logical children array of its previous logical parent by using
+    // Array.prototype.indexOf to determine its previous sibling index).
+    // But again, since there's not currently any scenario that would use it, we would not
+    // have any test coverage for such an implementation.
+    throw new Error('Not implemented: moving existing logical children');
+  }
+
+  const newSiblings = getLogicalChildrenArray(parent);
+  const newPhysicalParent = getClosestDomElement(parent);
+  if (childIndex < newSiblings.length) {
+    newPhysicalParent.insertBefore(child, newSiblings[childIndex] as any as Node);
+    newSiblings.splice(childIndex, 0, childAsLogicalElement);
+  } else {
+    if (parent instanceof Comment) {
+      const parentLogicalNextSibling = getLogicalNextSibling(parent);
+      if (parentLogicalNextSibling) {
+        newPhysicalParent.insertBefore(child, parentLogicalNextSibling as any as Node);
+      } else {
+        newPhysicalParent.appendChild(child);
+      }
+    } else {
+      newPhysicalParent.appendChild(child);
+    }
+
+    newSiblings.push(childAsLogicalElement);
+  }
+
+  childAsLogicalElement[logicalParentPropname] = parent;
+  if (!(logicalChildrenPropname in childAsLogicalElement)) {
+    childAsLogicalElement[logicalChildrenPropname] = [];
+  }
+}
+
+export function removeLogicalChild(parent: LogicalElement, childIndex: number) {
+  const childrenArray = getLogicalChildrenArray(parent);
+  const childToRemove = childrenArray.splice(childIndex, 1)[0];
+
+  // If it's a logical container, also remove its descendants
+  if (childToRemove instanceof Comment) {
+    const grandchildrenArray = getLogicalChildrenArray(childToRemove);
+    while (grandchildrenArray.length > 0) {
+      removeLogicalChild(childToRemove, 0);
+    }
+  }
+
+  // Finally, remove the node itself
+  const domNodeToRemove = childToRemove as any as Node;
+  domNodeToRemove.parentNode!.removeChild(domNodeToRemove);
+}
+
+export function getLogicalParent(element: LogicalElement): LogicalElement | null {
+  return (element[logicalParentPropname] as LogicalElement) || null;
+}
+
+export function getLogicalChild(parent: LogicalElement, childIndex: number): LogicalElement {
+  return getLogicalChildrenArray(parent)[childIndex];
+}
+
+export function isSvgElement(element: LogicalElement) {
+  return getClosestDomElement(element).namespaceURI === 'http://www.w3.org/2000/svg';
+}
+
+function getLogicalChildrenArray(element: LogicalElement) {
+  return element[logicalChildrenPropname] as LogicalElement[];
+}
+
+function getLogicalNextSibling(element: LogicalElement): LogicalElement | null {
+  const siblings = getLogicalChildrenArray(getLogicalParent(element)!);
+  const siblingIndex = Array.prototype.indexOf.call(siblings, element);
+  return siblings[siblingIndex + 1] || null;
+}
+
+function getClosestDomElement(logicalElement: LogicalElement) {
+  if (logicalElement instanceof Element) {
+    return logicalElement;
+  } else if (logicalElement instanceof Comment) {
+    return logicalElement.parentNode! as Element;
+  } else {
+    throw new Error('Not a valid logical element');
+  }
+}
+
+function createSymbolOrFallback(fallback: string): symbol | string {
+  return typeof Symbol === 'function' ? Symbol() : fallback;
+}
+
+// Nominal type to represent a logical element without needing to allocate any object for instances
+export interface LogicalElement { LogicalElement__DO_NOT_IMPLEMENT: any };

src/Microsoft.AspNetCore.Blazor.Browser.JS/src/Rendering/Renderer.ts

@@ -17,8 +17,8 @@ export function attachRootComponentToElement(browserRendererId: number, elementS
   if (!browserRenderer) {
     browserRenderer = browserRenderers[browserRendererId] = new BrowserRenderer(browserRendererId);
   }
-  browserRenderer.attachRootComponentToElement(componentId, element);
   clearElement(element);
+  browserRenderer.attachRootComponentToElement(componentId, element);
 }
 
 export function renderBatch(browserRendererId: number, batch: RenderBatchPointer) {

test/Microsoft.AspNetCore.Blazor.E2ETest/Tests/ComponentRenderingTest.cs

@@ -116,12 +116,7 @@ namespace Microsoft.AspNetCore.Blazor.E2ETest.Tests
             Assert.Equal("Parent component",
                 appElement.FindElement(By.CssSelector("fieldset > legend")).Text);
 
-            // TODO: Once we remove the wrapper elements from around child components,
+            var styledElement = appElement.FindElement(By.CssSelector("fieldset > h1"));
-            // assert that the child component text node is directly inside the <fieldset>
-            var childComponentWrapper = appElement.FindElement(By.CssSelector("fieldset > blazor-component"));
-            Assert.Single(childComponentWrapper.FindElements(By.CssSelector("*")));
-
-            var styledElement = childComponentWrapper.FindElement(By.TagName("h1"));
             Assert.Equal("Hello, world!", styledElement.Text);
             Assert.Equal("color: red;", styledElement.GetAttribute("style"));
             Assert.Equal("somevalue", styledElement.GetAttribute("customattribute"));
@@ -131,13 +126,13 @@ namespace Microsoft.AspNetCore.Blazor.E2ETest.Tests
         public void CanTriggerEventsOnChildComponents()
         {
             // Counter is displayed as child component. Initial count is zero.
-            var childComponentWrapper = MountTestComponent<CounterComponentWrapper>()
+            var appElement = MountTestComponent<CounterComponentWrapper>();
-                .FindElements(By.CssSelector("blazor-component")).Single();
+            var counterDisplay = appElement
-            var counterDisplay = childComponentWrapper.FindElement(By.TagName("p"));
+                .FindElements(By.TagName("p"))
-            Assert.Equal("Current count: 0", counterDisplay.Text);
+                .Single(element => element.Text == "Current count: 0");
 
             // Clicking increments count in child component
-            childComponentWrapper.FindElement(By.TagName("button")).Click();
+            appElement.FindElement(By.TagName("button")).Click();
             Assert.Equal("Current count: 1", counterDisplay.Text);
         }