Incorporate initial feedback

mozilla · Aug 14, 2019 · 210644c · 210644c
1 parent 0b326d9
commit 210644c
Show file tree

Hide file tree

Showing 7 changed files with 71 additions and 104 deletions.
diff --git a/.eslintrc.yml b/.eslintrc.yml
@@ -1,10 +1,11 @@
 env:
   es6: true
   node: true
+  mocha: true
 
 parserOptions:
   sourceType: module
-  ecmaVersion: 7
+  ecmaVersion: 8
 
 extends:
   - eslint:recommended
@@ -17,6 +18,7 @@ plugins:
 root: true
 
 rules:
+  array-bracket-spacing: [error, never]
   eqeqeq: error
   generator-star-spacing: [warn, {before: true, after: false}]
   guard-for-in: warn  # There's nothing wrong with for..in if you know what you're doing. This is here just to keep me from accidentally saying "for..in" when I mean "for..of". Delete this and come up with a better solution if we ever need to use "for..in".
@@ -36,6 +38,7 @@ rules:
   no-useless-escape: error
   no-var: warn
   no-warning-comments: [warn, {terms: [xxx, fixme, hack], location: start}]
+  object-curly-spacing: [error, never]
   object-shorthand: [error, properties]
   prefer-const: off
   quotes: [error, single, {avoidEscape: true, allowTemplateLiterals: true}]

diff --git a/.travis.yml b/.travis.yml
@@ -14,7 +14,7 @@ jobs:
         firefox: latest
       script:
         - export PATH=$PATH:$TRAVIS_BUILD_DIR/node_modules/geckodriver/bin
-        - test/functional/setup_http_server &
+        - test/functional/start_http_server &
         - npm test
         - kill %1
       after_success:

diff --git a/Makefile b/Makefile
@@ -8,6 +8,7 @@ all: $(JS)
 
 lint:
 	@node_modules/.bin/eslint --ext mjs .
+	@node_modules/.bin/eslint test/functional
 
 test: $(JS)
 	@node_modules/.bin/mocha --recursive

diff --git a/test/.eslintrc.yml b/test/.eslintrc.yml
diff --git a/test/functional/isVisible.html b/test/functional/isVisible.html
@@ -2,86 +2,54 @@
 <html lang="en" dir="ltr">
   <head>
     <meta charset="utf-8">
-    <link rel="shortcut icon" href="">
+    <link rel="icon" href="data:,"> 
     <title>isVisible functional test</title>
     <style type="text/css">
       :root {
         --size: 100px;
       }
-      #not-visible-1 {
-        width: 0px;
-        overflow: hidden;
-      }
-      #not-visible-2 {
-        height: 0px;
-        overflow: hidden;
-      }
-      #not-visible-3 {
-        display: none;
-      }
-      #not-visible-4-ancestor {
-        display: none;
-      }
-      #not-visible-5 {
-        opacity: 0;
-      }
-      #not-visible-6-ancestor {
-        opacity: 0;
-      }
-      #not-visible-7-ancestor {
-        width: 0;
-      }
-      #not-visible-7 {
-        overflow: hidden;
-      }
-      #not-visible-8-ancestor {
-        height: 0;
-      }
-      #not-visible-8 {
-        overflow: hidden;
-      }
       .off-screen {
         width: var(--size);
         height: var(--size);
         border: 1px solid black;
         position: absolute;
       }
-      #not-visible-9 {
-        left: calc(-2 * var(--size));
-      }
-      #not-visible-10 {
-        top: calc(-2 * var(--size));
-      }
-      #not-visible-11 {
-        right: calc(100vw + 2 * var(--size));
-      }
-      #not-visible-12 {
-        bottom: calc(100vh + 2 * var(--size));
-      }
-
     </style>
   </head>
   <body>
     <h1>isVisible functional test</h1>
-    <div id="not-visible-1"></div>
-    <div id="not-visible-2"></div>
-    <div id="not-visible-3"></div>
-    <div id="not-visible-4-ancestor">
+    <div id="not-visible-1" style="width: 0px; overflow: hidden;"></div>
+    <div id="not-visible-2" style="height: 0px; overflow: hidden;"></div>
+    <div id="not-visible-3" style="display: none;"></div>
+    <!-- ``display: none`` ancestor -->
+    <div id="ancestor-not-visible-4" style="display: none;">
       <div id="not-visible-4"></div>
     </div>
-    <div id="not-visible-5"></div>
-    <div id="not-visible-6-ancestor">
+    <div id="not-visible-5" style="opacity: 0;"></div>
+    <!-- ``opacity: 0`` ancestor -->
+    <div id="ancestor-not-visible-6" style="opacity: 0;">
       <div id="not-visible-6"></div>
     </div>
-    <div id="not-visible-7-ancestor">
-      <div id="not-visible-7"></div>
+    <!-- ``overflow: hidden`` with zero width ancestor -->
+    <div id="ancestor-not-visible-7" style="width: 0;">
+      <div id="not-visible-7" style="overflow: hidden;"></div>
+    </div>
+    <!-- ``overflow: hidden`` with zero height ancestor -->
+    <div id="ancestor-not-visible-8" style="height: 0;">
+      <div id="not-visible-8" style="overflow: hidden;"></div>
     </div>
