595518 - After focusing the icon in the right-top corner in tabcandy, trying to activate tabcandy afterwards goes imediately back to normal view

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I don't know what the icon is for, but seems like it somehow exists the
tabcandy view. 
But the focus also stays in that icon, so that if I afterwards try to go to 
TabCandy, TabCandy is activated by it exits immediately.

Sounds like the icon was tested only on OSX, where focus handling is 
quite different (and IMO OSX in general has pretty insane focus handling).

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: V1

This will require the patch for bug 595191 to land first because this patch uses the blurAll method

Comment 2

[Raymond Lee [:raymondlee]]

Pushed to try and waiting for the results

Comment 3

[Raymond Lee [:raymondlee]]

Passed try

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 475031 [details] [diff] [review]
V1

+    if (navigator.platform.indexOf("Mac") != -1) {
+      charCode = 160;
+      eventObject = { altKey: true };
+    } else {
+      charCode = 32;
+      eventObject = { ctrlKey: true };
+    }

Use KeyEvent.DOM_VK_SPACE instead of the 32, and comment the 160 to make it clear that that's the character code for option + space. 

Have you verified this fix on Windows?

I'm a little confused by this... isn't it just space we're looking for? Or is option space a different character code? Also, shouldn't we be using

Comment 5

[Ian Gilman [:iangilman]]

(In reply to comment #4) 
> I'm a little confused by this... isn't it just space we're looking for? Or is
> option space a different character code? Also, shouldn't we be using

Please ignore these lines; earlier draft... clearly I figured it out. :)

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v2

I've tested this patch on Windows 7 and it worked fine.

Comment 7

[Raymond Lee [:raymondlee]]

Comment on attachment 475120 [details] [diff] [review]
v2

It requires the patch for bug 595191 to land first because it
depends on the blurAll method

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 475120 [details] [diff] [review]
v2

r=me. before landing, please add some commentary to the test.

Comment 9

[Ian Gilman [:iangilman]]

Attachment: patch v3 (ready for check in, needs to land after 595191)

Updated patch with comments, as well as r+ in commit message. Also, the test had the wrong bug number (595191); fixed.

Comment 10

[Ian Gilman [:iangilman]]

Also note that this patch still requires bug 595191 to land first.

Comment 11

[Johnathan Nightingale [:johnath]]

You can clear the approval request - marked blocking betaN+

Comment 12

[Ian Gilman [:iangilman]]

Attachment: patch for check-in [r=dietrich, a=blocking]

Updated patch with comment message, and remerged with 595191.

Comment 13

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/633ec4ed3cb2

Comment 15

[Tracy Walker [:tracy]]

verified on recent nightly builds of minefield