463802 - "keyup" event for Ctrl key not dispatched when focusing the Ctrl+Tab panel

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Dão Gottwald [:dao]]

The keypress event for Tab (with event.ctrlKey == true) opens the Ctrl+Tab panel. Releasing the Ctrl key should close it again. This doesn't happen if focus was moved to the panel. This can be tested with the patch in bug 463799.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I'm afraid I don't understand.

Could you give a more detailed description (and STR)?

Comment 2

[Dão Gottwald [:dao]]

STR:
With the patch in bug 463799, hit Ctrl+Tab, release Tab, release Ctrl.

Expected:
Different tab selected

Actual:
No different tab selected and the Ctrl+Tab panel stays open. (At least, that used to be the case, see e.g. the last paragraph of bug 436304 comment 77.)

Comment 3

[Dão Gottwald [:dao]]

See also bug 465076 comment 12 and bug 465076 comment 13.

Comment 4

[Josh Aas]

blocking1.9.1+ for now, we need to sort this out

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Turns out this *is* a Cocoa widgets bug ... or at least that it's
triggered by one.  Here's a patch that fixes it.

The keyboard modifier state is global (not per-view or per-window).
So the variable that stores the previous modifier state should also be
global.

Prior to my patch, mLastModifierState was per-ChildView.  But the
ChildView that has the keyboard focus when the Control key is first
pressed is different from the ChildView that has the keyboard focus
when the Control key is released (after the Tab key has been pressed
and released).  So the value of mLastModifierState (for the currently
focused ChildView) is '0' when the Control key is released, when it
should be 0x00040000 (== NSControlKeyMask).  This means
fireKeyEventForFlagsChanged doesn't get called when the Control key is
released.

A tryserver build of this patch plus the patch for bug 463799 will
follow in a bit.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 352331 [details] [diff] [review]
Fix

Masayuki, it seems you're the last one who touched this code.  So I'm
also asking you to review my patch.

Comment 7

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

Comment on attachment 352331 [details] [diff] [review]
Fix

Looks ok for now... However, should not we initialize sLastModifierStat at every application activating time?

E.g., if Ctrl or something is down on another application and switched the active application to us by mouse click, then, we cannot handle the Ctrl keyup event correctly. But it's really rare case, and it can be separated to another bug.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #7)

> However, should not we initialize sLastModifierStat at every
> application activating time?

Good point.  But if flagsChanged: is called (by the OS) on a ChildView
when the browser isn't active, we might not have to explicitly set
sLastModifierState when the browser becomes active.

I'll look into this further (later this afternoon).

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (follow Masayuki's suggestion)

Here's another patch that incorporates Masayuki's suggestion ... which
wasn't difficult.

There's still one (very minor) problem:  If you hold down the Option
key while making Firefox active (by left-clicking on a browser
window), gLastModifierState gets set to '0' in [AppShellDelegate
applicationDidBecomeActive:] (instead of to 0x00080000 as it should).
This is presumably some kind of OS bug.  But I don't think it matters
(I can't think of a scenario where it does matter).

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #8)

> But if flagsChanged: is called (by the OS) on a ChildView when the
> browser isn't active,

Forgot to mention that I did some tests and never saw [ChildView
flagsChanged:] called while the browser was inactive.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

A tryserver build of my rev1 patch plus the patch for bug 463799 will
follow in several hours.  The Mac tryserver is a bit slooooow today.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Combo of rev1 patch and bug 463799 patch (for debugging)

And here's a tryserver build:

https://build.mozilla.org/tryserver-builds/2008-12-10_16:[e-mail]-bugzilla463802-rev1-debug/[e-mail]

Comment 13

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

Comment on attachment 352437 [details] [diff] [review]
Fix rev1 (follow Masayuki's suggestion)

Great!

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central

http://hg.mozilla.org/mozilla-central/rev/4497c86397f4

Comment 15

[Marcia Knous [:marcia]]

Hi Steven: Since this is plussed for 3.1, when will it get checked into that branch? I am just back from PTO and trying to figure out the workflow for these types of bugs.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

I'm hoping to do that right now, Marcia :-)

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Landed on the 1.9.1 branch:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ad98cc73f7c

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't see a way how to verify the fix. The STR from comment 0 are also working with a 081215 trunk nightly. Do we have another STR which are more visible?

Comment 19

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #18)

Unfortunately, this bug's STR (from comment #2) only "works" in a
build that also has the patch for bug 463799.

So to see this bug (unfixed) you'll need to create a build with the
patch for bug 463799 but without this bug's patch.

Comment 20

[Nochum Sossonko [:Natch]]

Anyone up to the verifying task? :p

Comment 21

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old