Fix firenvim on pages that remove its spans

Some pages (e.g. Jira) remove the span when they're created. This
resulted in two issues:
- Either the Firenvim element was never initialized (because its frame
  didn't have enough time to register with the background script and
  propagate its frame id). This resulted in the frameIdPromise timeout
  being triggered. This is fixed by re-appending the frame to the page
  until it gets a frameId or the span has been re-appended X times and
  firenvim gives up and logs an error.
- Or the Firenvim element was initialized and when the corresponding
  textarea was focused, the Firenvim element that didn't exist in the
  page got focused instead of creating a new firenvim element. This is
  fixed by killing the firenvim element (because its websocket is broken
  when the iframe disappears from the page) that doesn't exist in the
  page and creating a new one.

Should help with glacambre#323

src/FirenvimElement.ts

@@ -27,7 +27,11 @@ export class FirenvimElement {
     // We use a mutation observer to detect whether the element is removed from
     // the page. When this happens, the FirenvimElement is removed from the
     // page.
-    private mutationObserver: MutationObserver;
+    private pageObserver: MutationObserver;
+    // We use a mutation observer to detect if the span is removed from the
+    // page by the page. When this happens, the span is re-inserted in the
+    // page.
+    private spanObserver: MutationObserver;
     // nvimify is the function that listens for focus events and creates
     // firenvim elements. We need it in order to be able to remove it as an
     // event listener from the element the user selected when the user wants to
@@ -108,7 +112,16 @@ export class FirenvimElement {
     }
 
     attachToPage (fip: Promise<number>) {
-        this.frameIdPromise = fip.then((f: number) => this.frameId = f);
+        this.frameIdPromise = fip.then((f: number) => {
+            this.frameId = f;
+            // Once a frameId has been acquired, the FirenvimElement would die
+            // if its span was removed from the page. Thus there is no use in
+            // keeping its spanObserver around. It'd even cause issues as the
+            // spanObserver would attempt to re-insert a dead frame in the
+            // page.
+            this.spanObserver.disconnect();
+            return this.frameId;
+        });
 
         // We don't need the iframe to be appended to the page in order to
         // resize it because we're just using the corresponding
@@ -152,6 +165,32 @@ export class FirenvimElement {
 
         this.iframe.src = (browser as any).extension.getURL("/NeovimFrame.html");
         this.span.attachShadow({ mode: "closed" }).appendChild(this.iframe);
+
+        // So pages (e.g. Jira, Confluence) remove spans from the page as soon
+        // as they're inserted. We don't wan't that, so for the 5 seconds
+        // following the insertion, detect if the span is removed from the page
+        // by checking visibility changes and re-insert if needed.
+        let reinserts = 0;
+        this.spanObserver = new MutationObserver(
+            (self => (mutations : MutationRecord[], observer: MutationObserver) => {
+            const span = self.getSpan();
+            for (const mutation of mutations) {
+                for (const node of mutation.removedNodes) {
+                    if (node === span) {
+                        reinserts += 1;
+                        if (reinserts >= 10) {
+                            console.error("Firenvim is trying to create an iframe on this site but the page is constantly removing it. Consider disabling Firenvim on this website.");
+                            observer.disconnect();
+                        } else {
+                            setTimeout(() => self.getElement().ownerDocument.body.appendChild(span), reinserts * 100);
+                        }
+                        return;
+                    }
+                }
+            }
+        })(this));
+        this.spanObserver.observe(this.getElement().ownerDocument.body, { childList: true });
+
         this.getElement().ownerDocument.body.appendChild(this.span);
 
         this.focus();
@@ -174,7 +213,7 @@ export class FirenvimElement {
         // We want to remove the FirenvimElement from the page when the
         // corresponding element is removed. We do this by adding a
         // mutationObserver to its parent.
-        this.mutationObserver = new MutationObserver((self => (mutations: MutationRecord[]) => {
+        this.pageObserver = new MutationObserver((self => (mutations: MutationRecord[]) => {
             const elem = self.getElement();
             mutations.forEach(mutation => mutation.removedNodes.forEach(node => {
                 const walker = document.createTreeWalker(node, NodeFilter.SHOW_ALL);
@@ -185,7 +224,7 @@ export class FirenvimElement {
                 }
             }));
         })(this));
-        this.mutationObserver.observe(document.documentElement, {
+        this.pageObserver.observe(document.documentElement, {
             subtree: true,
             childList: true
         });
@@ -195,8 +234,9 @@ export class FirenvimElement {
         const elem = this.getElement();
         this.resizeObserver.unobserve(elem);
         this.intersectionObserver.unobserve(elem);
-        this.mutationObserver.disconnect();
-        this.span.parentNode.removeChild(this.span);
+        this.pageObserver.disconnect();
+        this.spanObserver.disconnect();
+        this.span.remove();
         this.onDetach(this.frameId);
     }
 
@@ -257,6 +297,10 @@ export class FirenvimElement {
         return this.editor.getElement();
     }
 
+    getFrameId () {
+        return this.frameId;
+    }
+
     getIframe () {
         return this.iframe;
     }

src/common.ts

@@ -33,41 +33,6 @@ export const global = {
             await global.disabled;
         }
 
-        // auto is true when nvimify() is called as an event listener, false
-        // when called from forceNvimify()
-        const auto = (evt instanceof FocusEvent);
-
-        const takeover = getConf().takeover;
-        if (global.disabled || (auto && takeover === "never")) {
-            return;
-        }
-
-        const firenvim = new FirenvimElement(
-            evt.target as HTMLElement,
-            global.nvimify,
-            (id: number) => global.firenvimElems.delete(id)
-        );
-        const editor = firenvim.getEditor();
-
-        // If this element already has a neovim frame, stop
-        const alreadyRunning = Array.from(global.firenvimElems.values())
-            .find((instance) => instance.getElement() === editor.getElement());
-        if (alreadyRunning !== undefined) {
-            alreadyRunning.show();
-            alreadyRunning.focus();
-            return;
-        }
-
-        if (auto && (takeover === "empty" || takeover === "nonempty")) {
-            const content = (await editor.getContent()).trim();
-            if ((content !== "" && takeover === "empty")
-                || (content === "" && takeover === "nonempty")) {
-                    return;
-                }
-        }
-
-        firenvim.prepareBufferInfo();
-
         // When creating new frames, we need to know their frameId in order to
         // communicate with them. This can't be retrieved through a
         // synchronous, in-page call so the new frame has to tell the
@@ -82,23 +47,74 @@ export const global = {
             lock = frameIdLock;
             await frameIdLock;
         }
+
         frameIdLock = new Promise(async (unlock: any) => {
-            // TODO: make this timeout the same as the one in background.ts
+            // auto is true when nvimify() is called as an event listener, false
+            // when called from forceNvimify()
+            const auto = (evt instanceof FocusEvent);
+
+            const takeover = getConf().takeover;
+            if (global.disabled || (auto && takeover === "never")) {
+                unlock();
+                return;
+            }
+
+            const firenvim = new FirenvimElement(
+                evt.target as HTMLElement,
+                global.nvimify,
+                (id: number) => global.firenvimElems.delete(id)
+            );
+            const editor = firenvim.getEditor();
+
+            // If this element already has a neovim frame, stop
+            const alreadyRunning = Array.from(global.firenvimElems.values())
+                .find((instance) => instance.getElement() === editor.getElement());
+            if (alreadyRunning !== undefined) {
+                // The span might have been removed from the page by the page
+                // (this happens on Jira/Confluence for example) so we check
+                // for that.
+                const span = alreadyRunning.getSpan();
+                if (span.ownerDocument.contains(span)) {
+                    alreadyRunning.show();
+                    alreadyRunning.focus();
+                    unlock();
+                    return;
+                } else {
+                    // If the span has been removed from the page, the editor
+                    // is dead because removing an iframe from the page kills
+                    // the websocket connection inside of it.
+                    // We just tell the editor to clean itself up and go on as
+                    // if it didn't exist.
+                    alreadyRunning.detachFromPage();
+                }
+            }
+
+            if (auto && (takeover === "empty" || takeover === "nonempty")) {
+                const content = (await editor.getContent()).trim();
+                if ((content !== "" && takeover === "empty")
+                    || (content === "" && takeover === "nonempty")) {
+                        unlock();
+                        return;
+                    }
+            }
+
+            firenvim.prepareBufferInfo();
             const frameIdPromise = new Promise((resolve: (_: number) => void, reject) => {
-                global.frameIdResolve = (frameId: number) => {
-                    global.firenvimElems.set(frameId, firenvim);
-                    global.frameIdResolve = () => undefined;
-                    resolve(frameId);
-                };
+                global.frameIdResolve = resolve;
+                // TODO: make this timeout the same as the one in background.ts
                 setTimeout(reject, 10000);
             });
+            frameIdPromise.then((frameId: number) => {
+                global.firenvimElems.set(frameId, firenvim);
+                global.frameIdResolve = () => undefined;
+                unlock();
+            });
+            frameIdPromise.catch(unlock);
             firenvim.attachToPage(frameIdPromise);
-            frameIdPromise
-                .then(unlock)
-                .catch(unlock);
         });
     },
 
+    // fienvimElems maps frame ids to firenvim elements.
     firenvimElems: new Map<number, FirenvimElement>(),
 };
 

tests/_common.ts

@@ -844,6 +844,33 @@ ${vimrcContent}`);
         expect((await input.getAttribute("innerHTML")).trim()).toBe("<i>Firenvim</i> <i>works</i>!");
 }
 
+export async function testDisappearing(driver: webdriver.WebDriver) {
+        await loadLocalPage(driver, "disappearing.html", "Modifier test");
+        const input = await driver.wait(Until.elementLocated(By.id("content-input")), 5000, "input not found");
+        await driver.executeScript("arguments[0].scrollIntoView(true);", input);
+        await driver.actions().click(input).perform();
+        let ready = firenvimReady(driver);
+        // Page will remove the span and firenvim should re-add it
+        let span = await driver.wait(Until.elementLocated(By.css("body > span:nth-child(2)")), 5000, "Firenvim span not found");
+        await ready;
+        // simulate the page making the span disappear again
+        await driver.executeScript("document.querySelector('span').remove()");
+        await driver.wait(Until.stalenessOf(span), 5000, "Firenvim span did not disappear");
+        await driver.actions().click(input).perform();
+        span = await driver.wait(Until.elementLocated(By.css("body > span:nth-child(2)")), 5000, "Firenvim span not found the second time");
+        await ready;
+        // somehow ready is too fast here, so we need an additional delay
+        await driver.sleep(FIRENVIM_INIT_DELAY);
+        await sendKeys(driver, "iworks".split("")
+                       .concat([webdriver.Key.ESCAPE])
+                       .concat(":wq!".split(""))
+                       .concat(webdriver.Key.ENTER))
+        await driver.wait(Until.stalenessOf(span), 5000, "Firenvim span did not disappear the second time");
+        await driver.wait(async () => (await input.getAttribute("value") !== ""), 5000, "Input value did not change");
+        expect("works").toBe(await input.getAttribute("value"));
+}
+
+
 export async function killDriver(driver: webdriver.WebDriver) {
         try {
                 await driver.close()

tests/chrome.ts

@@ -12,6 +12,7 @@ import {
  testEvalJs,
  testCodemirror,
  testContentEditable,
+ testDisappearing,
  testDynamicTextareas,
  testFocusGainedLost,
  testGStartedByFirenvim,
@@ -100,6 +101,7 @@ describe("Chrome", () => {
         nonHeadlessTest()("Firenvim works on CodeMirror", () => testCodemirror(driver));
         nonHeadlessTest()("Firenvim works on ContentEditable", () => testContentEditable(driver));
         nonHeadlessTest()("Firenvim works on Monaco", () => testMonaco(driver));
+        nonHeadlessTest()("Firenvim works when span disappears", () => testDisappearing(driver));
         nonHeadlessTest()("Firenvim works on dynamically created elements", () => testDynamicTextareas(driver));
         nonHeadlessTest()("Firenvim works on dynamically created nested elements", () => testNestedDynamicTextareas(driver));
         nonHeadlessTest()("Firenvim works with large buffers", () => testLargeBuffers(driver));

tests/firefox.ts

@@ -15,6 +15,7 @@ import {
  testAce,
  testCodemirror,
  testContentEditable,
+ testDisappearing,
  testDynamicTextareas,
  testEvalJs,
  testFocusGainedLost,
@@ -92,6 +93,7 @@ describe("Firefox", () => {
         test("Firenvim works on CodeMirror", () => testCodemirror(driver));
         test("Firenvim works with ContentEditatble", () => testContentEditable(driver));
         test("Firenvim works on Monaco", () => testMonaco(driver));
+        test("Firenvim works when span disappears", () => testDisappearing(driver));
         test("Firenvim works on dynamically created elements", () => testDynamicTextareas(driver));
         test("Firenvim works on dynamically created nested elements", () => testNestedDynamicTextareas(driver));
         test("Firenvim works with large buffers", () => testLargeBuffers(driver));

tests/pages/disappearing.html

@@ -0,0 +1,28 @@
+<!doctype html>
+<html lang="en" id="html">
+        <head>
+                <meta charset="utf-8">
+                <meta name="viewport" content="width=device-width, initial-scale=1">
+                <title>Test Page</title>
+                <script>
+window.addEventListener("load", () => {
+  const observer = new MutationObserver((mutations, observer) => {
+    let removed = false;
+    for (const mutation of mutations) {
+      for (let node of mutation.addedNodes) {
+        node.remove();
+        removed = true;
+      }
+    }
+    if (removed) {
+      observer.disconnect();
+    }
+  });
+  observer.observe(document.body, { childList: true, subtree: true });
+});
+                </script>
+        </head>
+        <body id="body">
+                <textarea id="content-input" rows="20" cols="80"></textarea>
+        </body>
+</html>