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

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

BUG=631522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9235cda388a249ed4149b7b3f36b2c06b17ccc52
Cr-Commit-Position: refs/heads/master@{#421729}

[stevenjb, 2016-09-26 22:34:03]

Description was changed from

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

BUG=631522
==========

to

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

BUG=631522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-09-26 22:35:16]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[stevenjb, 2016-09-26 22:35:55]

stevenjb@chromium.org changed reviewers:
+ dschuyler@chromium.org

[stevenjb, 2016-09-26 22:35:55]

+dschuyler@ since michaelpg@ may be out.

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

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-09-27 00:04:53]

https://codereview.chromium.org/2371993002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display_overscan_dialog.js
(right):

https://codereview.chromium.org/2371993002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:35:
window.addEventListener('keydown', this.keyHandler_);
why are we using window.addEventListener() rather than a more narrow scope like
this.addEventListener()?

[dschuyler, 2016-09-27 23:45:25]

https://codereview.chromium.org/2371993002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display_overscan_dialog.js
(right):

https://codereview.chromium.org/2371993002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:78:
handleKeyEvent_: function(event) {
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]

PTAL

https://codereview.chromium.org/2371993002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display_overscan_dialog.js
(right):

https://codereview.chromium.org/2371993002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:35:
window.addEventListener('keydown', this.keyHandler_);
On 2016/09/27 00:04:52, Dan Beam wrote:
> why are we using window.addEventListener() rather than a more narrow scope
like
> this.addEventListener()?

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

Changed.

https://codereview.chromium.org/2371993002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:78:
handleKeyEvent_: function(event) {
On 2016/09/27 23:45:24, dschuyler wrote:
> 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;

Done.

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

https://codereview.chromium.org/2371993002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_overscan_dialog.js
(right):

https://codereview.chromium.org/2371993002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:109:
event.preventDefault();
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:30]

https://codereview.chromium.org/2371993002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_overscan_dialog.js
(right):

https://codereview.chromium.org/2371993002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:109:
event.preventDefault();
On 2016/09/28 18:15:45, Dan Beam wrote:
> 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"

Thus the 'default: return;' ...

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

https://codereview.chromium.org/2371993002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_overscan_dialog.js
(right):

https://codereview.chromium.org/2371993002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:109:
event.preventDefault();
On 2016/09/28 18:21:29, stevenjb (OOO 9-28 - 10-12) wrote:
> On 2016/09/28 18:15:45, Dan Beam wrote:
> > 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"
> 
> Thus the 'default: return;' ...

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:31]

https://codereview.chromium.org/2371993002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_overscan_dialog.js
(right):

https://codereview.chromium.org/2371993002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:109:
event.preventDefault();
On 2016/09/28 18:31:15, Dan Beam wrote:
> On 2016/09/28 18:21:29, stevenjb (OOO 9-28 - 10-12) wrote:
> > On 2016/09/28 18:15:45, Dan Beam wrote:
> > > 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"
> > 
> > Thus the 'default: return;' ...
> 
> 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?

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

[dschuyler, 2016-09-28 19:20:36]

lgtm

[stevenjb, 2016-09-28 20:16:01]

Hopefully this is better / more clear?

PTAL

[Dan Beam, 2016-09-28 22:10:38]

lgtm with either patchset #2 or #3 but I think just adding a comment to #2 as i
mentioned inline might be better

I appreciate your sincere gesture to include my review feedback, but making a
method always return true is kinda odd to me.

I didn't know that resize_ and move_ are basically guaranteed to always work.  

Often up/down/left/right arrow keys only work until you run out of stuff to do.

If you scroll up in a <textarea> and reach the top, for instance, and keep
pressing up, it'll scroll the main document instead.  This would be an example
of when you'd only want to preventDefault() when your local code has taken an
action.  move_ or resize_ seem to always take an action (or the extension API
doesn't let you know if it had an effect, either one), so always preventing
default for arrows and adding a comment for when it's not an arrow seems the
best to me.

again, lgtm either way

https://codereview.chromium.org/2371993002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_overscan_dialog.js
(left):

https://codereview.chromium.org/2371993002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_overscan_dialog.js:111:
default:
// Only preventDefault() for keys that do something (i.e. arrow keys).

[stevenjb, 2016-09-28 22:24:49]

On 2016/09/28 22:10:38, Dan Beam wrote:
> lgtm with either patchset #2 or #3 but I think just adding a comment to #2 as
i
> mentioned inline might be better
> 
> I appreciate your sincere gesture to include my review feedback, but making a
> method always return true is kinda odd to me.
> 
Yeah, I agree. Switch statements kind of suck in general. I'll just revert to
the previous patch and add the suggested comment.

> I didn't know that resize_ and move_ are basically guaranteed to always work. 

> 
> Often up/down/left/right arrow keys only work until you run out of stuff to
do.
> 
> If you scroll up in a <textarea> and reach the top, for instance, and keep
> pressing up, it'll scroll the main document instead.  This would be an example
> of when you'd only want to preventDefault() when your local code has taken an
> action.  move_ or resize_ seem to always take an action (or the extension API
> doesn't let you know if it had an effect, either one), so always preventing
> default for arrows and adding a comment for when it's not an arrow seems the
> best to me.
> 

Ah, now I understand your question. There is a limit, i.e. a point where move
anyway may not do anything, but in this case, you really wouldn't want to, e.g.,
scroll the page if you reach the limit, so we always want to prevent the default
action.

[stevenjb, 2016-09-28 22:28:02]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-09-28 22:28:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2371993002/#ps60001
(title: "Add comment")

[commit-bot: I haz the power, 2016-09-28 22:28:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaelpg, 2016-09-28 23:11:59]

lgtm2

[commit-bot: I haz the power, 2016-09-29 00:29:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 00:29:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2016-09-29 02:19:40]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-09-29 02:19:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 02:52:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-29 02:55:51]

Description was changed from

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

BUG=631522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

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

BUG=631522
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9235cda388a249ed4149b7b3f36b2c06b17ccc52
Cr-Commit-Position: refs/heads/master@{#421729}
==========

[commit-bot: I haz the power, 2016-09-29 02:55:53]

Patchset 4 (id:??) landed as
https://crrev.com/9235cda388a249ed4149b7b3f36b2c06b17ccc52
Cr-Commit-Position: refs/heads/master@{#421729}