Closed Bug 966612 Opened 6 years ago Closed 6 years ago

Escape key should exit Print Preview

Categories

(Toolkit :: Printing, defect)

25 Branch
x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox26 --- affected
firefox27 --- affected
firefox28 --- affected
firefox29 --- affected
firefox-esr24 --- unaffected

People

(Reporter: peter, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
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
Blocks: 501496
Status: UNCONFIRMED → NEW
Component: Untriaged → Event Handling
Ever confirmed: true
Keywords: regression
OS: Linux → All
Product: Firefox → Core
Version: 26 Branch → 25 Branch
Hmm, I'm surprised that nobody has already reported this!?
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: Event Handling → Printing
Product: Core → Toolkit
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).
Attachment #8369332 - Flags: review?(gavin.sharp)
Comment on attachment 8369333 [details] [diff] [review]
part.2 Add tests on printpreview UI

Test for this bug and bug 940843.
Attachment #8369333 - Flags: review?(gavin.sharp)
Attachment #8369332 - Flags: review?(gavin.sharp) → review?(enndeakin)
Attachment #8369333 - Flags: review?(gavin.sharp) → review?(enndeakin)
Duplicate of this bug: 967408
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();
  }
}
Attachment #8369332 - Flags: review?(enndeakin) → review-
Attachment #8369333 - Flags: review?(enndeakin) → review+
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.
Attachment #8370550 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/mozilla-central/rev/5959f80e1831
https://hg.mozilla.org/mozilla-central/rev/993ddadafaea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.