Merge pull request #856 from glimmerjs/fix-bounds

`Bounds` must be non-empty

packages/@glimmer/interfaces/lib/dom/simple.d.ts

@@ -83,4 +83,5 @@ export interface Element extends Node {
   removeAttributeNS(namespaceURI: string, name: string): void;
   setAttribute(name: string, value: string): void;
   setAttributeNS(namespaceURI: string, qualifiedName: string, value: string): void;
+  insertAdjacentHTML(position: 'beforebegin' | 'beforeend', html: string): void;
 }

packages/@glimmer/node/lib/node-dom-helper.ts

@@ -1,6 +1,7 @@
 import * as SimpleDOM from 'simple-dom';
 import { DOMTreeConstruction, Bounds, ConcreteBounds } from '@glimmer/runtime';
 import { Simple } from '@glimmer/interfaces';
+import { Option } from '@glimmer/util';
 
 export default class NodeDOMTreeConstruction extends DOMTreeConstruction {
   protected document!: SimpleDOM.Document; // Hides property on base class
@@ -11,16 +12,10 @@ export default class NodeDOMTreeConstruction extends DOMTreeConstruction {
   // override to prevent usage of `this.document` until after the constructor
   protected setupUselessElement() {}
 
-  insertHTMLBefore(parent: Simple.Element, reference: Simple.Node, html: string): Bounds {
-    let prev = reference ? reference.previousSibling : parent.lastChild;
-
+  insertHTMLBefore(parent: Simple.Element, reference: Option<Simple.Node>, html: string): Bounds {
     let raw = this.document.createRawHTMLSection(html);
     parent.insertBefore(raw, reference);
-
-    let first = prev ? prev.nextSibling : parent.firstChild;
-    let last = reference ? reference.previousSibling : parent.lastChild;
-
-    return new ConcreteBounds(parent, first, last);
+    return new ConcreteBounds(parent, raw, raw);
   }
 
   // override to avoid SVG detection/work when in node (this is not needed in SSR)

packages/@glimmer/runtime/index.ts

@@ -77,7 +77,6 @@ export {
   DOMChanges as IDOMChanges,
   DOMTreeConstruction,
   isWhitespace,
-  insertHTMLBefore,
 } from './lib/dom/helper';
 export { normalizeProperty } from './lib/dom/props';
 export {

packages/@glimmer/runtime/lib/bounds.ts

@@ -1,11 +1,11 @@
 import { Simple } from '@glimmer/interfaces';
-import { Option, Destroyable } from '@glimmer/util';
+import { Option, Destroyable, expect } from '@glimmer/util';
 
 export interface Bounds {
   // a method to future-proof for wormholing; may not be needed ultimately
   parentElement(): Simple.Element;
-  firstNode(): Option<Simple.Node>;
-  lastNode(): Option<Simple.Node>;
+  firstNode(): Simple.Node;
+  lastNode(): Simple.Node;
 }
 
 export class Cursor {
@@ -19,77 +19,75 @@ export interface DestroyableBounds extends Bounds, Destroyable {}
 export class ConcreteBounds implements Bounds {
   constructor(
     public parentNode: Simple.Element,
-    private first: Option<Simple.Node>,
-    private last: Option<Simple.Node>
+    private first: Simple.Node,
+    private last: Simple.Node
   ) {}
 
-  parentElement() {
+  parentElement(): Simple.Element {
     return this.parentNode;
   }
-  firstNode() {
+
+  firstNode(): Simple.Node {
     return this.first;
   }
-  lastNode() {
+
+  lastNode(): Simple.Node {
     return this.last;
   }
 }
 
 export class SingleNodeBounds implements Bounds {
   constructor(private parentNode: Simple.Element, private node: Simple.Node) {}
 
-  parentElement() {
+  parentElement(): Simple.Element {
     return this.parentNode;
   }
-  firstNode() {
+
+  firstNode(): Simple.Node {
     return this.node;
   }
-  lastNode() {
+
+  lastNode(): Simple.Node {
     return this.node;
   }
 }
 
-export function bounds(
-  parent: Simple.Element,
-  first: Simple.Node,
-  last: Simple.Node
-): ConcreteBounds {
-  return new ConcreteBounds(parent, first, last);
-}
-
-export function single(parent: Simple.Element, node: Simple.Node): SingleNodeBounds {
-  return new SingleNodeBounds(parent, node);
-}
-
-export function move(bounds: Bounds, reference: Option<Simple.Node>) {
+export function move(bounds: Bounds, reference: Option<Simple.Node>): Option<Simple.Node> {
   let parent = bounds.parentElement();
   let first = bounds.firstNode();
   let last = bounds.lastNode();
 
-  let node: Option<Simple.Node> = first;
+  let current: Simple.Node = first;
 
-  while (node) {
-    let next = node.nextSibling;
-    parent.insertBefore(node, reference);
-    if (node === last) return next;
-    node = next;
-  }
+  while (true) {
+    let next = current.nextSibling;
+
+    parent.insertBefore(current, reference);
 
-  return null;
+    if (current === last) {
+      return next;
+    }
+
+    current = expect(next, 'invalid bounds');
+  }
 }
 
 export function clear(bounds: Bounds): Option<Simple.Node> {
   let parent = bounds.parentElement();
   let first = bounds.firstNode();
   let last = bounds.lastNode();
 
-  let node: Option<Simple.Node> = first;
+  let current: Simple.Node = first;
 
-  while (node) {
-    let next = node.nextSibling;
-    parent.removeChild(node);
-    if (node === last) return next;
-    node = next;
-  }
+  while (true) {
+    let next = current.nextSibling;
+
+    parent.removeChild(current);
 
-  return null;
+    if (current === last) {
+      return next;
+    }
+
+    current = expect(next, 'invalid bounds');
+  }
 }

packages/@glimmer/runtime/lib/compat/svg-inner-html-fix.ts

@@ -1,6 +1,7 @@
-import { Bounds, ConcreteBounds } from '../bounds';
+import { Bounds } from '../bounds';
 import { moveNodesBefore, DOMOperations } from '../dom/helper';
-import { Option, unwrap } from '@glimmer/util';
+import { Option, unwrap, assert } from '@glimmer/util';
+import { Simple } from '@glimmer/interfaces';
 
 export const SVG_NAMESPACE = 'http://www.w3.org/2000/svg';
 export type SVG_NAMESPACE = typeof SVG_NAMESPACE;
@@ -30,7 +31,15 @@ export function applySVGInnerHTMLFix(
   let div = document.createElement('div');
 
   return class DOMChangesWithSVGInnerHTMLFix extends DOMClass {
-    insertHTMLBefore(parent: HTMLElement, nextSibling: Node, html: string): Bounds {
+    insertHTMLBefore(
+      parent: Simple.Element,
+      nextSibling: Option<Simple.Node>,
+      html: string
+    ): Bounds {
+      if (html === '') {
+        return super.insertHTMLBefore(parent, nextSibling, html);
+      }
+
       if (parent.namespaceURI !== svgNamespace) {
         return super.insertHTMLBefore(parent, nextSibling, html);
       }
@@ -40,31 +49,37 @@ export function applySVGInnerHTMLFix(
   };
 }
 
-function fixSVG(parent: Element, div: HTMLElement, html: string, reference: Node): Bounds {
+function fixSVG(
+  parent: Simple.Element,
+  div: HTMLElement,
+  html: string,
+  reference: Option<Simple.Node>
+): Bounds {
+  assert(html !== '', 'html cannot be empty');
+
   let source: Node;
 
   // This is important, because decendants of the <foreignObject> integration
   // point are parsed in the HTML namespace
   if (parent.tagName.toUpperCase() === 'FOREIGNOBJECT') {
     // IE, Edge: also do not correctly support using `innerHTML` on SVG
     // namespaced elements. So here a wrapper is used.
-    let wrappedHtml = '<svg><foreignObject>' + (html || '<!---->') + '</foreignObject></svg>';
+    let wrappedHtml = '<svg><foreignObject>' + html + '</foreignObject></svg>';
 
     div.innerHTML = wrappedHtml;
 
     source = div.firstChild!.firstChild!;
   } else {
     // IE, Edge: also do not correctly support using `innerHTML` on SVG
     // namespaced elements. So here a wrapper is used.
-    let wrappedHtml = '<svg>' + (html || '<!---->') + '</svg>';
+    let wrappedHtml = '<svg>' + html + '</svg>';
 
     div.innerHTML = wrappedHtml;
 
     source = div.firstChild!;
   }
 
-  let [first, last] = moveNodesBefore(source, parent, reference);
-  return new ConcreteBounds(parent, first, last);
+  return moveNodesBefore(source, parent, reference);
 }
 
 function shouldApplyFix(document: Document, svgNamespace: SVG_NAMESPACE) {

packages/@glimmer/runtime/lib/compat/text-node-merging-fix.ts

@@ -1,6 +1,7 @@
 import { Bounds } from '../bounds';
 import { DOMOperations } from '../dom/helper';
 import { Option } from '@glimmer/util';
+import { Simple } from '@glimmer/interfaces';
 
 // Patch:    Adjacent text node merging fix
 // Browsers: IE, Edge, Firefox w/o inspector open
@@ -32,10 +33,19 @@ export function applyTextNodeMergingFix(
       this.uselessComment = document.createComment('');
     }
 
-    insertHTMLBefore(parent: HTMLElement, nextSibling: Node, html: string): Bounds {
+    insertHTMLBefore(
+      parent: Simple.Element,
+      nextSibling: Option<Simple.Node>,
+      html: string
+    ): Bounds {
+      if (html === '') {
+        return super.insertHTMLBefore(parent, nextSibling, html);
+      }
+
       let didSetUselessComment = false;
 
       let nextPrevious = nextSibling ? nextSibling.previousSibling : parent.lastChild;
+
       if (nextPrevious && nextPrevious instanceof Text) {
         didSetUselessComment = true;
         parent.insertBefore(this.uselessComment, nextSibling);