[MD Settings][Night Light] CL8: Add the ripple focus effect to the slider knobs

chrome/browser/resources/settings/device_page/night_light_slider.js

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview
  * night-light-slider is used to set the custom automatic schedule of the
  * Night Light feature, so that users can set their desired start and end
  * times.
  */
 
 /** @const */ var HOURS_PER_DAY = 24;

[dpapad, 2017/06/20 17:35:19]

These are on global scope. Can you wrap the contents of this file in an function
scope to avoid this? See example at
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people....

[afakhry, 2017/06/20 19:59:54]

Done.

 /** @const */ var MIN_KNOBS_DISTANCE_MINUTES = 30;
 /** @const */ var OFFSET_MINUTES_6PM = 18 * 60;
 /** @const */ var TOTAL_MINUTES_PER_DAY = 24 * 60;
 
 Polymer({
   is: 'night-light-slider',
 
   behaviors: [
     I18nBehavior,

[dpapad, 2017/06/20 17:35:20]

Is this used anywhere?

[afakhry, 2017/06/20 19:59:54]

It's used in the HTML $i18n{...}. Isn't that what it's for?

[stevenjb, 2017/06/20 20:19:02]

No, $i18n is a pre-processing macro. I18nBehavior is for i18n() calls in the JS.

[afakhry, 2017/06/20 20:25:16]

Oh, I see. In that case, removed.

     PrefsBehavior,
     Polymer.IronA11yKeysBehavior,
+    Polymer.PaperInkyFocusBehavior,
   ],
 
   properties: {
     /**
      * The object currently being dragged. Either the start or end knobs.
      * @type {?Object}
      * @private
      */
     dragObject_: {
       type: Object,
       value: null,
     },
 
     /**
      * The start knob time as a string to be shown on the start label bubble.
      * @private
      */
     startTime_: {

[dpapad, 2017/06/20 17:35:19]

Nit: Use shorthand (here and elsewhere, where applicable)

startTime_: String,

[afakhry, 2017/06/20 19:59:54]

Done.

       type: String,
     },
 
     /**
      * The end knob time as a string to be shown on the end label bubble.
      * @private
      */
     endTime_: {
       type: String,
     },
   },
 
   observers: [
     'customTimesChanged_(prefs.ash.night_light.custom_start_time.*, ' +
                         'prefs.ash.night_light.custom_end_time.*)',
   ],
 
   keyBindings: {
     'left': 'onLeftKey_',
     'right': 'onRightKey_',
   },
 
   ready: function() {

[dpapad, 2017/06/20 17:35:19]

Missing annotation:

@override

[afakhry, 2017/06/20 19:59:54]

Done.

     // Build the legend markers.
     var markersContainer = this.$.markersContainer;
     var width = markersContainer.offsetWidth;
     for (var i = 0; i <= HOURS_PER_DAY; ++i) {
       var marker = document.createElement('div');
       marker.className = 'markers';
       markersContainer.appendChild(marker);
       marker.style.left = (i * 100 / HOURS_PER_DAY) + '%';
     }
+
+    this.$.startKnob.addEventListener('focus', this.onFocus_.bind(this));

[dpapad, 2017/06/20 01:24:47]

Why are those listeners added programmatically instead of declaratively?

[afakhry, 2017/06/20 19:59:54]

I have no idea. I tried that at first, but Chrome kept complaining with the
following error:

"Refused to execute inline event handler because it violates the following
Content Security Policy directive: "script-src chrome://resources 'self'
'unsafe-eval'". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or a
nonce ('nonce-...') is required to enable inline execution."

[dpapad, 2017/06/20 20:39:09]

That means that the declarative syntax was most likely incorrect.

It should look roughly like this
<button on-tap="onTap_">....</button>

where onTap_ is a method you define in the JS. See examples
https://cs.chromium.org/search/?q=%22on-tap%22+file:%5Esrc/chrome/browser/res....

[afakhry, 2017/06/20 20:50:09]

Oh, I was using onfocus="onFocus_" instead of on-focus="onFocus_".

Done.

+    this.$.startKnob.addEventListener('blur', this.onBlur_.bind(this));
+    this.$.endKnob.addEventListener('focus', this.onFocus_.bind(this));
+    this.$.endKnob.addEventListener('blur', this.onBlur_.bind(this));
+
     this.async(function() {
       // Read the initial prefs values and refresh the slider.
       this.customTimesChanged_();
     });
   },
 
   /**
    * Expands or un-expands the knob being dragged along with its corresponding
    * label bubble.
    * @param {boolean} expand True to expand, and false to un-expand.
@@ skipping 20 matching lines @@
     if (activeElement == this.$.startKnob || activeElement == this.$.endKnob)
       activeElement.blur();
   },
 
   /**
    * Start dragging the target knob.
    * @private
    */
   startDrag_: function(event) {
     event.preventDefault();
-    this.dragObject_ = event.target;
+
+    // Only handle start or end knobs. Use the "knob-inner" divs just to display
+    // the knobs.
+    if (event.target == this.$.startKnob ||
+        event.target == this.$.startKnob.firstElementChild) {
+      this.dragObject_ = this.$.startKnob;

[dpapad, 2017/06/20 01:24:47]

Is dragObject_ used in the HTML? If not, there is no good reason to have it as a
Polymer property.

[afakhry, 2017/06/20 19:59:54]

Sorry, I'm not very familiar with javascript. What's the alternative? Should I
define it somewhere in the file? Or should I just use it as is and it will be
added to the prototype of object 'this'?

[dpapad, 2017/06/20 20:39:09]

See example
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/defaul....
This is more of a peculiarity of Polymer v1 syntax, and less of Javascript.

In essence everything defined in 'properties' is a Polymer property, allowing
the HTML and JS code to interoperate. There is no need to use it for variables
that are only used in JS (and not involved in any Polymer data bindings).

[afakhry, 2017/06/20 20:50:09]

Thanks for the pointer and the explanation. Done.

+    } else if (event.target == this.$.endKnob ||
+               event.target == this.$.endKnob.firstElementChild) {
+      this.dragObject_ = this.$.endKnob;
+    } else {
+      return;
+    }
+
     this.setExpanded_(true);
 
     // Focus is only given to the knobs by means of keyboard tab navigations.
     // When we start dragging, we don't want to see any focus halos around any
     // knob.
     this.blurAnyFocusedKnob_();
 
     // However, our night-light-slider element must get the focus.
     this.focus();
   },
@@ skipping 314 matching lines @@
    * @private
    */
   onRightKey_: function(e) {
     e.preventDefault();
     var knobPref = this.getFocusedKnobPrefPathIfAny_();
     if (!knobPref)
       return;
 
     this.incrementPref_(knobPref, 1);
   },
+
+  /**
+   * Checks if any of the two knobs is focused.

[stevenjb, 2017/06/20 00:31:44]

s/any/either/
(Also, redundant comment not needed)

[afakhry, 2017/06/20 19:59:54]

Done.

+   *
+   * @return {boolean} true if either of the two knobs is focused, false
+   * otherwise.

[stevenjb, 2017/06/20 00:31:44]

s/false otherwise//

[dpapad, 2017/06/20 01:24:47]

Or even shorter as follows:

"Whether either of the two knobs is focused."

[afakhry, 2017/06/20 19:59:54]

Done.

[afakhry, 2017/06/20 19:59:54]

Done.

+   * @private
+   */
+  isEitherKnobFocused_: function() {
+    var activeElement = this.shadowRoot.activeElement;
+    return activeElement == this.$.startKnob || activeElement == this.$.endKnob;
+  },
+
+  /**
+   * Overrides _createRipple() from PaperInkyFocusBehavior to create the ripple
+   * only on a knob if it's focused, or on a dummy hidden element so that it
+   * doesn't show.
+   * @private

[stevenjb, 2017/06/20 00:31:44]

@override

[afakhry, 2017/06/20 19:59:54]

It doesn't actually override, but rather shadows that function we get from the
behavior. If I add @override, the closure compiler complains with:

##
/work2/chromium/src/chrome/browser/resources/settings/device_page/night_light_slider.js:489:
ERROR - property _createRipple not defined on any superclass of
NightLightSliderElement
##   _createRipple: function() {
##                  ^^^^^^^^^^^^

[dpapad, 2017/06/20 20:39:09]

I don't think @override works for inherited behaviors. 

+   */
+  _createRipple: function() {
+    if (this.isEitherKnobFocused_()) {
+      this._rippleContainer = this.shadowRoot.activeElement;
+    } else {
+      // We can't just skip the ripple creation and return early with null here.
+      // The code inherited from PaperInkyFocusBehavior expects that this
+      // function return a ripple element. So to avoid crashes, we'll setup the

[stevenjb, 2017/06/20 00:31:44]

returns

[afakhry, 2017/06/20 19:59:54]

Done.

+      // ripple to be created under a hidden element.
+      this._rippleContainer = this.$.hidden;

[stevenjb, 2017/06/20 00:31:44]

Ugh, this is kind of awkward. Can we just create a dummy unparented element?

+dpapad@ who might have a suggestion.

[dpapad, 2017/06/20 17:35:19]

Don't have a good suggestion.

[afakhry, 2017/06/20 19:59:54]

If the ripple container is null, the behavior will use the root:
https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro...

Which will show a huge circle in the middle of the page!

The only problem here that makes it different from other polymer elements is
that we have two different ripple container per the same polymer element and
they should change (e.g. from the start knob to the end knob or vice versa)
while focus is still in the same parent polymer element.

+    }
+
+    return Polymer.PaperInkyFocusBehaviorImpl._createRipple.call(this);
+  },
+
+  /**
+   * Handles focus events on the start and end knobs.
+   * @private
+   */
+  onFocus_: function() {
+    this.ensureRipple();
+
+    if (this.hasRipple()) {
+      this._ripple.style.display = '';
+      this._ripple.holdDown = true;
+    }
+  },
+
+  /**
+   * Handles blur events on the start and end knobs.
+   * @private
+   */
+  onBlur_: function() {
+    if (this.hasRipple()) {
+      this._ripple.remove();
+      this._ripple = null;
+    }
+  },
+
+  /** @private */

[stevenjb, 2017/06/20 00:31:44]

@override; also, group the overrides together.

[afakhry, 2017/06/20 19:59:54]

This is also not an override, but shadowing of the Behavior's method with the
same name. See my comment above.

Maybe change the comment and remove the word "override" to avoid confusion?

+  _focusedChanged: function(receivedFocusFromKeyboard) {
+    // Overrides the _focusedChanged() from the PaperInkyFocusBehavior so that
+    // it does nothing. This function is called only once for the entire
+    // night-light-slider element even when focus is moved between the two
+    // knobs. This doesn't allow us to decide on which knob the ripple will be
+    // created. Hence we handle focus and blur explicitly above.
+  }
 });