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

VERIFIED FIXED in Firefox 6

Status

()

Firefox
General
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: Josh Aas, Assigned: Dolske)

Tracking

Trunk
Firefox 6
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

7 years ago
Summary: tab-modal dialogs can get into bad focus state and key event bleed through tabs → tab-modal dialogs can get into bad focus state and key events can bleed through tabs
(Reporter)

Comment 1

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

7 years ago
should this really block though?

Comment 3

7 years ago
sounds similar to bug 631485. I swear we had another bug on this but I can't find it.
(Reporter)

Comment 4

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

7 years ago
This applies to Linux as well as OS X.

Updated

7 years ago
Blocks: 59314
Version: unspecified → Trunk

Comment 6

7 years ago
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").

Updated

7 years ago
OS: Mac OS X → All

Updated

7 years ago
Component: General → General
Product: Toolkit → Firefox
QA Contact: general → general

Comment 7

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

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

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

7 years ago
(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.
blocking2.0: ? → .x+
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.
Attachment #516668 - Flags: feedback?(dolske)
(Assignee)

Comment 12

7 years ago
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...
Assignee: nobody → dolske
Attachment #516668 - Attachment is obsolete: true
Attachment #516668 - Flags: feedback?(dolske)
Attachment #516792 - Flags: feedback?(enndeakin)
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
Attachment #516792 - Flags: feedback?(enndeakin) → feedback+
(Assignee)

Updated

7 years ago
Blocks: 617507
blocking2.0: .x+ → ---
blocking2.0: --- → .x+
(Assignee)

Comment 14

7 years ago
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
Attachment #516792 - Flags: review?(enndeakin)
(Assignee)

Comment 15

7 years ago
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.
Attachment #516792 - Attachment is obsolete: true
Attachment #516792 - Flags: review?(enndeakin)
Attachment #522913 - Flags: review?(gavin.sharp)

Updated

6 years ago
Duplicate of this bug: 649548
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.
Attachment #522913 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 18

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

Comment 19

6 years ago
Created attachment 526436 [details] [diff] [review]
Patch v.3
Attachment #522913 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/1d48ae2a81a8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6

Comment 21

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.