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

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

BUG=281038
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220645

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

Awesome. Thanks for fixing it!
Just have two comments.

https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:658:
* Close the active overlay on pressing the Escape key.
please document function parameter like this:
@param {Event} event A keydown event.

https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:661:
if (event.keyCode == 27) {
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:58]

Addressed your feedback hopefully well.

https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:658:
* Close the active overlay on pressing the Escape key.
On 2013/08/29 16:05:03, bshe wrote:
> please document function parameter like this:
> @param {Event} event A keydown event.

Done.

https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:661:
if (event.keyCode == 27) {
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:
> 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.

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

https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:661:
if (event.keyCode == 27) {
On 2013/08/29 16:32:59, François Beaufort wrote:
> 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:
> > 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.
> 

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.

[François Beaufort, 2013-08-30 12:09:30]

TODO and comment have been added.
From now on, we only close the last opened overlay.

On 2013/08/29 17:10:35, bshe wrote:
>
https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
> File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
> (right):
> 
>
https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:661:
> if (event.keyCode == 27) {
> On 2013/08/29 16:32:59, François Beaufort wrote:
> > 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:
> > > 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.
> > 
> 
> 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.

[bshe, 2013-08-30 14:07:41]

On 2013/08/30 12:09:30, François Beaufort wrote:
> TODO and comment have been added.
> From now on, we only close the last opened overlay.
> 
> On 2013/08/29 17:10:35, bshe wrote:
> >
>
https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
> > File
> chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/23514006/diff/6001/chrome/browser/resources/c...
> >
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:661:
> > if (event.keyCode == 27) {
> > On 2013/08/29 16:32:59, François Beaufort wrote:
> > > 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:
> > > > 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.
> > > 
> > 
> > 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.

lgtm

[commit-bot: I haz the power, 2013-08-30 14:10:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/23514006/1...

[commit-bot: I haz the power, 2013-08-30 14:24:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaufort.francois@gmail.com/23514006/1...

[commit-bot: I haz the power, 2013-08-30 20:40:33]

Change committed as 220645