587276 - KeyEvent in TabCandy window affect browser

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre)
Gecko/20100813 Minefield/4.0b4pre ID:20100813041308
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre)
Gecko/20100813 Minefield/4.0b4pre ID:20100813041308

Keypress Ctrl++,Ctrl+- and Crtl+0 in TabCandy window affect Zoom level of current Tab.
I think these keyevent should be ignored,


Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open TabView
3. Keypress Ctrl++Ctrl++Ctrl++Ctrl++
4. Exit TabView

Actual Results:
 Zoom level of last current tab changed.

Expected Results:
 Should not

Comment 1

[Alice0775 White]

Related bug: Ctrl+F in TabCandy , and then return to browser, Find toolbar activated.

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Nice catch Alice.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Not a blocker, kinda strange, though!

Comment 6

[Daniel Holbert [:dholbert]]

Note: Bug 608774 is a more serious version of this -- Ctrl+W currently "works" in Panorama, when it shouldn't.  (It closes you last active tab, followed by every other tab in its tab-group, and then every tab in some other tab-group, in a non-user-controllable order).

Ctrl+W is also probably likely to be accidentally triggerred in Panorama mode, since W is right next to E, and Ctrl+E is the key combo to leave panorama.

Comment 7

[Anant Narayanan [:anant]]

Attachment: Stop propagating keyboard events

We were attaching keyboard events to the whole window instead of just the tabcandy view, this patch changes that and makes all events not propagate (which fixes this bug).

However, as a result, ui.js is now exclusively responsible for all keyboard events when the panorama view is active. This is both a blessing and a curse -- we can easily fix bug #579199 for example; however we have to define an extensive of keyboard shortcuts that *should* work when panorama is open (for example apple-q does not quite firefox when panorama is visible with this patch).

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 487716 [details] [diff] [review]
Stop propagating keyboard events

Actually, now that I think of it, this needs to be handled at another level. Most of these key combos are just shortcuts to menu items, each of which should also be disabled. So, for instance, if you want cmd-+ to not zoom random web pages while you're in the Panorama UI, you need to actually disable the "zoom in" menu item (which will presumably also disable the key combo).

Comment 9

[Anant Narayanan [:anant]]

I don't entirely agree. This means we'd have to go through the entire list of possible keyboard shortcuts on a web page and disable them one-by-one and re-enable them when panorama exits. I think it is more logical for panorama to grab keyboard events when it is active instead of relaying back events to the active page (which is what happens now).

This makes it easier for us to define new panorama-specific keyboard shortcuts in the future, and the number of actions that are global (i.e. applicable to the whole browser and not a specific page) are likely to be lesser than the number of actions one can perform on a page making it easier for us to code them in.

With either approach, we have to come up with possible user actions -- either on the browser or a page. My argument is that it's easier for us to pass through actions on the global browser.

Comment 10

[Anant Narayanan [:anant]]

A clearer, shorter argument: I think it is better for us to define what one /can/ do when in panorama mode as opposed to what one /cannot/ :)

Comment 11

[Ian Gilman [:iangilman]]

(In reply to comment #10)
> A clearer, shorter argument: I think it is better for us to define what one
> /can/ do when in panorama mode as opposed to what one /cannot/ :)

Fair enough. I'm just saying that those menu items needs to be disabled one way or another, so any fix that focuses solely on the keys isn't good enough. 

If we can find a way to disable those menu items by defining what one can do in Panorama as opposed to what one cannot, I'm all for it!

Comment 12

[Kevin Hanes]

Moving over to Raymond.

Comment 13

[Kevin Hanes]

Punting

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Anant, Raymond, will you have a chance to revisit this soon? If not, let's reset the assignee so someone can look at it.

Comment 16

[Kevin Hanes]

Tim is working on bug 621795, which should fix this one as well. Assigning to him so no one els snags it

Comment 17

[Kevin Hanes]

bugspam. Moving b10 to b11

Comment 18

[Kevin Hanes]

bugspam. Removing b10

Comment 20

[Ian Gilman [:iangilman]]

To verify that this covers all cases, please check all the bugs have have been duplicated against this.

Comment 22

