WebUI[MD-refresh]: fix various tabindex issues for cr-input

This fixes a few bugs introduced by 1103002 to cr-input, as well as making
cr-input more robust against edge-cases:

likely cases:
- |tabindex| initially set by element are mistakenly set to 0.
- if element has both |tabindex| and |disabled| set, element
  would not know what to set |tabindex| to once |disabled| is removed.
- clicking on a |disabled| cr-input would set its |tabindex| to 0 again.

edge case:
- if |disabled| is changed to true in the same cycle as pointerdown is fired,
  then |tabindex| state gets messed up.

Bug: 832177, 856118
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I91161ecde90e816340aa38db4646b3f8eedd7a8b
Reviewed-on: https://chromium-review.googlesource.com/1111201
Commit-Queue: Scott Chen <scottchen@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570198}

chrome/browser/resources/settings/people_page/sync_page.js

@@ -290,7 +290,8 @@ Polymer({
       listenOnce(document, 'show-container', () => {
         const input = /** @type {!CrInputElement} */ (
             this.$$('#existingPassphraseInput'));
-        input.focus();
+        if (!input.matches(':focus-within'))
+          input.focus();
       });
     }
   },

chrome/test/data/webui/cr_elements/cr_input_test.js

@@ -42,17 +42,89 @@ suite('cr-input', function() {
   });
 
   test('togglingDisableModifiesTabIndexCorrectly', function() {
-    crInput.tabindex = 14;
+    // Do innerHTML instead of createElement to make sure it's correct right
+    // after being attached, and not messed up by disabledChanged_.
+    PolymerTest.clearBody();
+    document.body.innerHTML = `
+      <cr-input tabindex="14"></cr-input>
+    `;
+    crInput = document.querySelector('cr-input');
+    input = crInput.$.input;
+    Polymer.dom.flush();
+
     assertEquals('14', crInput.getAttribute('tabindex'));
     assertEquals(14, input.tabIndex);
     crInput.disabled = true;
-    assertEquals('-1', crInput.getAttribute('tabindex'));
-    assertEquals(-1, input.tabIndex);
+    assertEquals(null, crInput.getAttribute('tabindex'));
+    assertEquals(true, input.disabled);
     crInput.disabled = false;
     assertEquals('14', crInput.getAttribute('tabindex'));
     assertEquals(14, input.tabIndex);
   });
 
