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_);
-  },
-
-  /** @override */
-  detached: function() {
-    window.removeEventListener('keydown', this.keyHandler_);
-  },
-
-  open: function() {
+    this.addEventListener('keydown', this.keyHandler_);
     this.comitted_ = false;
     this.$.dialog.showModal();
   },
 
   close: function() {
+    this.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 16 matching lines @@
         this.displayId);
     this.comitted_ = true;
     this.close();
   },
 
   /**
    * @param {Event} event
    * @private
    */
   handleKeyEvent_: function(event) {
+    if (event.altKey || event.ctrlKey || event.metaKey)
+      return;
     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);
         else
           this.resize_(0, -1);
         break;
       case 39:  // right arrow
         if (event.shiftKey)
           this.move_(1, 0);
         else
           this.resize_(-1, 0);
         break;
       case 40:  // down arrow
         if (event.shiftKey)
           this.move_(0, 1);
         else
           this.resize_(0, 1);
         break;
       default:
         return;
     }
     event.preventDefault();

[Dan Beam, 2016/09/28 18:15:45]

you don't really want to do this either.  I think you should only call
preventDefault() in the case that one of your case ##: statements match (and an
action is taken).

preventDefault() more-or-less means "this key event is something i already
handled because it doesn't something useful in my part of this UI"

[stevenjb, 2016/09/28 18:21:29]

Thus the 'default: return;' ...

[Dan Beam, 2016/09/28 18:31:15]

ah, OK

if it's a useful signal: I obviously didn't see that on first pass (but the code
is correct and maybe I just wasn't paying enough attention).

do you know for sure that all the move_ and resize_ calls will actually work?

[stevenjb, 2016/09/28 18:35:30]

I've tested the code if that's what you're asking? I'm not sure I understand the
question.

   },
 
   /**
    * @param {number} x
    * @param {number} y
    * @private
    */
   move_: function(x, y) {
     /** @type {!chrome.system.display.Insets} */ var delta = {
       left: x,
@@ skipping 14 matching lines @@
     /** @type {!chrome.system.display.Insets} */ var delta = {
       left: x,
       top: y,
       right: x,
       bottom: y,
     };
     settings.display.systemDisplayApi.overscanCalibrationAdjust(
         this.displayId, delta);
   }
 });