940843 - Print preview Alt+C doesn't activate "close" button anymore (& similar for other print preview UI with alt-keys)

Status: VERIFIED FIXED

(Core :: Print Preview, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. View about:blank
 2. File | Print Preview
 3. Alt+c to activate "Close" button. (or Alt+p, Alt+u, etc, to activate other UI)

ACTUAL RESULTS: Nothing happens.
EXPECTED RESULT: Close button (or whatever you chose) should be activated.

From a local mozregression run, it appears this regressed a few months ago:
{
Last good nightly: 2013-07-25
First bad nightly: 2013-07-26

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a4c1961bf723&tochange=46d73e889cb4
}

There isn't anything print-related in that pushlog, but Masayuki landed a bunch of keypress handling stuff for bug 501496 that day -- that seems like the most likely related thing, at first glance.

Comment 1

[Daniel Holbert [:dholbert]]

(This isn't about:blank specific, of course; I just chose that as a trivial testcase)

Comment 2

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

I can reproduce this bug on Win8.1 too. I guess that print preview consumes keydown event before ESM handles accesskey if bug 501496 is the actual cause.

Comment 3

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

Looks like this consumed all keydown events while Alt key is pressed:
http://mxr.mozilla.org/mozilla-central/source/layout/printing/nsPrintPreviewListener.cpp

Comment 4

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

Access key is handled by ESM::PreHandleEvent() with keypress event without checking defaultPrevented value.

Therefore, before bug 501496, even if keydown event is consumed by content, following keypress event worked as modifier key. This means that such key event may have caused "double action".

Anyway, we need to kill only the event propagation of keydown events of printable keys. But looks like nsPrintPreviewListener approach looks hacky to me. I believe that it should be killed by PresShell level but such change is really risky and too big change.

Comment 5

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

Attachment: Patch

nsPrintPreviewListener shouldn't consume keydown event because it kills following keypress event. Instead, it should just stop propagation of keydown event both in default event group and system event group.

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4101d7df065a

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8336717 [details] [diff] [review]
Patch

The code changes looks fine to me, so r=mats if Olli thinks this is the
right fix from an event handling point of view.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f73da8ab5911

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f73da8ab5911

Comment 9

[Ashickur Rahman]

Check on Firefox Nightly 29. The fix works for me. 

[bugday-20120108]

Comment 10

[Ashickur Rahman]

[bugday-20140108]

Comment 11

[u279076]

(In reply to AShickur Rahman from comment #9)
> Check on Firefox Nightly 29. The fix works for me. 

Would you mind checking the latest Aurora build? (http://aurora.mozilla.org)

Comment 12

[Ashickur Rahman]

Checked on Aurora build 28.0a2 (2014-01-08), linux x86_64. Fix works for me.

Comment 13

[svbabukumar]

Works fine in latest aurora 28.0a2 (2014-01-21), Win7 x32

Comment 14

[svbabukumar]

work fine in Nightly build 29.0a1 (2014-01-21), Win 7 x64

Comment 15

[svbabukumar]

[bugday-20140120]

Comment 16

[svbabukumar]

[bugday-20140122] - works fine in Latest aurora and Nightly build - Win 7 x64