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

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

Index: chrome/browser/resources/settings/device_page/night_light_slider.js
diff --git a/chrome/browser/resources/settings/device_page/night_light_slider.js b/chrome/browser/resources/settings/device_page/night_light_slider.js
index 64e3aa5ac0648791c9254318c4054f488fe1015d..7f42f6a5548caa79a3f52cae6c8103b460e94666 100644
--- a/chrome/browser/resources/settings/device_page/night_light_slider.js
+++ b/chrome/browser/resources/settings/device_page/night_light_slider.js
@@ -21,6 +21,7 @@ Polymer({
     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: {
@@ -71,6 +72,12 @@ Polymer({
       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_();
@@ -111,7 +118,19 @@ Polymer({
    */
   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.
@@ -446,4 +465,69 @@ Polymer({
 
     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.
+  }
 });