Merge changes I4937ba59,I87b9ade6,I65f1eadd

* changes:
  Apply diff preferences immediately after clicking save
  Make the flush() compatible with async/await
  Convert gr-diff_test.html to Karma tests

polygerrit-ui/app/.eslintrc.js

@@ -180,7 +180,13 @@ module.exports = {
       },
     },
     {
-      "files": ["*.html", "common-test-setup.js", "common-test-setup-karma.js", "*_test.js"],
+      "files": [
+        "*.html",
+        "common-test-setup.js",
+        "common-test-setup-karma.js",
+        "*_test.js",
+        "a11y-test-utils.js",
+      ],
       // Additional global variables allowed in tests
       "globals": {
         // Global variables from 3rd party test libraries/frameworks.
@@ -188,6 +194,7 @@ module.exports = {
         // variables from these libraries and import is not possible
         "MockInteractions": "readonly",
         "_": "readonly",
+        "axs": "readonly",
         "a11ySuite": "readonly",
         "assert": "readonly",
         "expect": "readonly",

polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog.js

@@ -34,9 +34,20 @@ class GrDiffPreferencesDialog extends GestureEventListeners(
 
   static get properties() {
     return {
-    /** @type {?} */
+      /** @type {?} */
       diffPrefs: Object,
 
+      /**
+       * _editableDiffPrefs is a clone of diffPrefs.
+       * All changes in the dialog are applied to this object
+       * immediately, when a value in an editor is changed.
+       * The "Save" button replaces the "diffPrefs" object with
+       * the value of _editableDiffPrefs.
+       *
+       * @type {?}
+       */
+      _editableDiffPrefs: Object,
+
       _diffPrefsChanged: Boolean,
     };
   }
@@ -62,6 +73,10 @@ class GrDiffPreferencesDialog extends GestureEventListeners(
   }
 
   open() {
+    // JSON.parse(JSON.stringify(...)) makes a deep clone of diffPrefs.
+    // It is known, that diffPrefs is obtained from an RestAPI call and
+    // it is safe to clone the object this way.
+    this._editableDiffPrefs = JSON.parse(JSON.stringify(this.diffPrefs));
     this.$.diffPrefsOverlay.open().then(() => {
       const focusStops = this.getFocusStops();
       this.$.diffPrefsOverlay.setFocusStops(focusStops);
@@ -70,6 +85,7 @@ class GrDiffPreferencesDialog extends GestureEventListeners(
   }
 
   _handleSaveDiffPreferences() {
+    this.diffPrefs = this._editableDiffPrefs;
     this.$.diffPreferences.save().then(() => {
       this.dispatchEvent(new CustomEvent('reload-diff-preference', {
         composed: true, bubbles: false,

polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_html.js

@@ -53,7 +53,7 @@ export const htmlTemplate = html`
     </div>
     <gr-diff-preferences
       id="diffPreferences"
-      diff-prefs="{{diffPrefs}}"
+      diff-prefs="{{_editableDiffPrefs}}"
       has-unsaved-changes="{{_diffPrefsChanged}}"
     ></gr-diff-preferences>
     <div class="diffActions">

polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.js

@@ -0,0 +1,50 @@
+/**
+ * @license
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import '../../../test/common-test-setup-karma.js';
+
+const basicFixture = fixtureFromElement('gr-diff-preferences-dialog');
+
+suite('gr-diff-preferences-dialog', () => {
+  let element;
+  setup(() => {
+    element = basicFixture.instantiate();
+  });
+  test('changes applies only on save', async () => {
+    const originalDiffPrefs = {
+      line_wrapping: true,
+    };
+    element.diffPrefs = originalDiffPrefs;
+
+    element.open();
+    await flush();
+    assert.isTrue(element.$.diffPreferences.$.lineWrappingInput.checked);
+
+    MockInteractions.tap(element.$.diffPreferences.$.lineWrappingInput);
+    await flush();
+    assert.isFalse(element.$.diffPreferences.$.lineWrappingInput.checked);
+    assert.isTrue(element._diffPrefsChanged);
+    assert.isTrue(element.diffPrefs.line_wrapping);
+    assert.isTrue(originalDiffPrefs.line_wrapping);
+
+    MockInteractions.tap(element.$.saveButton);
+    await flush();
+    // Original prefs must remains unchanged, dialog must expose a new object
+    assert.isTrue(originalDiffPrefs.line_wrapping);
+    assert.isFalse(element.diffPrefs.line_wrapping);
+  });
+});

polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js

@@ -685,21 +685,26 @@ class GrDiff extends mixinBehaviors( [
   }
 
   _prefsObserver(newPrefs, oldPrefs) {
-    // Scan the preference objects one level deep to see if they differ.
-    let differ = !oldPrefs;
-    if (newPrefs && oldPrefs) {
-      for (const key in newPrefs) {
-        if (newPrefs[key] !== oldPrefs[key]) {
-          differ = true;
-        }
-      }
-    }
-
-    if (differ) {
+    if (!this._prefsEqual(newPrefs, oldPrefs)) {
       this._prefsChanged(newPrefs);
     }
   }
 
+  _prefsEqual(prefs1, prefs2) {
+    if (prefs1 === prefs2) {
+      return true;
+    }
+    if (!prefs1 || !prefs2) {
+      return false;
+    }
+    // Scan the preference objects one level deep to see if they differ.
+    const keys1 = Object.keys(prefs1);
+    const keys2 = Object.keys(prefs2);
+    return keys1.length === keys2.length &&
+        keys1.every(key => prefs1[key] === prefs2[key]) &&
+        keys2.every(key => prefs1[key] === prefs2[key]);
+  }
+
   _pathObserver() {
     // Call _prefsChanged(), because line-limit style value depends on path.
     this._prefsChanged(this.prefs);

polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js

@@ -1,39 +1,21 @@
-<!DOCTYPE html>
-<!--
-@license
-Copyright (C) 2015 The Android Open Source Project
+/**
+ * @license
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
--->
-
-<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
-<meta charset="utf-8">
-<title>gr-diff</title>
-
-<script src="/node_modules/@webcomponents/webcomponentsjs/custom-elements-es5-adapter.js"></script>
-
-<script src="/node_modules/@webcomponents/webcomponentsjs/webcomponents-lite.js"></script>
-<script src="/components/wct-browser-legacy/browser.js"></script>
-<script src="/components/web-component-tester/data/a11ySuite.js"></script>
-
-<test-fixture id="basic">
-  <template>
-    <gr-diff></gr-diff>
-  </template>
-</test-fixture>
-
-<script type="module">
-import '../../../test/common-test-setup.js';
+import '../../../test/common-test-setup-karma.js';
 import '../../shared/gr-rest-api-interface/gr-rest-api-interface.js';
 import {getMockDiffResponse} from '../../../test/mocks/diff-response.js';
 import './gr-diff.js';
@@ -41,6 +23,16 @@ import {dom, flush} from '@polymer/polymer/lib/legacy/polymer.dom.js';
 import {GrDiffBuilderImage} from '../gr-diff-builder/gr-diff-builder-image.js';
 import {util} from '../../../scripts/util.js';
 import {_setHiddenScroll} from '../../../scripts/hiddenscroll.js';
+import {runA11yAudit} from '../../../test/a11y-test-utils.js';
+import '@polymer/paper-button/paper-button.js';
+
+const basicFixture = fixtureFromElement('gr-diff');
+
+suite('gr-diff a11y test', () => {
+  test('audit', async () => {
+    await runA11yAudit(basicFixture);
+  });
+});
 
 suite('gr-diff tests', () => {
   let element;
@@ -62,7 +54,7 @@ suite('gr-diff tests', () => {
     };
 
     setup(() => {
-      element = fixture('basic');
+      element = basicFixture.instantiate();
       sandbox.stub(element.$.highlights, 'handleSelectionChange');
     });
 
@@ -80,21 +72,21 @@ suite('gr-diff tests', () => {
   });
 
   test('cancel', () => {
-    element = fixture('basic');
+    element = basicFixture.instantiate();
     const cancelStub = sandbox.stub(element.$.diffBuilder, 'cancel');
     element.cancel();
     assert.isTrue(cancelStub.calledOnce);
   });
 
   test('line limit with line_wrapping', () => {
-    element = fixture('basic');
+    element = basicFixture.instantiate();
     element.prefs = Object.assign({}, MINIMAL_PREFS, {line_wrapping: true});
     flushAsynchronousOperations();
     assert.equal(util.getComputedStyleValue('--line-limit', element), '80ch');
   });
 
   test('line limit without line_wrapping', () => {
-    element = fixture('basic');
+    element = basicFixture.instantiate();
     element.prefs = Object.assign({}, MINIMAL_PREFS, {line_wrapping: false});
     flushAsynchronousOperations();
     assert.isNotOk(util.getComputedStyleValue('--line-limit', element));
@@ -105,7 +97,7 @@ suite('gr-diff tests', () => {
     let contentEl;
 
     setup(() => {
-      element = fixture('basic');
+      element = basicFixture.instantiate();
       lineEl = document.createElement('td');
       contentEl = document.createElement('span');
     });
@@ -191,7 +183,7 @@ suite('gr-diff tests', () => {
       stub('gr-rest-api-interface', {
         getLoggedIn() { return getLoggedInPromise; },
       });
-      element = fixture('basic');
+      element = basicFixture.instantiate();
       return getLoggedInPromise;
     });
 
@@ -645,7 +637,7 @@ suite('gr-diff tests', () => {
   suite('logged in', () => {
     let fakeLineEl;
     setup(() => {
-      element = fixture('basic');
+      element = basicFixture.instantiate();
       element.loggedIn = true;
       element.patchRange = {};
 
@@ -710,6 +702,22 @@ suite('gr-diff tests', () => {
         assert.isTrue(element._renderDiffTable.called);
       });
 
+      test('adding/removing property in preferences re-renders diff', () => {
+        const stub = sandbox.stub(element, '_renderDiffTable');
+        const newPrefs1 = Object.assign({}, MINIMAL_PREFS,
+            {line_wrapping: true});
+        element.prefs = newPrefs1;
+        element.flushDebouncer('renderDiffTable');
+        assert.isTrue(element._renderDiffTable.called);
+        stub.reset();
+
+        const newPrefs2 = Object.assign({}, newPrefs1);
+        delete newPrefs2.line_wrapping;
+        element.prefs = newPrefs2;
+        element.flushDebouncer('renderDiffTable');
+        assert.isTrue(element._renderDiffTable.called);
+      });
+
       test('change in preferences does not re-renders diff with ' +
           'noRenderOnPrefsChange', () => {
         sandbox.stub(element, '_renderDiffTable');
@@ -724,7 +732,7 @@ suite('gr-diff tests', () => {
 
   suite('diff header', () => {
     setup(() => {
-      element = fixture('basic');
+      element = basicFixture.instantiate();
       element.diff = {
         meta_a: {name: 'carrot.jpg', content_type: 'image/jpeg', lines: 66},
         meta_b: {name: 'carrot.jpg', content_type: 'image/jpeg',
@@ -769,7 +777,7 @@ suite('gr-diff tests', () => {
     let renderStub;
 
     setup(() => {
-      element = fixture('basic');
+      element = basicFixture.instantiate();
       renderStub = sandbox.stub(element.$.diffBuilder, 'render',
           () => {
             element.$.diffBuilder.dispatchEvent(
@@ -821,7 +829,7 @@ suite('gr-diff tests', () => {
 
   suite('blame', () => {
     setup(() => {
-      element = fixture('basic');
+      element = basicFixture.instantiate();
     });
 
     test('unsetting', () => {
@@ -847,7 +855,7 @@ suite('gr-diff tests', () => {
       element.shadowRoot.querySelector('.newlineWarning').textContent;
 
     setup(() => {
-      element = fixture('basic');
+      element = basicFixture.instantiate();
       element.showNewlineWarningLeft = false;
       element.showNewlineWarningRight = false;
     });
@@ -907,7 +915,7 @@ suite('gr-diff tests', () => {
     let renderStub;
 
     setup(() => {
-      element = fixture('basic');
+      element = basicFixture.instantiate();
       element.prefs = {};
       renderStub = sandbox.stub(element.$.diffBuilder, 'render')
           .returns(new Promise(() => {}));
@@ -956,7 +964,7 @@ suite('gr-diff tests', () => {
   });
   const setupSampleDiff = function(params) {
     const {ignore_whitespace, content} = params;
-    element = fixture('basic');
+    element = basicFixture.instantiate();
     element.prefs = {
       ignore_whitespace: ignore_whitespace || 'IGNORE_ALL',
       auto_hide_diff_table_header: true,
@@ -1115,7 +1123,7 @@ suite('gr-diff tests', () => {
   });
 
   test('`render` event has contentRendered field in detail', done => {
-    element = fixture('basic');
+    element = basicFixture.instantiate();
     element.prefs = {};
     sandbox.stub(element.$.diffBuilder, 'render')
         .returns(Promise.resolve());
@@ -1125,7 +1133,21 @@ suite('gr-diff tests', () => {
     });
     element._renderDiffTable();
   });
-});
 
-a11ySuite('basic');
-</script>
+  test('_prefsEqual', () => {
+    element = basicFixture.instantiate();
+    assert.isTrue(element._prefsEqual(null, null));
+    assert.isTrue(element._prefsEqual({}, {}));
+    assert.isTrue(element._prefsEqual({x: 1}, {x: 1}));
+    assert.isTrue(element._prefsEqual({x: 1, abc: 'def'}, {x: 1, abc: 'def'}));
+    const somePref = {abc: 'def', p: true};
+    assert.isTrue(element._prefsEqual(somePref, somePref));
+
+    assert.isFalse(element._prefsEqual({}, null));
+    assert.isFalse(element._prefsEqual(null, {}));
+    assert.isFalse(element._prefsEqual({x: 1}, {x: 2}));
+    assert.isFalse(element._prefsEqual({x: 1, y: 'abc'}, {x: 1, y: 'abcd'}));
+    assert.isFalse(element._prefsEqual({x: 1, y: 'abc'}, {x: 1}));
+    assert.isFalse(element._prefsEqual({x: 1}, {x: 1, y: 'abc'}));
+  });
+});

polygerrit-ui/app/test/a11y-test-utils.js

@@ -0,0 +1,44 @@
+/**
+ * @license
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import './common-test-setup-karma.js';
+
+// Run a11y audit on test fixture
+// The code is inspired by the
+// https://github.com/Polymer/web-component-tester/blob/master/data/a11ySuite.js
+export async function runA11yAudit(fixture, ignoredRules) {
+  fixture.instantiate();
+  await flush();
+  const axsConfig = new axs.AuditConfiguration();
+  axsConfig.scope = document.body;
+  axsConfig.showUnsupportedRulesWarning = false;
+  axsConfig.auditRulesToIgnore = ignoredRules;
+
+  const auditResults = axs.Audit.run(axsConfig);
+  const errors = [];
+  auditResults.forEach((result, index) => {
+    // only show applicable tests
+    if (result.result === 'FAIL') {
+      const title = result.rule.heading;
+      // fail test if audit result is FAIL
+      const error = axs.Audit.accessibilityErrorMessage(result);
+      errors.push(`${title}: ${error}`);
+    }
+  });
+  if (errors.length > 0) {
+    assert.fail(errors.join('\n') + '\n');
+  }
+}

polygerrit-ui/app/test/common-test-setup-karma.js

@@ -22,7 +22,8 @@ self.assert = window.chai.assert;
 
 /**
  * Triggers a flush of any pending events, observations, etc and calls you back
- * after they have been processed.
+ * after they have been processed if callback is passed; otherwise returns
+ * promise.
  *
  * @param {function()} callback
  */
@@ -31,7 +32,13 @@ function flush(callback) {
   // doesn't support a callback yet
   // (https://github.com/Polymer/polymer-dev/issues/851)
   window.Polymer.dom.flush();
-  window.setTimeout(callback, 0);
+  if (callback) {
+    window.setTimeout(callback, 0);
+  } else {
+    return new Promise(resolve => {
+      window.setTimeout(resolve, 0);
+    });
+  }
 }
 
 self.flush = flush;

polygerrit-ui/app/test/tests.js

@@ -108,7 +108,6 @@ const elements = [
   'diff/gr-diff-selection/gr-diff-selection_test.html',
   'diff/gr-diff-view/gr-diff-view_test.html',
   'diff/gr-diff/gr-diff-group_test.html',
-  'diff/gr-diff/gr-diff_test.html',
   'diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.html',
   'diff/gr-patch-range-select/gr-patch-range-select_test.html',
   'diff/gr-ranged-comment-layer/gr-ranged-comment-layer_test.html',

polygerrit-ui/karma.conf.js

@@ -34,12 +34,12 @@ function getModulesDir() {
   ];
 }
 
-function getSinonPath() {
+function getUiDevNpmFilePath(importPath) {
   if(runUnderBazel) {
-    return "external/ui_dev_npm/node_modules/sinon/pkg/sinon.js";
+    return `external/ui_dev_npm/node_modules/${importPath}`;
   }
   else {
-    return "polygerrit-ui/node_modules/sinon/pkg/sinon.js"
+    return `polygerrit-ui/node_modules/${importPath}`
   }
 }
 
@@ -73,7 +73,8 @@ module.exports = function(config) {
 
     // list of files / patterns to load in the browser
     files: [
-      getSinonPath(),
+      getUiDevNpmFilePath('accessibility-developer-tools/dist/js/axs_testing.js'),
+      getUiDevNpmFilePath('sinon/pkg/sinon.js'),
       { pattern: testFilesPattern, type: 'module' },
     ],
     esm: {
@@ -127,7 +128,8 @@ module.exports = function(config) {
 
     client: {
       mocha: {
-        ui: 'tdd'
+        ui: 'tdd',
+        timeout: 5000,
       }
     },
 

polygerrit-ui/package.json

@@ -7,6 +7,7 @@
     "@open-wc/karma-esm": "^2.13.21",
     "@polymer/iron-test-helpers": "^3.0.1",
     "@polymer/test-fixture": "^4.0.2",
+    "accessibility-developer-tools": "^2.12.0",
     "chai": "^4.2.0",
     "karma": "^4.4.1",
     "karma-chrome-launcher": "^3.1.0",