[Wallpaper Picker] Pressing <Escape> closes the active overlay

chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js

 // Copyright (c) 2013 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.
 
 /**
  * WallpaperManager constructor.
  *
  * WallpaperManager objects encapsulate the functionality of the wallpaper
  * manager extension.
  *
@@ skipping 256 matching lines @@
       window.addEventListener('online', function() {
         self.downloadedListMap_ = null;
         $('wallpaper-grid').classList.remove('image-picker-offline');
       });
     }
     $('window-close-button').addEventListener('click', function() {
       window.close();
     });
     this.document_.defaultView.addEventListener(
         'resize', this.onResize_.bind(this));
+    this.document_.defaultView.addEventListener(
+        'keydown', this.onKeyDown_.bind(this));
     $('learn-more').href = LearnMoreURL;
     $('close-error').addEventListener('click', function() {
       $('error-container').hidden = true;
     });
     $('close-wallpaper-selection').addEventListener('click', function() {
       $('wallpaper-selection-container').hidden = true;
       $('set-wallpaper-layout').disabled = true;
     });
 
     this.onResize_();
@@ skipping 359 matching lines @@
 
   /**
    * Resize thumbnails grid and categories list to fit the new window size.
    */
   WallpaperManager.prototype.onResize_ = function() {
     this.wallpaperGrid_.redraw();
     this.categoriesList_.redraw();
   };
 
   /**
+   * Close the active overlay on pressing the Escape key.

[bshe, 2013/08/29 16:05:03]

please document function parameter like this:
@param {Event} event A keydown event.

[François Beaufort, 2013/08/29 16:32:59]

Done.

+   */
+  WallpaperManager.prototype.onKeyDown_ = function(event) {
+    if (event.keyCode == 27) {

[bshe, 2013/08/29 16:05:03]

There might be two overlays opened at the same time, I would suggest to use:
var overlays = this.document_.querySelectorAll('.overlay-container');
for (var i = 0; i < overlays.length; i++)
  overlays[i].hidden = true;
It should hide all cases.

[François Beaufort, 2013/08/29 16:32:59]

Setting "hidden" to true is not enough since it's doing more than that for the
Custom Wallpaper Selection overlay. See line 285.
Hope it's ok this way.

On 2013/08/29 16:05:03, bshe wrote:

[bshe, 2013/08/29 17:10:35]

Aha. you are right. We can't simply hide it. Would you mind to add a short
comment?

I was mostly worry about the case that two overlays show at the same time. Error
overlay may show on top of custom wallpaper selection overlay. When esc is
pressed in this case, I originally though we should dismiss all overlays. But it
probably makes more sense to dismiss the last opened(topmost) overlay (error
overlay in this case) first. Using querySelector didn't explicitly specify the
dismiss order. However it should work since error overlay is the first match of
querySelector (implicitly has a higher dismiss order). But it will break if
someone change the order of error overlay and custom wallpaper overlay in DOM.
Correct fix may involve a bit work. I am fine with leaving a TODO or have some
kind of documentation here just in case we forget about the dismiss order issue
in the future.

+      var closeButtonSelector = '.overlay-container:not([hidden]) .close';
+      var closeButton = this.document_.querySelector(closeButtonSelector);
+      if (closeButton) {
+        closeButton.click();
+        event.preventDefault();
+      }
+    }
+  };
+
+  /**
    * Constructs the categories list.
    */
   WallpaperManager.prototype.initCategoriesList_ = function() {
     this.categoriesList_ = $('categories-list');
     cr.ui.List.decorate(this.categoriesList_);
     // cr.ui.list calculates items in view port based on client height and item
     // height. However, categories list is displayed horizontally. So we should
     // not calculate visible items here. Sets autoExpands to true to show every
     // item in the list.
     // TODO(bshe): Use ul to replace cr.ui.list for category list.
@@ skipping 319 matching lines @@
           }
         }
       }
       this.wallpaperGrid_.dataModel = wallpapersDataModel;
       this.wallpaperGrid_.selectedItem = selectedItem;
       this.wallpaperGrid_.activeItem = selectedItem;
     }
   };
 
 })();