Last Comment Bug 638352 - tab-modal dialogs can get into bad focus state and key events can bleed through tabs
: tab-modal dialogs can get into bad focus state and key events can bleed throu...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 6
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
: 649548 (view as bug list)
Depends on:
Blocks: 59314 617507
  Show dependency treegraph
 
Reported: 2011-03-02 21:17 PST by Josh Aas
Modified: 2013-12-27 14:23 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
partial patch (1.18 KB, patch)
2011-03-03 12:08 PST, Neil Deakin
no flags Details | Diff | Review
Patch v.1 (7.42 KB, patch)
2011-03-03 20:12 PST, Justin Dolske [:Dolske]
enndeakin: feedback+
Details | Diff | Review
Patch v.2 (7.45 KB, patch)
2011-03-29 19:24 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Review
Patch v.3 (7.65 KB, patch)
2011-04-15 17:40 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review

Description Josh Aas 2011-03-02 21:17:55 PST
1. Open a browser window with one tab and put focus in a text area in the page.
2. Hit cmd-t to open a new tab.
3. In the address bar of the new tab type and execute: "javascript: alert(1)"
4. You'll see a tab-modal dialog alert come up.
5. Hit cmd-1 to switch back to the first tab.
6. Hit cmd-2 to switch back to the second tab with the alert dialog.
7. Hit enter.

Expected: Alert should go away.
Actual result: Alert doesn't go away.

8. Now press the "a" key while you're staring at the tab that should have gone away.
9. Switch back to the first tab and notice that the key press from the other tab bled through and put the "a" character into the text area.

I bet there are simpler ways to make these things happen.
Comment 1 Josh Aas 2011-03-02 21:20:34 PST
Sorry, step 8 should read:

8. Now press the "a" key while you're staring at the alert (in tab #2) that should have gone away.

The tab itself should not have gone away, that was a typo.
Comment 2 mattburles 2011-03-02 21:55:11 PST
should this really block though?
Comment 3 Asa Dotzler [:asa] 2011-03-02 23:36:06 PST
sounds similar to bug 631485. I swear we had another bug on this but I can't find it.
Comment 4 Josh Aas 2011-03-02 23:50:07 PST
The worst part about this, to me, is the key presses bleeding through the tabs. As for dismissing the alert, the problem is obvious to the user and it can be worked around with the mouse. The bleeding keys problem is not obvious and users could end up doing things they didn't mean to and not notice it.
Comment 5 B.J. Herbison 2011-03-03 01:02:43 PST
This applies to Linux as well as OS X.
Comment 6 B.J. Herbison 2011-03-03 07:02:15 PST
And also Windows XP.

The problem appears to be that focus does not move when switching tabs if the destination tab has an alert. The "enter" in step 7 is given to the previous tab. If the first tab (in step 1) is a bugzilla bug report and the text box is the search field, step 7 will cause an alert to be raised in the first tab ("Please enter one or more search terms first").
Comment 7 Benjamin Smedberg [:bsmedberg] 2011-03-03 08:27:15 PST
Does this only happen if you switch tabs using the keyboard, or does it also happen if you switch tabs using the mouse?
Comment 8 joren.de.cuyper 2011-03-03 08:35:30 PST
Also when using mouse. 
With my Mac it isn't even necessary to focus in a text area one the page (see step one).

