Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9663037:
Print Preview: Pass direction keys to the PDF viewer when possible. (Closed)
|
Created:
8 years, 9 months ago by Lei Zhang Modified:
8 years, 9 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionPrint Preview: Pass direction keys to the PDF viewer when possible.
BUG=116275
TEST=see bug.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126217
Patch Set 1 : #
Total comments: 25
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 4
Patch Set 5 : fix all the nits! #Messages
Total messages: 14 (0 generated)
LGTM. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... File chrome/browser/resources/print_preview/print_preview.js (right): https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:234: * @param {MouseEvent} e The mouse event. extra space in front. nit optional: Since this is a one line function could probably be an anonymous function at line 156. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1136: (e.keyCode >= 37 && e.keyCode <= 40))) { Nit: Indentation should be 4 spaces?
https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... File chrome/browser/resources/print_preview/print_preview.js (right): https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:234: * @param {MouseEvent} e The mouse event. 1 less space nit: Generally we say Event instead of MouseEvent so if that changes the doc isn't stale (IIRC). https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1119: window.onkeydown = null; why don't you change to using addEventListener()/detachEventListener() or just remove this? (I don't see why you'd care if the tab is being destroyed...) https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1126: * @param {KeyboardEvent} e The keyboard event. nit: I'd say same here, but apparently is mattered a bit in this case so up to you... https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1127: * @return {boolean} true if the keyboard event has been handled. nit: s/true i/I/ https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1148: nit: remove \n https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1174: * @return {boolean} true to allow the browser to handle the key, else false. * @return {boolean} Whether the key was handled. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1193: return !tryToHandleDirectionKeyDown(e); who actually listens to this return value?
https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... File chrome/browser/resources/print_preview/print_preview.js (right): https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:234: * @param {MouseEvent} e The mouse event. On 2012/03/10 01:14:26, Dan Beam wrote: > 1 less space > nit: Generally we say Event instead of MouseEvent so if that changes the doc > isn't stale (IIRC). Ok, but this is intended to be for mouse events only. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1119: window.onkeydown = null; On 2012/03/10 01:14:26, Dan Beam wrote: > why don't you change to using addEventListener()/detachEventListener() or just > remove this? (I don't see why you'd care if the tab is being destroyed...) I don't want any other events to get triggered after this point. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1126: * @param {KeyboardEvent} e The keyboard event. On 2012/03/10 01:14:26, Dan Beam wrote: > nit: I'd say same here, but apparently is mattered a bit in this case so up to > you... Done. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1127: * @return {boolean} true if the keyboard event has been handled. On 2012/03/10 01:14:26, Dan Beam wrote: > nit: s/true i/I/ Done. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1136: (e.keyCode >= 37 && e.keyCode <= 40))) { On 2012/03/10 01:14:00, dpapad wrote: > Nit: Indentation should be 4 spaces? Done. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1148: On 2012/03/10 01:14:26, Dan Beam wrote: > nit: remove \n Done. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1174: * @return {boolean} true to allow the browser to handle the key, else false. On 2012/03/10 01:14:27, Dan Beam wrote: > * @return {boolean} Whether the key was handled. Isn't it actually the opposite? https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1193: return !tryToHandleDirectionKeyDown(e); On 2012/03/10 01:14:27, Dan Beam wrote: > who actually listens to this return value? The renderer. It's like the return value in: <a href="http://foo.com" onclick="alert('hi'); return false;">click does not navigate</a>
https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... File chrome/browser/resources/print_preview/print_preview.js (right): https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:234: * @param {MouseEvent} e The mouse event. On 2012/03/10 01:35:45, Lei Zhang wrote: > On 2012/03/10 01:14:26, Dan Beam wrote: > > 1 less space > > nit: Generally we say Event instead of MouseEvent so if that changes the doc > > isn't stale (IIRC). > > Ok, but this is intended to be for mouse events only. That's fine, though pressing space or enter also trigger a click, btw (and this is convincing your user otherwise by being called a MouseEvent, but that's a different problem). https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1119: window.onkeydown = null; On 2012/03/10 01:35:45, Lei Zhang wrote: > On 2012/03/10 01:14:26, Dan Beam wrote: > > why don't you change to using addEventListener()/detachEventListener() or just > > remove this? (I don't see why you'd care if the tab is being destroyed...) > > I don't want any other events to get triggered after this point. removeEventListener** is cleaner, IMO, as maybe some day people want to trigger synthetic events or call this function ad-hoc, ;), and now you've just set that function to null. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1174: * @return {boolean} true to allow the browser to handle the key, else false. On 2012/03/10 01:35:45, Lei Zhang wrote: > On 2012/03/10 01:14:27, Dan Beam wrote: > > * @return {boolean} Whether the key was handled. > > Isn't it actually the opposite? See below (you shouldn't have to return anything). https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1193: return !tryToHandleDirectionKeyDown(e); On 2012/03/10 01:35:45, Lei Zhang wrote: > On 2012/03/10 01:14:27, Dan Beam wrote: > > who actually listens to this return value? > > The renderer. It's like the return value in: <a href="http://foo.com" > onclick="alert('hi'); return false;">click does not navigate</a> use e.preventDefault() instead either here or inside the method, IMO (this can still be tested as well)
https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... File chrome/browser/resources/print_preview/print_preview.js (right): https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:234: * @param {MouseEvent} e The mouse event. On 2012/03/10 01:57:33, Dan Beam wrote: > On 2012/03/10 01:35:45, Lei Zhang wrote: > > On 2012/03/10 01:14:26, Dan Beam wrote: > > > 1 less space > > > nit: Generally we say Event instead of MouseEvent so if that changes the doc > > > isn't stale (IIRC). > > > > Ok, but this is intended to be for mouse events only. > > That's fine, though pressing space or enter also trigger a click, btw (and this > is convincing your user otherwise by being called a MouseEvent, but that's a > different problem). Ok, I changed this to just "click event". https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1119: window.onkeydown = null; On 2012/03/10 01:57:33, Dan Beam wrote: > On 2012/03/10 01:35:45, Lei Zhang wrote: > > On 2012/03/10 01:14:26, Dan Beam wrote: > > > why don't you change to using addEventListener()/detachEventListener() or > just > > > remove this? (I don't see why you'd care if the tab is being destroyed...) > > > > I don't want any other events to get triggered after this point. > > removeEventListener** is cleaner, IMO, as maybe some day people want to trigger > synthetic events or call this function ad-hoc, ;), and now you've just set that > function to null. Done. https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1193: return !tryToHandleDirectionKeyDown(e); On 2012/03/10 01:57:33, Dan Beam wrote: > On 2012/03/10 01:35:45, Lei Zhang wrote: > > On 2012/03/10 01:14:27, Dan Beam wrote: > > > who actually listens to this return value? > > > > The renderer. It's like the return value in: <a href="http://foo.com" > > onclick="alert('hi'); return false;">click does not navigate</a> > > use e.preventDefault() instead either here or inside the method, IMO (this can > still be tested as well) Done.
http://codereview.chromium.org/9663037/diff/2003/chrome/browser/resources/pri... File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/9663037/diff/2003/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:1119: window.removeEventListener('onkeydown', onKeyDown); Are you sure this should not be 'keydown'?
http://codereview.chromium.org/9663037/diff/2003/chrome/browser/resources/pri... File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/9663037/diff/2003/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:1119: window.removeEventListener('onkeydown', onKeyDown); On 2012/03/10 02:28:22, dpapad wrote: > Are you sure this should not be 'keydown'? I'm sure it should be just 'keydown' http://codereview.chromium.org/9663037/diff/2003/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:1127: */ /** * Pass certain directional keyboard events to the PDF viewer. * @param {Event} e The keydown event. */ http://codereview.chromium.org/9663037/diff/2003/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:1145: if (tagName == "INPUT" || tagName == "SELECT" || tagName == "EMBED") s/"/'/ (single quotes in js) http://codereview.chromium.org/9663037/diff/2003/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:1184: window.detachEventListener('onkeydown', onKeyDown); 'keydown' here as well, sorry to confuse, but removeEventListener is what you want (IE uses detachEvent) http://codereview.chromium.org/9663037/diff/2003/chrome/browser/resources/pri... chrome/browser/resources/print_preview/print_preview.js:1194: window.addEventListener('onkeydown', onKeyDown); 'keydown'
Ok, see patch set 4.
https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... File chrome/browser/resources/print_preview/print_preview.js (right): https://chromiumcodereview.appspot.com/9663037/diff/2001/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1136: (e.keyCode >= 37 && e.keyCode <= 40))) { On 2012/03/10 01:35:45, Lei Zhang wrote: > On 2012/03/10 01:14:00, dpapad wrote: > > Nit: Indentation should be 4 spaces? > > Done. Basically undoing this. https://chromiumcodereview.appspot.com/9663037/diff/3004/chrome/browser/resou... File chrome/browser/resources/print_preview/print_preview.js (right): https://chromiumcodereview.appspot.com/9663037/diff/3004/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1127: */ :1125,1127s/^ // https://chromiumcodereview.appspot.com/9663037/diff/3004/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1135: (e.keyCode >= 37 && e.keyCode <= 40))) { :1135s/^/ / to show that this is a sub expression
lgtm w/nits
https://chromiumcodereview.appspot.com/9663037/diff/3004/chrome/browser/resou... File chrome/browser/resources/print_preview/print_preview.js (right): https://chromiumcodereview.appspot.com/9663037/diff/3004/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1127: */ On 2012/03/10 02:58:51, Dan Beam wrote: > :1125,1127s/^ // Done. https://chromiumcodereview.appspot.com/9663037/diff/3004/chrome/browser/resou... chrome/browser/resources/print_preview/print_preview.js:1135: (e.keyCode >= 37 && e.keyCode <= 40))) { On 2012/03/10 02:58:51, Dan Beam wrote: > :1135s/^/ / to show that this is a sub expression Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/9663037/5007
Change committed as 126217 |
