567605 - Jetpack widget bar won't hide/show on ctrl+shift+u

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Erik Vold]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: 

With http://github.com/erikvold/gtranslate-it-jetpack, on my mac, I tried `cfx run -a firefox -b nightly-loc` and that worked, so I ran `cfx xpi` then uploaded it to AMO here: https://addons.mozilla.org/en-US/firefox/addon/161922/ and installed it on my FF 3.6 profile and cmd+shift+u worked, then I switched to the same profile on minefield and cmd+shift+u did not work. Then I installed the xpi on my windows machine and the same thing, ctrl+shift+u worked for FF 3.6 but not the nightly. I also cloned my repo above on the windows machine, then ran `cfx run -a firefox -b nightly-loc` and that works fine..

Reproducible: Always

Steps to Reproduce:
1. Install https://addons.mozilla.org/en-US/firefox/addon/161922/ on minefield with an existing profile.
2. cmd/ctrl+shift+u does not seem to work
Actual Results:  
the bottom panel does not show/hide when the cmd/ctrl+shift+u hotkey is pressed

Expected Results:  
the bottom panel should show/hide when the cmd/ctrl+shift+u hotkey is pressed

Comment 1

[Erik Vold]

I also disabled all of my other addons on the mac osx profile but that didn't help.

Comment 2

[Dietrich Ayala (:dietrich)]

confirmed on trunk, taking.

i don't think we need to block 0.4 on this, since we show the UI by default now. cc'ing myk for his consideration of this.

Comment 3

[Dietrich Ayala (:dietrich)]

hm, it might be that adding the keyboard shortcut via xul doesn't work if added post-window load. if so, this would've only manifested after the rebootless support landed.

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: v1

stop using xul altogether for this.

- works on trunk and 3.6, rebootless or no
- better for cross-app support

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 446965 [details] [diff] [review]
v1

>+  _onKeyPress: function BW__onKeyPress(aEvent) {
>+    if (aEvent.charCode == 85 && aEvent.shiftKey &&
>+        aEvent.ctrlKey)
>+      this._onToggleUI();

This should check that !aEvent.metaKey && !aEvent.altKey.  Otherwise, Ctrl+Shift+(Alt&|Meta)-u will also work.

Also, it's unclear what happens when charCode is not provided (because the key pressed was a non-character key), so it would be better to check aEvent.which, which is always provided.

And it probably makes sense to compare with KeyEvent.DOM_VK_U rather than the hardcoded value.

Finally, this changes the shortcut on Mac from Command+Shift+u to Control+Shift+u.  That's ok, if a bit unconventional (most Firefox keyboard shortcuts seem to be Command-based, whether or not they include Shift, although a few use Control).  But the docs need to be updated to take this into account.

Alternately, check window.navigator.platform, and if it's MacPPC or MacIntel, then test for aEvent.metaKey instead of aEvent.ctrlKey to keep the shortcut Command+Shift+u on the Mac.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #5)
> (most Firefox keyboard shortcuts seem to be Command-based, whether or not they
> include Shift, although a few use Control)

Erm, I meant to say most Firefox keyboard shortcuts *on Mac* seem to be Command-based.

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #5)
> Finally, this changes the shortcut on Mac from Command+Shift+u to
> Control+Shift+u.  That's ok, if a bit unconventional (most Firefox keyboard
> shortcuts seem to be Command-based, whether or not they include Shift, although
> a few use Control).  But the docs need to be updated to take this into account.

Bah, no I meant to look up how to detect command, and forgot.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: v2

- comments fixed
- added tests

tested on Mac and Linux (should be enough, the special case was Mac).

Comment 9

[Myk Melez [:myk] [@mykmelez]]

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

>+  _onKeyPress: function BW__onKeyPress(aEvent) {
>+    let accelKey = /^Mac/.test(this.window.navigator.platform) ?
>+                   aEvent.metaKey : aEvent.ctrlKey;
>+    if (aEvent.which == aEvent.DOM_VK_U && aEvent.shiftKey &&
>+        accelKey && !aEvent.altKey)
>+      this._onToggleUI();

Nit this should also do something like:

    let nonAccelKey = /^Mac/.test(this.window.navigator.platform) ?
                      aEvent.ctrlKey : aEvent.metaKey;

So it can do:

    if (aEvent.which == aEvent.DOM_VK_U && aEvent.shiftKey &&
        accelKey && !nonAccelKey && !aEvent.altKey)

Otherwise, this looks great.  However, it doesn't work on trunk nightlies on Mac, although it does work on 3.6.3 on the Mac, and it works on both trunk and 3.6.3 on Linux & Windows.  A bit of digging suggests that the keypress event is firing twice on trunk Mac (although it only fires once for "u", Shift+U, and Command+U), causing the container to toggle one way and then back the other.

This seems like a platform bug, not a Jetpack-specific issue, and I have no idea how to work around it (special case trunk Mac and throw away the second event?), and the behavior with this patch is till better than the behavior without it, so r=myk, but we should get a bug filed on the wonky behavior (if there isn't one filed already).

Comment 10

[Dietrich Ayala (:dietrich)]

Hrm, I just tested on the latest Mac nightly and it's working for me.

Comment 11

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/labs/jetpack-sdk/rev/10dae1cded21

Comment 12

[Hernán Rodriguez Colmeiro (:peregrino)]

I can reproduce the bug again on latest minefield in OSX 10.6.

BTW, it work's OK in Firefox 3.6.7 on OSX, and in Firefox 3.6.7 and Minefield on Linux (Ubuntu 10.04).

Comment 13

[Hernán Rodriguez Colmeiro (:peregrino)]

It seems that the onKeyPress handler is being triggered twice in latest OSX Minefield (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3pre) Gecko/20100724 Minefield/4.0b3pre), so the bar is hidden and shown at the same time.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: take 2, patch v1: fixes problem

(In reply to comment #13)
> It seems that the onKeyPress handler is being triggered twice in latest OSX
> Minefield (Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3pre)
> Gecko/20100724 Minefield/4.0b3pre), so the bar is hidden and shown at the same
> time.

Yup, that's exactly right.  I filed bug 582052 on the underlying platform problem, and I added it to the list of platform dependencies <https://wiki.mozilla.org/Labs/Jetpack/Dependencies>.  I also found a workaround--prevent the default action via event.preventDefault()--and am attaching a patch that uses it to fix the bug.

Dietrich is on vacation, so asking Drew for review.

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 460322 [details] [diff] [review]
take 2, patch v1: fixes problem

Works great, r=adw!  Thanks for investigating.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Thanks, Drew, for the quick review!  a=self for checkin during freeze, and fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/407403189df3.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".