Using: minefield 4.0b13pre (2011-03-02); Mac OSX 10.6.6;
Comment 9 Daniel 2011-03-03 08:46:38 PST
If the text area in the STR is a search field which isn't empty, attempting to close the alert box by hitting enter performs a search in the background. It could presumably submit any other form that has focus as well.
Comment 10 B.J. Herbison 2011-03-03 09:01:59 PST
(In reply to comment #7)
> Does this only happen if you switch tabs using the keyboard, or does it also
> happen if you switch tabs using the mouse?

For Windows XP I can reproduce it several ways:

o Using the mouse in two ways:
  -  clicking the tabs in the tab bar
  -  using the tab list drop down on the right of the tab bar
o Using the keyboard in two ways:
  -  control-1, control-2, etc.
  -  control-page-up, control-page-down

The behavior is slightly different if I use tab groups -- if I use control-shift-e and select the tab with the alert the characters I type do not go to the previous tab. I suspect the characters may go to Panorama -- see the next paragraph.

An interesting scenario: Create two tabs, one with an alert. Open Panorama and select the tab with an alert. Type "a" (or another letter) to that tab and reopen Panorama. Panorama opens in a pseudo "search mode": the screen is gray and the search box is present, but the "a" is not in the search box and the search box does not have focus. (I have not been able to get Panorama in this state without using a tab with an alert.)

Some other strange behavior I observed: This latest testing was done with an existing profile rather than a clean profile and Adblock Plus was installed. I twice saw control-shift-e open up the preferences dialog for Adblock Plus. I don't know what sequence I use to get into that state, and I just wasted ten minutes and was unable to find that state again.
Comment 11 Neil Deakin 2011-03-03 12:08:59 PST
Created attachment 516668 [details] [diff] [review]
partial patch

When a modal prompt is open, the prompt needs to be refocused when switching tabs. This patch shows the general idea, but it needs to be changed (by someone else hopefully) to focus the prompt instead.
Comment 12 Justin Dolske [:Dolske] 2011-03-03 20:12:17 PST
Created attachment 516792 [details] [diff] [review]
Patch v.1

This is basically a complete version (and fixes bug 617507 too, since it's a similar fix).

I think I'd like to make it a bit stricter my not assuming .lastChild is a prompt, and instead look for the last <tabprompt> in the <stack>.

And should I use focus manager + FLAG_NOSCROLL instead of just browser.focus()? Seems to work the same way as far as I can tell...
Comment 13 Neil Deakin 2011-03-14 10:20:04 PDT
Comment on attachment 516792 [details] [diff] [review]
Patch v.1

> And should I use focus manager + FLAG_NOSCROLL instead of just browser.focus()?

Does it work in this case:

1. Focus some field in the page
2. Scroll down so it is out of view
3. The page opens an alert
4. Close the alert
Comment 14 Justin Dolske [:Dolske] 2011-03-29 18:34:18 PDT
Comment on attachment 516792 [details] [diff] [review]
Patch v.1

Yeah, it doesn't scroll. So I think this should be fine as-is.

Wrote a little testcase: http://dolske.net/mozilla/tests/prompt/longpage.html
Comment 15 Justin Dolske [:Dolske] 2011-03-29 19:24:22 PDT
Created attachment 522913 [details] [diff] [review]
Patch v.2

Oh, forgot to fix the last bit from comment 12. And I suppose gavin actually needs to review this.
Comment 16 Thomas Ahlblom 2011-04-12 21:37:40 PDT
*** Bug 649548 has been marked as a duplicate of this bug. ***
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-15 13:09:55 PDT
Comment on attachment 522913 [details] [diff] [review]
Patch v.2

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

I'm a bit worried that some addon would have depended on the value of tabmodalPromptShowing, but I guess that's quite unlikely.

>+                let prompts = this.listPrompts();
>+                if (prompts.length) {
>+                    let prompt = prompts[prompts.length - 1];

Could use:
let prompt = this.listPrompts().slice(-1)[0];
if (prompt)
  prompt.Dialog.setDefaultFocus();

>               listPrompts : function(aPrompt) {

This could use Array.slice(els) rather than the loop, fwiw.

>               // Adjust focus

>+                if (newBrowser.hasAttribute("tabmodalPromptShowing")) {
>+                    let XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

Grr, how did I let you pollute this file with 4 space indentation? :) Let's not mix-and-match within this existing method, at least.
Comment 18 Justin Dolske [:Dolske] 2011-04-15 17:39:40 PDT
(In reply to comment #17)

> I'm a bit worried that some addon would have depended on the value of
> tabmodalPromptShowing, but I guess that's quite unlikely.

    Agreed.


> >+                    let prompt = prompts[prompts.length - 1];
> 
> Could use:
> let prompt = this.listPrompts().slice(-1)[0];

    Eh, I think that's less obvious to read, so keeping.


> >               listPrompts : function(aPrompt) {
> 
> This could use Array.slice(els) rather than the loop, fwiw.

    Ah, indeed. Fixed.


> Grr, how did I let you pollute this file with 4 space indentation? :) Let's not
> mix-and-match within this existing method, at least.

    Sorry, I'll never use 4-space indents again. ;)
Comment 19 Justin Dolske [:Dolske] 2011-04-15 17:40:49 PDT
Created attachment 526436 [details] [diff] [review]
Patch v.3
Comment 20 Justin Dolske [:Dolske] 2011-04-19 14:19:33 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/1d48ae2a81a8
Comment 21 George Carstoiu 2011-06-29 06:52:35 PDT
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110629 Firefox/7.0a1

Verified issue on Ubuntu 11.04, Mac OS X 10.6, WinXP, Win7 and it's no longer reproducible. Setting status to Verified Fixed.

Note You need to log in before you can comment on or make changes to this bug.