Merge pull request #670 from threema-ch/574-safari-emoji

Fix inserting emoji after a newline in Safari

README.md

@@ -78,6 +78,10 @@ For example:
     npm run test:ui firefox
     npm run test:ui chrome
 
+You can also filter the test cases:
+
+    npm run test:ui firefox emoji
+
 To run linting checks:
 
     npm run lint

src/directives/compose_area.ts

@@ -19,6 +19,7 @@ import {extractText, isActionTrigger, logAdapter} from '../helpers';
 import {BrowserService} from '../services/browser';
 import {StringService} from '../services/string';
 import {TimeoutService} from '../services/timeout';
+import {isElementNode, isTextNode} from '../typeguards';
 
 /**
  * The compose area where messages are written.
@@ -500,18 +501,22 @@ export default [
                     insertEmoji(this.textContent);
                 }
 
-                function insertEmoji(emoji, posFrom = null, posTo = null): void {
+                function insertEmoji(emoji, posFrom?: number, posTo?: number): void {
                     const emojiElement = ($filter('emojify') as any)(emoji, true, true, scope.emojiImagePath) as string;
                     insertHTMLElement(emoji, emojiElement, posFrom, posTo);
                 }
 
-                function insertMention(mentionString, posFrom = null, posTo = null): void {
+                function insertMention(mentionString, posFrom?: number, posTo?: number): void {
                     const mentionElement = ($filter('mentionify') as any)(mentionString) as string;
                     insertHTMLElement(mentionString, mentionElement, posFrom, posTo);
                 }
 
-                function insertHTMLElement(original: string, formatted: string, posFrom = null, posTo = null): void {
-
+                function insertHTMLElement(
+                    original: string,
+                    formatted: string,
+                    posFrom?: number,
+                    posTo?: number,
+                ): void {
                     // In Chrome in right-to-left mode, our content editable
                     // area may contain a DIV element.
                     const childNodes = composeDiv[0].childNodes;
@@ -527,11 +532,11 @@ export default [
 
                     let currentHTML = '';
                     for (let i = 0; i < contentElement.childNodes.length; i++) {
-                        const node = contentElement.childNodes[i];
+                        const node: Node = contentElement.childNodes[i];
 
-                        if (node.nodeType === node.TEXT_NODE) {
+                        if (isTextNode(node)) {
                             currentHTML += node.textContent;
-                        } else if (node.nodeType === node.ELEMENT_NODE) {
+                        } else if (isElementNode(node)) {
                             const tag = node.tagName.toLowerCase();
                             if (tag === 'img' || tag === 'span') {
                                 currentHTML += getOuterHtml(node);
@@ -541,13 +546,23 @@ export default [
                                 if (i < contentElement.childNodes.length - 1) {
                                     currentHTML += getOuterHtml(node);
                                 }
+                            } else if (tag === 'div') {
+                                // Safari inserts a <div><br></div> after editing content editable fields.
+                                // Remove the last instance to fix this.
+                                if (node.childNodes.length === 1
+                                    && isElementNode(node.lastChild)
+                                    && node.lastChild.tagName.toLowerCase() === 'br') {
+                                    // Ignore
+                                } else {
+                                    currentHTML += getOuterHtml(node);
+                                }
                             }
                         }
                     }
 
                     if (caretPosition !== null) {
-                        posFrom = null === posFrom ? caretPosition.from : posFrom;
-                        posTo = null === posTo ? caretPosition.to : posTo;
+                        posFrom = posFrom === undefined ? caretPosition.from : posFrom;
+                        posTo = posTo === undefined ? caretPosition.to : posTo;
                         currentHTML = currentHTML.substr(0, posFrom)
                             + formatted
                             + currentHTML.substr(posTo);

src/helpers.ts

@@ -14,6 +14,8 @@
  * You should have received a copy of the GNU Affero General Public License
  * along with Threema Web. If not, see <http://www.gnu.org/licenses/>.
  */
+// tslint:disable:no-reference
+/// <reference path="threema.d.ts" />
 
 /**
  * Convert an Uint8Array to a hex string.
@@ -400,21 +402,29 @@ export function extractText(targetNode: HTMLElement, logWarning: (msg: string) =
         //
         // Thus, for Safari, we need to detect <div>s and insert a newline.
 
+        let lastNodeType;
         // tslint:disable-next-line: prefer-for-of (see #98)
         for (let i = 0; i < parentNode.childNodes.length; i++) {
             const node = parentNode.childNodes[i] as HTMLElement;
             switch (node.nodeType) {
                 case Node.TEXT_NODE:
+                    lastNodeType = 'text';
                     // Append text, but strip leading and trailing newlines
                     text += node.nodeValue.replace(/(^[\r\n]*|[\r\n]*$)/g, '');
                     break;
                 case Node.ELEMENT_NODE:
                     const tag = node.tagName.toLowerCase();
+                    const _lastNodeType = lastNodeType;
+                    lastNodeType = tag;
                     if (tag === 'div') {
                         text += '\n';
                         visitChildNodes(node);
                         break;
                     } else if (tag === 'img') {
+                        if (_lastNodeType === 'div') {
+                            // An image following a div should go on a new line
+                            text += '\n';
+                        }
                         text += (node as HTMLImageElement).alt;
                         break;
                     } else if (tag === 'br') {

src/typeguards.ts

@@ -77,3 +77,17 @@ export function isValidDisconnectReason(
     }
     return false;
 }
+
+/**
+ * Text nodes type guard.
+ */
+export function isTextNode(node: Node): node is Text {
+    return node.nodeType === node.TEXT_NODE;
+}
+
+/**
+ * Element nodes type guard.
+ */
+export function isElementNode(node: Node): node is HTMLElement {
+    return node.nodeType === node.ELEMENT_NODE;
+}

