KeyEvent in TabCandy window affect browser

VERIFIED FIXED in Firefox 5

Status

defect
P2
minor
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: alice0775, Assigned: raymondlee)

Tracking

({polish})

Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
Related bug: Ctrl+F in TabCandy , and then return to browser, Find toolbar activated.
(Reporter)

Updated

9 years ago
Summary: Keypress Ctrl++,Ctrl+- and Crtl+0 in TabCandy window affect Zoom level of last current Tab → KeyEvent in TabCandy window affect browser
(Reporter)

Updated

9 years ago
Blocks: 585689
Nice catch Alice.
Priority: -- → P4
Duplicate of this bug: 587212
Duplicate of this bug: 579199

Updated

9 years ago
Severity: normal → minor
Priority: P4 → P5
Target Milestone: --- → Firefox 4.0
Not a blocker, kinda strange, though!
blocking2.0: --- → -
Keywords: polish
Assignee: nobody → anant
Blocks: 608028
No longer blocks: 585689
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.
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).
Attachment #487716 - Flags: feedback?(ian)
Status: NEW → ASSIGNED
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).
Attachment #487716 - Flags: feedback?(ian) → feedback-
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.
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/ :)
(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

9 years ago
Moving over to Raymond.
Assignee: anant → raymond
(Assignee)

Updated

9 years ago
Status: ASSIGNED → NEW

Updated

8 years ago
Blocks: 621795

Comment 13

8 years ago
Punting
Priority: P5 → P2
Target Milestone: Firefox 4.0 → ---

Updated

8 years ago
Duplicate of this bug: 627017
Anant, Raymond, will you have a chance to revisit this soon? If not, let's reset the assignee so someone can look at it.
(Assignee)

Updated

8 years ago
Assignee: raymond → nobody

Comment 16

8 years ago
Tim is working on bug 621795, which should fix this one as well. Assigning to him so no one els snags it
Assignee: nobody → tim.taubert
OS: Windows 7 → All
Hardware: x86 → All

Comment 17

8 years ago
bugspam. Moving b10 to b11
Blocks: 627096

Comment 18

8 years ago
bugspam. Removing b10
No longer blocks: 608028
Duplicate of this bug: 608774
To verify that this covers all cases, please check all the bugs have have been duplicated against this.
Duplicate of this bug: 623768
No longer blocks: 621795
Depends on: 621795
Assignee: tim.taubert → nobody
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?
Assignee: nobody → raymond
Blocks: 621795
No longer depends on: 621795
(Assignee)

Comment 23

8 years ago
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #487716 - Attachment is obsolete: true
Attachment #512420 - Flags: feedback?(ian)
(Assignee)

Comment 24

8 years ago
Posted patch v2 (obsolete) — Splinter Review
Minor update
Attachment #512420 - Attachment is obsolete: true
Attachment #512482 - Flags: feedback?(ian)
Attachment #512420 - Flags: feedback?(ian)
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?
Attachment #512482 - Flags: feedback?(ian) → feedback+
(Assignee)

Comment 27

8 years ago
(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.
(Assignee)

Comment 28

8 years ago
Posted patch v3 (obsolete) — Splinter Review
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?
Attachment #512482 - Attachment is obsolete: true
Attachment #512749 - Flags: review?(dao)
Attachment #512749 - Flags: feedback?(ian)
(Assignee)

Comment 29

8 years ago
Posted patch v3 (obsolete) — Splinter Review
The test is missing in the last patch.
Attachment #512749 - Attachment is obsolete: true
Attachment #512826 - Flags: review?(dao)
Attachment #512826 - Flags: feedback?(ian)
Attachment #512749 - Flags: review?(dao)
Attachment #512749 - Flags: feedback?(ian)
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
Attachment #512826 - Flags: feedback?(ian) → feedback-
(Assignee)

Comment 31

8 years ago
Posted patch v4 (obsolete) — Splinter Review
(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 });
Attachment #512826 - Attachment is obsolete: true
Attachment #513210 - Flags: review?(ian)
Attachment #512826 - Flags: review?(dao)
Comment on attachment 513210 [details] [diff] [review]
v4

Looks good. Let's do this.
Attachment #513210 - Flags: review?(ian) → review+
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Attachment #513210 - Flags: approval2.0?
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.
Attachment #513210 - Flags: approval2.0? → approval2.0-
Punted to the future. :(
Blocks: 603789
No longer blocks: 627096
Target Milestone: --- → Future
Duplicate of this bug: 636178
(Assignee)

Updated

8 years ago
Status: ASSIGNED → NEW
(Assignee)

Updated

8 years ago
Blocks: 640592
(Assignee)

Updated

8 years ago
Blocks: 640594

Comment 37

8 years ago
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

8 years ago
This patch doesn't apply cleanly any more.
Whiteboard: not-ready
(Assignee)

Comment 39

8 years ago
Posted patch v5 (obsolete) — Splinter Review
Un-bitrot the patch. Sent it to try and waiting for the results.
Attachment #513210 - Attachment is obsolete: true
(Assignee)

Comment 40

8 years ago
Posted patch Patch for checkin (obsolete) — Splinter Review
Attachment #522271 - Attachment is obsolete: true
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
(Assignee)

Comment 43

8 years ago
Posted patch v6 (obsolete) — Splinter Review
Attachment #522348 - Attachment is obsolete: true
Attachment #522422 - Flags: review?(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.
Attachment #522422 - Flags: review?(dao)
(Assignee)

Comment 45

8 years ago
Posted patch Patch for checkin (obsolete) — Splinter Review
Attachment #522422 - Attachment is obsolete: true
(Assignee)

Comment 46

8 years ago
Minor update.
Attachment #522554 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 47

8 years ago
(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
(Assignee)

Updated

8 years ago
Blocks: 646523

Updated

8 years ago
Target Milestone: Future → ---

Comment 48

8 years ago
bugspam

Comment 49

8 years ago
http://hg.mozilla.org/mozilla-central/rev/565c588e3e51
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: not-ready
Target Milestone: --- → Firefox4.2

Comment 50

8 years ago
Verified issue on Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Target Milestone: Firefox5 → Firefox 5
(Reporter)

Updated

8 years ago
Depends on: 654601
(Assignee)

Updated

8 years ago
Blocks: 671809
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.