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

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

Index: chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
diff --git a/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js b/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
index be090ff18d4d8c9a55d477dd671deab3cad8a174..9c895ec1daa24e280ba1aba21d8733098a88290d 100644
--- a/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
+++ b/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
@@ -274,6 +274,8 @@ function WallpaperManager(dialogDom) {
     });
     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;
@@ -653,6 +655,20 @@ function WallpaperManager(dialogDom) {
   };
 
   /**
+   * 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() {