966612 - Escape key should exit Print Preview

Status: RESOLVED FIXED

(Toolkit :: Printing, defect)

Description

[peter]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206152142

Steps to reproduce:

step 1. Click 'Print preview' > shows preview screen.
step 2. Press Esc > 


Actual results:

step 1. preview screen shown
step 2. no response


Expected results:

step 2. should close preview screen

Comment 1

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/12c11406ed75
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130724 Firefox/25.0 ID:20130724230740
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/949a5e125a8d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130724 Firefox/25.0 ID:20130724230940
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=12c11406ed75&tochange=949a5e125a8d

Regressed by: Bug 501496

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I'm surprised that nobody has already reported this!?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Print preview should handle Esc key with keydown event

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Add tests on printpreview UI

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=6f49d4cf2b00

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8369332 [details] [diff] [review]
part.1 Print preview should handle Esc key with keydown event

Non-printable key should be handled at keydown event because preventDefault() of non-printable key's keydown event is called by nsPrintPreviewListener, then, following keypress event won't be fired.

On the other hand, charCode is set only when the event is keypress event. Therefore, PrintUtils needs to handle both keydown event and keypress event. The former is for handling Esc key (non-printable key). The latter is for Ctrl + "w" (printable key).

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8369333 [details] [diff] [review]
part.2 Add tests on printpreview UI

Test for this bug and bug 940843.

Comment 9

[Neil Deakin]

Comment on attachment 8369332 [details] [diff] [review]
part.1 Print preview should handle Esc key with keydown event

I'm not clear why the two event handlers are combined. The keypress event listener could be left as is (or the escape key part removed) and then just add a new keydown listener:

onKeyDown: function(aEvent)
{
  if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE) {
    PrintUtils.exitPrintPreview();
  }
}

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Print preview should handle Esc key with keydown event

Comment 11

[Neil Deakin]

Comment on attachment 8370550 [details] [diff] [review]
part.1 Print preview should handle Esc key with keydown event

+      // XXX Why don't we call aEvent.preventDefault() here?
+    }

Likely because ctrl+w needs to be prevented to stop the window from closing, but escape doesn't have any default behaviour or listeners higher up.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5959f80e1831
https://hg.mozilla.org/integration/mozilla-inbound/rev/993ddadafaea

Thank you. I removed the XXX comment at landing.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5959f80e1831
https://hg.mozilla.org/mozilla-central/rev/993ddadafaea