tests/ui/run.sh

@@ -6,12 +6,9 @@ fi
 
 export PATH=$PATH:"$(pwd)/node_modules/.bin/"
 
-browser=$1
-shift
-
 concurrently \
     --kill-others \
     -s first \
     --names \"server,test\" \
     "npm run testserver" \
-    "ts-node --skip-project -O '{\"target\": \"ES2015\"}' tests/ui/run.ts $browser"
+    "ts-node --skip-project -O '{\"target\": \"ES2015\"}' tests/ui/run.ts $*"

tests/ui/run.ts

@@ -14,10 +14,11 @@ import { expect } from 'chai';
 import { Builder, By, Key, until, WebDriver, WebElement } from 'selenium-webdriver';
 import * as TermColor from 'term-color';
 
-import { extractText } from '../../src/helpers';
+import { extractText as extractTextFunc } from '../../src/helpers';
 
 // Script arguments
 const browser = process.argv[2];
+const filterQuery = process.argv[3];
 
 // Type aliases
 type Testfunc = (driver: WebDriver) => void;
@@ -27,6 +28,18 @@ const composeArea = By.css('div.compose');
 const emojiKeyboard = By.css('.emoji-keyboard');
 const emojiTrigger = By.css('.emoji-trigger');
 
+/**
+ * Helper function to extract text.
+ */
+async function extractText(driver: WebDriver): Promise<string> {
+    const script = `
+        ${extractTextFunc.toString()}
+        const element = document.querySelector("div.compose");
+        return extractText(element);
+    `;
+    return driver.executeScript<string>(script);
+}
+
 /**
  * The emoji trigger should toggle the emoji keyboard.
  */
@@ -91,20 +104,38 @@ async function insertNewline(driver: WebDriver) {
     await driver.findElement(composeArea).sendKeys(Key.SHIFT, Key.ENTER);
     await driver.findElement(composeArea).sendKeys('web');
 
-    const script = `
-        ${extractText.toString()}
-        const element = document.querySelector("div.compose");
-        return extractText(element);
-    `;
-    const text = await driver.executeScript<string>(script);
+    const text = await extractText(driver);
     expect(text).to.equal('hello\nthreema\nweb');
 }
 
+/**
+ * Insert an emoji after some newlines.
+ * Regression test for #574.
+ */
+async function regression574(driver: WebDriver) {
+    // Insert text
+    await driver.findElement(composeArea).click();
+    await driver.findElement(composeArea).sendKeys('hello');
+    await driver.findElement(composeArea).sendKeys(Key.SHIFT, Key.ENTER);
+    await driver.findElement(composeArea).sendKeys('threema');
+    await driver.findElement(composeArea).sendKeys(Key.SHIFT, Key.ENTER);
+    await driver.findElement(composeArea).sendKeys('web');
+    await driver.findElement(composeArea).sendKeys(Key.SHIFT, Key.ENTER);
+
+    // Insert emoji
+    await driver.findElement(emojiTrigger).click();
+    await driver.findElement(By.css('.e1[title=":smile:"]')).click();
+
+    const text = await extractText(driver);
+    expect(text).to.equal('hello\nthreema\nweb\n😄');
+}
+
 // Register tests here
 const TESTS: Array<[string, Testfunc]> = [
     ['Show and hide emoji selector', showEmojiSelector],
     ['Insert emoji and text', insertEmoji],
     ['Insert three lines of text', insertNewline],
+    ['Regression test #574', regression574],
 ];
 
 // Test runner
@@ -114,13 +145,21 @@ const TEST_URL = 'http://localhost:7777/tests/ui/compose_area.html';
     let i = 0;
     let success = 0;
     let failed = 0;
+    let skipped = 0;
     console.info('\n====== THREEMA WEB UI TESTS ======\n');
+    if (filterQuery !== undefined) {
+        console.info(`Filter query: "${filterQuery}"\n`);
+    }
     try {
         for (const [name, testfunc] of TESTS) {
-            console.info(TermColor.blue(`» ${i + 1}: Running test: ${name}`));
-            await driver.get(TEST_URL);
-            await testfunc(driver);
-            success++;
+            if (filterQuery === undefined || name.toLowerCase().indexOf(filterQuery.toLowerCase()) !== -1) {
+                console.info(TermColor.blue(`» ${i + 1}: Running test: ${name}`));
+                await driver.get(TEST_URL);
+                await testfunc(driver);
+                success++;
+            } else {
+                skipped++;
+            }
             i++;
         }
     } catch (e) {
@@ -131,6 +170,6 @@ const TEST_URL = 'http://localhost:7777/tests/ui/compose_area.html';
         await driver.quit();
     }
     const colorFunc = failed > 0 ? TermColor.red : TermColor.green;
-    console.info(colorFunc(`\nSummary: ${i} tests run, ${success} succeeded, ${failed} failed`));
+    console.info(colorFunc(`\nSummary: ${i} tests run, ${success} succeeded, ${failed} failed, ${skipped} skipped`));
     process.exit(failed > 0 ? 1 : 0);
 })();