638352 - tab-modal dialogs can get into bad focus state and key events can bleed through tabs

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Josh Aas]

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]

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]

should this really block though?

Comment 3

[Asa Dotzler [:asa]]

sounds similar to bug 631485. I swear we had another bug on this but I can't find it.

Comment 4

[Josh Aas]

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]

This applies to Linux as well as OS X.

Comment 6

[B.J. Herbison]

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]

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]

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]

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]

(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]

Attachment: 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]]

Attachment: 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]

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]]

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]]

Attachment: Patch v.2

Oh, forgot to fix the last bit from comment 12. And I suppose gavin actually needs to review this.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

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]]

(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]]

Attachment: Patch v.3

Comment 20

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/1d48ae2a81a8

Comment 21

[George Carstoiu]

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.