MD Settings: Display: Move event listeners to dialog open/close

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

 // Copyright 2016 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
  * 'settings-display-overscan-dialog' is the dialog for display overscan
  * adjustments.
  */
 
@@ skipping 12 matching lines @@
     comitted_: Boolean,
   },
 
   /**
    * Keyboard event handler for overscan adjustments.
    * @type {?function(!Event)}
    * @private
    */
   keyHandler_: null,
 
-  /** @override */
-  attached: function() {
+  open: function() {
     this.keyHandler_ = this.handleKeyEvent_.bind(this);
     window.addEventListener('keydown', this.keyHandler_);

[Dan Beam, 2016/09/27 00:04:52]

why are we using window.addEventListener() rather than a more narrow scope like
this.addEventListener()?

[stevenjb, 2016/09/28 17:49:35]

Because I'm still fairly new to web development and forgot that you can do that?

Changed.

-  },
-
-  /** @override */
-  detached: function() {
-    window.removeEventListener('keydown', this.keyHandler_);
-  },
-
-  open: function() {
     this.comitted_ = false;
     this.$.dialog.showModal();
   },
 
   close: function() {
+    window.removeEventListener('keydown', this.keyHandler_);
+
     this.displayId = '';  // Will trigger displayIdChanged_.
 
     if (this.$.dialog.open)
       this.$.dialog.close();
   },
 
   /** @private */
   displayIdChanged_: function(newValue, oldValue) {
     if (oldValue && !this.comitted_) {
       settings.display.systemDisplayApi.overscanCalibrationReset(oldValue);
@@ skipping 15 matching lines @@
     settings.display.systemDisplayApi.overscanCalibrationComplete(
         this.displayId);
     this.comitted_ = true;
     this.close();
   },
 
   /**
    * @param {Event} event
    * @private
    */
   handleKeyEvent_: function(event) {

[dschuyler, 2016/09/27 23:45:24]

IIUC, the above narrows the scope where alt-left
could be eaten by this function, but would it
help if this function did an early out if alt
(or ctrl) are down? (So it would never eat alt-left
or other combinations).

if (event.altKey || event.ctrlKey || event.mentaKey)
  return;

[stevenjb, 2016/09/28 17:49:35]

Done.

     switch (event.keyCode) {
       case 37:  // left arrow
         if (event.shiftKey)
           this.move_(-1, 0);
         else
           this.resize_(1, 0);
         break;
       case 38:  // up arrow
         if (event.shiftKey)
           this.move_(0, -1);
@@ skipping 43 matching lines @@
     /** @type {!chrome.system.display.Insets} */ var delta = {
       left: x,
       top: y,
       right: x,
       bottom: y,
     };
     settings.display.systemDisplayApi.overscanCalibrationAdjust(
         this.displayId, delta);
   }
 });