+  test('startingWithDisableSetsTabIndexCorrectly', function() {
+    // Makes sure tabindex is recorded even if cr-input starts as disabled
+    PolymerTest.clearBody();
+    document.body.innerHTML = `
+      <cr-input tabindex="14" disabled></cr-input>
+    `;
+    crInput = document.querySelector('cr-input');
+    input = crInput.$.input;
+    Polymer.dom.flush();
+
+    assertEquals(null, crInput.getAttribute('tabindex'));
+    assertEquals(true, input.disabled);
+    crInput.disabled = false;
+    assertEquals('14', crInput.getAttribute('tabindex'));
+    assertEquals(14, input.tabIndex);
+  });
+
+  test('pointerDownAndTabIndex', function() {
+    crInput.fire('pointerdown');
+    assertEquals(null, crInput.getAttribute('tabindex'));
+    return test_util.whenAttributeIs(crInput, 'tabindex', '0').then(() => {
+      assertEquals(0, input.tabIndex);
+    });
+  });
+
+  test('pointerdownWhileDisabled', function(done) {
+    // pointerdown while disabled doesn't mess with tabindex.
+    crInput.tabindex = 14;
+    crInput.disabled = true;
+    assertEquals(null, crInput.getAttribute('tabindex'));
+    assertEquals(true, input.disabled);
+    crInput.fire('pointerdown');
+    assertEquals(null, crInput.getAttribute('tabindex'));
+
+    // Wait one cycle to make sure tabindex is still unchanged afterwards.
+    setTimeout(() => {
+      assertEquals(null, crInput.getAttribute('tabindex'));
+      // Makes sure tabindex is correctly restored after reverting disabled.
+      crInput.disabled = false;
+      assertEquals('14', crInput.getAttribute('tabindex'));
+      assertEquals(14, input.tabIndex);
+      done();
+    });
+  });
+
+  test('pointerdownThenDisabledInSameCycle', function(done) {
+    crInput.tabindex = 14;
+    // Edge case: pointerdown and disabled are changed in the same cycle.
+    crInput.fire('pointerdown');
+    crInput.disabled = true;
+    assertEquals(null, crInput.getAttribute('tabindex'));
+
+    // Wait one cycle to make sure tabindex is still unchanged afterwards.
+    setTimeout(() => {
+      assertEquals(null, crInput.getAttribute('tabindex'));
+      // Makes sure tabindex is correctly restored after reverting disabled.
+      crInput.disabled = false;
+      assertEquals('14', crInput.getAttribute('tabindex'));
+      assertEquals(14, input.tabIndex);
+      done();
+    });
+  });
+
   test('placeholderCorrectlyBound', function() {
     assertFalse(input.hasAttribute('placeholder'));
 

ui/webui/resources/cr_elements/cr_input/cr_input.js

@@ -102,7 +102,7 @@ Polymer({
       reflectToAttribute: true,
     },
 
-    /** @type {number|undefined} */
+    /** @type {?number} */
     tabindex: {
       type: Number,
       value: 0,
@@ -135,11 +135,19 @@ Polymer({
     'pointerdown': 'onPointerDown_',
   },
 
+  /** @private {?number} */
+  originalTabIndex_: null,
+
   /** @override */
   attached: function() {
     const ariaLabel = this.ariaLabel || this.label || this.placeholder;
     if (ariaLabel)
       this.inputElement.setAttribute('aria-label', ariaLabel);
+
+    // Run this for the first time in attached instead of in disabledChanged_
+    // since this.tabindex might not be set yet then.
+    if (this.disabled)
+      this.reconcileTabindex_();
   },
 
   /** @return {!HTMLInputElement} */
@@ -147,20 +155,30 @@ Polymer({
     return this.$.input;
   },
 
-  /** @private {number} */
-  originalTabIndex_: 0,
-
   /** @private */
-  disabledChanged_: function() {
+  disabledChanged_: function(current, previous) {
     this.setAttribute('aria-disabled', this.disabled ? 'true' : 'false');
     // In case input was focused when disabled changes.
     this.removeAttribute('focused_');
 
+    // Don't change tabindex until after finished attaching, since this.tabindex
+    // might not be intialized yet.
+    if (previous !== undefined)
+      this.reconcileTabindex_();
+  },
+
+  /**
+   * This helper function manipulates the tabindex based on disabled state. If
+   * this element is disabled, this function will remember the tabindex and
+   * unset it. If the element is enabled again, it will restore the tabindex
+   * to it's previous value.
+   * @private
+   */
+  reconcileTabindex_: function() {
     if (this.disabled) {
-      this.originalTabIndex_ = /** @type {number} */ (this.tabindex);
-      this.tabindex = -1;
+      this.recordAndUnsetTabIndex_();
     } else {
-      this.tabindex = this.originalTabIndex_;
+      this.restoreTabIndex_();
     }
   },
 
@@ -183,6 +201,21 @@ Polymer({
       this.inputElement.focus();
   },
 
+  /** @private */
+  recordAndUnsetTabIndex_: function() {
+    // Don't change originalTabIndex_ if it just got changed.
+    if (this.originalTabIndex_ === null)
+      this.originalTabIndex_ = this.tabindex;
+
+    this.tabindex = null;
+  },
+
+  /** @private */
+  restoreTabIndex_: function() {
+    this.tabindex = this.originalTabIndex_;
+    this.originalTabIndex_ = null;
+  },
+
   /**
    * Prevents clicking random spaces within cr-input but outside of <input>
    * from triggering focus.
@@ -190,15 +223,18 @@ Polymer({
    * @private
    */
   onPointerDown_: function(e) {
+    // Don't need to manipulate tabindex if cr-input is already disabled.
+    if (this.disabled)
+      return;
+
     // Should not mess with tabindex when <input> is clicked, otherwise <input>
     // will lose and regain focus, and replay the focus animation.
     if (e.path[0].tagName !== 'INPUT') {
-      // This is intentionally different from this.originalTabIndex_ since this
-      // only needs to be temporarily stored.
-      const prevTabindex = this.tabindex;
-      this.tabindex = undefined;
+      this.recordAndUnsetTabIndex_();
       setTimeout(() => {
-        this.tabindex = prevTabindex;
+        // Restore tabindex, unless disabled in the same cycle as pointerdown.
+        if (!this.disabled)
+          this.restoreTabIndex_();
       }, 0);
     }
   },

ui/webui/resources/cr_elements/cr_input/cr_input_style_css.html

@@ -11,6 +11,7 @@
         --cr-input-error-color: var(--google-red-600);
         --cr-input-focus-color: var(--google-blue-600);
         display: block;
+        outline: none;
       }
 
       /* Label styling below. */