-    <div id="not-visible-8-ancestor">
-      <div id="not-visible-8"></div>
+    <!-- off-screen to the left of viewport -->
+    <div id="not-visible-9" class="off-screen" style="left: calc(-2 * var(--size));"></div>
+    <!-- off-screen above viewport -->
+    <div id="not-visible-10" class="off-screen" style="top: calc(-2 * var(--size));"></div>
+    <!-- off-screen to the right of viewport -->
+    <div id="not-visible-11" class="off-screen" style="right: calc(100vw + 2 * var(--size));"></div>
+    <!-- off-screen below viewport -->
+    <div id="not-visible-12" class="off-screen" style="bottom: calc(100vh + 2 * var(--size));"></div>
+    <div id="not-visible-13" style="display: contents"></div>
+    <!-- ``display: contents`` ancestor -->
+    <div id="ancestor-visible-1" style="display: contents">
+      <div id="visible-1"></div>
     </div>
-    <div id="not-visible-9" class="off-screen"></div>
-    <div id="not-visible-10" class="off-screen"></div>
-    <div id="not-visible-11" class="off-screen"></div>
-    <div id="not-visible-12" class="off-screen"></div>
   </body>
 </html>
diff --git a/test/functional/isVisible.js b/test/functional/isVisible.js
@@ -1,9 +1,9 @@
-const { assert } = require('chai');
+const {assert} = require('chai');
 const firefox = require('selenium-webdriver/firefox');
 const webdriver = require('selenium-webdriver');
-const { ancestors, isDomElement, isVisible, toDomElement } = require('../../utilsForFrontend');
+const {ancestors, isDomElement, isVisible, toDomElement} = require('../../utilsForFrontend');
 
-const { until, By } = webdriver;
+const {Builder, until, By} = webdriver;
 
 const WAIT_MS = 10000;
 const TEST_PAGE_URL = 'http://localhost:8000/isVisible.html';
@@ -12,45 +12,46 @@ describe('isVisible', () => {
     const options = new firefox.Options();
     options.headless();
 
-    const driver = new webdriver.Builder()
+    const driver = new Builder()
         .forBrowser('firefox')
         .setFirefoxOptions(options)
         .build();
 
-    it('Should return false when an element is hidden', async () => {
-        const hiddenElementIds = [
-            'not-visible-1',
-            'not-visible-2',
-            'not-visible-3',
-            'not-visible-4',
-            'not-visible-5',
-            'not-visible-6',
-            'not-visible-7',
-            'not-visible-8',
-            'not-visible-9',
-            'not-visible-10',
-            'not-visible-11',
-            'not-visible-12',
-        ];
+    async function checkVisibility(id, expected) {
+        await driver.wait(until.elementLocated(By.id(id)), WAIT_MS);
+        const isElementVisible = await driver.executeScript(`
+            ${ancestors}
+            ${isDomElement}
+            ${toDomElement}
+            return ${isVisible}(document.getElementById('${id}'));
+        `);
+        assert.equal(
+            isElementVisible,
+            expected,
+            `isVisible should return false for element with id '${id}'.`
+        );
+    }
+
+    it('should return false when an element is hidden', async () => {
+        const hiddenElementIds = await driver.executeScript(`
+            return [].slice.call(document.querySelectorAll('[id^="not-visible-"]')).map((element) => element.id);
+        `);
+
         await driver.get(TEST_PAGE_URL);
-        // await driver.wait(async () => {
-        //     const readyState = await driver.executeScript('return document.readyState');
-        //     return readyState === 'complete';
-        // }, WAIT_MS, `Page did not finish loading after ${WAIT_MS} ms`);
 
         for (const id of hiddenElementIds) {
-            await driver.wait(until.elementLocated(By.id(id)), WAIT_MS);
-            const isElementVisible = await driver.executeScript(`
-                ${ancestors}
-                ${isDomElement}
-                ${toDomElement}
-                return ${isVisible}(document.getElementById('${id}'));
-            `);
-            assert.equal(
-                isElementVisible,
-                false,
-                `isVisible should return false for element with id '${id}'.`
-            );
+            await checkVisibility(id, false);
+        }
+    }).timeout(60000);
+
+    it('should return true when an element is visible', async () => {
+        const visibleElementIds = await driver.executeScript(`
+            return [].slice.call(document.querySelectorAll('[id^="visible-"]')).map((element) => element.id);
+        `);
+        await driver.get(TEST_PAGE_URL);
+
+        for (const id of visibleElementIds) {
+            await checkVisibility(id, true);
         }
     }).timeout(60000);
 

diff --git a/test/functional/setup_http_server → test/functional/start_http_server b/test/functional/setup_http_server → test/functional/start_http_server
@@ -10,9 +10,7 @@ const server = http.createServer((request, response) => {
     const path = url.parse(request.url).pathname;
     fs.readFile(__dirname + path, 'utf8', (error, data) => {
         if (error) {
-            response.writeHead(404);
-            response.write(`There was an error: ${error.errno}, ${error.code}`);
-            response.end();
+            console.error(`There was a ${error.code} error fetching the resource at ${path}.`);
         } else {
             response.writeHead(200, {'Content-Type': 'text/html'});
             response.write(data);