303730 - <textarea>s in other tabs mess up focus in current 'tab'

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

(Picking component from other focus stealing bugs like bug 124750, but this
might belong elsewhere.)

In my embedding application (Epiphany), which uses GtkMozEmbed for its tabs, I
use nsIWebBrowserFocus (::Activate
[http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWebBrowser.cpp#1738]
and ::Deactivate
[http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWebBrowser.cpp#1810]),
to control focus, to work around the broken gtkmozembed focus (bug 210373 and
bug 244664). ::Activate/Deactivate primarily set the root focus controller's
'active' state.

However, I've found that a <textarea> in an inactive tab (with inactive focus
controller) can still take the focus away from elements in the active tab (with
active focus controller). The focus is taken away from the focused element in
the active tab but does not actually end up in the inactive tab (i.e. what I
type does not go to the other tab, so this is probably not a security problem).
This only happens for <textarea> with .focus and .select; <input> works as
expected (tested with type="text" and type="button" at least).

Steps to reproduce:
0) Load the testcases test-textarea.html and test-textarea-select.html from
http://www.gnome.org/~chpe/testcases/ in Epiphany.
1) Open the links in a new tab and switch to that tab
2) Try to input text in the textarea or the text input field.

Result:
The <textarea> in the background tab prevents continued input by taking focus away.

Analysis:

Comparing nsHTMLInputElement::SetFocus
[http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp#1053]
with nsHTMLTextAreaElement::SecFocus
[http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLTextAreaElement.cpp#232],
I find that the nsHTMLInputElement's code contains something that the
nsHTMLTextArea's code does not: it tests the focus controller's state, and only
sets the focused window and element if it's inactive and then returns early.
Copying that code to nsHTMLTextArea::SetFocus and ::Select fixes this bug; patch
coming up.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: proposed fix

Code copied from nsHTMLInputElement: Test the focus controller's "active" state
and return early if it's inactive.

Comment 2

[Crispin Flowerday (not receiving bug mail)]

The test case also appears to steal focus from FireFox's location entry.

Comment 3

[Martijn Wargers (dead)]

This reminds me of bug 124750. But the fix for that bug didn't fix it for you?

Comment 4

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #3)
> This reminds me of bug 124750. But the fix for that bug didn't fix it for you?

No, the fix for bug 124750 doesn't fix this. I tested this bug with Epiphany
HEAD using gecko from firefox 1.0.6 and trunk as backend and while both contain
that fix, this bug is still present.

Comment 5

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 191835 [details] [diff] [review]
proposed fix

Let's request r/sr... (I hope I picked the right ones)

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 191835 [details] [diff] [review]
proposed fix

Looks good to me, but I'd prefer someone who knows focus to take a look.

Comment 7

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: trunk patch

the is the same patch as attachment 191835 [details] [diff] [review], rediff'd against trunk (no code
changes, only context changes).

Comment 8

[Martijn Wargers (dead)]

Johnny, could you review the patch? Thanks.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 200072 [details] [diff] [review]
trunk patch

Sorry for not getting to this any earlier, this got lost in my sometimes unmanageable amount of bugmail and requests...

+  nsCOMPtr<nsPIDOMWindow> win =
+    do_QueryInterface(doc->GetScriptGlobalObject());
+  if (win) {

"win" could now be a raw nsPIDOMWindow pointer and you get the window by calling doc->GetWindow(), no need to QI any more (and GetWindow() will return the outer window, which you want, not the inner window that you get by QI'ing the script global object).

r=jst with that changed (in both places).

Comment 10

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch for reviewer's comment

Comment 11

[Martijn Wargers (dead)]

chpe, I can check the patch in later today, if you want to (unless someone beats me on doing it).

Comment 12

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #11)
> chpe, I can check the patch in later today, if you want to (unless someone
> beats me on doing it).
Since I don't have cvs commit access, that would be appreciated. Thanks!

Comment 13

[Martijn Wargers (dead)]

Checking in nsHTMLTextAreaElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLTextAreaElement.cpp,v  <--  nsHT
MLTextAreaElement.cpp
new revision: 1.184; previous revision: 1.183
done

Checked into trunk. Thanks for the patch.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We probably should get this on the 1.8.1 branch for chpe's sake.

Comment 15

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

Let's let this bake on the trunk for a couple of days; we're a little nervous about playing with focus this late in the game.

bryner: you sr'd this patch, what's the risk factor in taking this, potential-regression-wise?

Comment 16

[Brian Ryner (not reading)]

(In reply to comment #15)
> Let's let this bake on the trunk for a couple of days; we're a little nervous
> about playing with focus this late in the game.
> 
> bryner: you sr'd this patch, what's the risk factor in taking this,
> potential-regression-wise?
> 

very low risk, this is just syncing textarea with input

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 228967 [details] [diff] [review]
updated patch for reviewer's comment

a=mconnor on behalf of drivers for checkin by Wednesday, July 19th

Comment 18

[Martijn Wargers (dead)]

Attachment: patch for 1.8.1 branch

This is what I'm going to check in in the 1.8.1 branch.

Comment 19

[Martijn Wargers (dead)]

Checking in nsHTMLTextAreaElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLTextAreaElement.cpp,v  <--  nsHT
MLTextAreaElement.cpp
new revision: 1.171.4.4; previous revision: 1.171.4.3
done

Fixed on the 1.8.1 branch.