[Ian Gilman [:iangilman]]

Ok, I think it's too late for bug 621795 to make it into Fx4, so we need this as a stopgap. In that case, I think the direction Anant started is the way to go. Raymond, can you take this over and bring it on home?

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: v2

Added a whitelist which allows ctrl/cmd + F and ctrl/cmd + shift + e to pass through because those key elements handles code both inside and outside panorama.

Comment 24

[Raymond Lee [:raymondlee]]

Attachment: v2

Minor update

Comment 25

[Dão Gottwald [:dao]]

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

Please follow the pattern used here:
http://hg.mozilla.org/mozilla-central/annotate/db495f146676/browser/base/content/browser-tabPreviews.js#l193
http://hg.mozilla.org/mozilla-central/annotate/db495f146676/browser/base/content/browser-tabPreviews.js#l482
And remove the TabView stuff from cmd_find in browser-sets.inc.

Comment 26

[Ian Gilman [:iangilman]]

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

Looks good (modulo Dao's comments, of course). Needs a test. 

>-    iQ("#searchbox")[0].focus(); 

Why this change?

Comment 27

[Raymond Lee [:raymondlee]]

(In reply to comment #26)
> Comment on attachment 512482 [details] [diff] [review]
> v2
> 
> Looks good (modulo Dao's comments, of course). Needs a test. 
> 
> >-    iQ("#searchbox")[0].focus(); 
> 
> Why this change?

When the search feature is initialized, the focus is set on the searchbox (but search feature is hidden) which causes a validation in the _setTabViewFrameKeyHandlers() thinks search feature is enabled and returns early.  Therefore, I have remove that to fix it.

Comment 28

[Raymond Lee [:raymondlee]]

Attachment: v3

I've updated the patch to follow the pattern suggested by Dao and also removed the TabView stuff from cmd_find in browser-sets.inc

However, I encountered some problems.  The following pattern checks the charCode. 
http://hg.mozilla.org/mozilla-central/annotate/db495f146676/browser/base/content/browser-tabPreviews.js#l482

However, in the _setTabViewFrameKeyHandlers(), keydown listener is used so event.charCode can't be detected (always returns 0).  I tried to change to a keypress listener, the charCode returned fine but it introduced a bug in the search feature. There is also a keydown listener in the search code.  When presses Esc in the search mode, it quits the search mode as well as existing the TabView UI.

And then, I changed it to keypress listener in the search code.  The previous bug doesn't exist anymore but it introduced another bug.  When in tabview UI, presses a char and the search mode is displayed but the char isn't entered into the search box.

At the end, I modified the patch to use "DOM_VK" + key in the _setupBrowserKeys and check event.keyCode instead in the _setTabViewFrameKeyHandlers(). Dao: is that ok?

Comment 29

[Raymond Lee [:raymondlee]]

Attachment: v3

The test is missing in the last patch.

Comment 30

[Ian Gilman [:iangilman]]

Comment on attachment 512826 [details] [diff] [review]
v3

(In reply to comment #27)
> When the search feature is initialized, the focus is set on the searchbox (but
> search feature is hidden) which causes a validation in the
> _setTabViewFrameKeyHandlers() thinks search feature is enabled and returns
> early.  Therefore, I have remove that to fix it.

Cool, thanks.

Comments on the new patch:

>-  // execute a find command (i.e. press cmd/ctrl F)
>-  document.getElementById("cmd_find").doCommand();
>+  // press cmd/ctrl F
>+  EventUtils.synthesizeKey("f", { accelKey: true });

Why this change?

Also, I wonder if the test for this bug needs to try more keys? Seems a bit much to try them all, but maybe a little broader coverage would be good?

Either way, the patch needs to have a much bigger white list. Here are a bunch of commands that should still work while in Panorama (Mac version; I'm sure there are Windows and Linux-specific items as well that we should address):

* Quit
* Preferences
* New Window
* Close Window (rather than close tab)
* Undo Close Tab
* Undo Close Window
* Private Browsing
* Minimize

Comment 31

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to comment #30)
> Comments on the new patch:
> 
> >-  // execute a find command (i.e. press cmd/ctrl F)
> >-  document.getElementById("cmd_find").doCommand();
> >+  // press cmd/ctrl F
> >+  EventUtils.synthesizeKey("f", { accelKey: true });
> 
> Why this change?

In comment 25, Dao requested to remove the TabView stuff from cmd_find in browser-sets.inc so cmd_find doesn't do anything in TabView UI, hence switching it to use the ctrl/cmd + f key

> 
> Also, I wonder if the test for this bug needs to try more keys? Seems a bit
> much to try them all, but maybe a little broader coverage would be good?

Added few more tests.

> 
> Either way, the patch needs to have a much bigger white list. Here are a bunch
> of commands that should still work while in Panorama (Mac version; I'm sure
> there are Windows and Linux-specific items as well that we should address):
> 
> * Quit
> * Preferences
> * New Window
> * Close Window (rather than close tab)
> * Undo Close Tab
> * Undo Close Window
> * Private Browsing
> * Minimize

I've checked the windows and linux, I think the list you cover all the things.

Since the "Preferences" key combination uses a cmd + , so I have to use keypress listener with charCode in UI__setTabViewFrameKeyHandlers and then added a workaround to fix the issue introduced in search feature. The issue was that when presses Esc in the search mode, it quits the search mode as well as existing the TabView UI.

Furthermore, you would notice in some tests, I've changed to the letter "e" to "E" when the key combination involves shiftKey=true.  When ctrl/cmd + shift + e is pressed in the tabview UI, the charCode of "E" would be passed into the white list function processBrowserKeys() because the shift button is also pressed.  However, the charCode of "e" is passed to the white list function when stimulating a key press using EventUtils.synthesizeKey().  That's why I've changed the lower case letter to upper one if the key combination involves shift key.  Another way to leave the tests and fix the problem is to add the lower case as well as upper case charCodes to the white list function processBrowserKeys()


-    EventUtils.synthesizeKey("e", { accelKey: true, shiftKey: true });
+    EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true });

Comment 32

[Raymond Lee [:raymondlee]]

Comment on attachment 513210 [details] [diff] [review]
v4

Passed try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=240a85be5dbb

Comment 33

[Ian Gilman [:iangilman]]

Comment on attachment 513210 [details] [diff] [review]
v4

Looks good. Let's do this.

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 513210 [details] [diff] [review]
v4

While a good fix, I think at this point it carries more risk than potential reward. Let's remember to get it as good polish after Firefox 4.

Comment 35

[Michael Yoshitaka Erlewine [:mitcho]]

Punted to the future. :(

Comment 37

[Kevin Hanes]

Raymond, would you please un-bitrot this patch and flag for check in? The tree isn't currently approval required, though it is managed by the sheriff.

Comment 38

[(no longer active)]

This patch doesn't apply cleanly any more.

Comment 39

[Raymond Lee [:raymondlee]]

Attachment: v5

Un-bitrot the patch. Sent it to try and waiting for the results.

Comment 40

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 41

[Raymond Lee [:raymondlee]]

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=3073de5db8cb

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 522348 [details] [diff] [review]
Patch for checkin

>+          // this return indicates to stop the default action
>+          return  preventDefault;
>+        } else {
>+          return false;
>+        }

Call event.preventDefault() instead

Comment 43

[Raymond Lee [:raymondlee]]

Attachment: v6

Comment 44

[Dão Gottwald [:dao]]

Comment on attachment 522422 [details] [diff] [review]
v6

It seems that I misread the previous patch. What I care about is that the function eventually handling the event doesn't return false but calls stopPropagation / preventDefault instead. It's fine for helper functions to arbitrarily pass around boolean values. It may make sense to change this and let helper functions cancel events directly, I'll leave it to your judgement whether that improves the code in this particular case.

Comment 45

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 46

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Minor update.

Comment 47

[Raymond Lee [:raymondlee]]

(In reply to comment #46)
> Created attachment 522557 [details] [diff] [review]
> Patch for checkin
> 
> Minor update.

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=d11be6008112

Comment 48

[Kevin Hanes]

bugspam

Comment 49

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/565c588e3e51

Comment 50

[George Carstoiu]

Verified issue on Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre