Last Comment Bug 799299 - focus contention with two visible html content areas
: focus contention with two visible html content areas
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Virgil Dicu [:virgil] [QA]
Mentors:
Depends on: 837069 800817
Blocks: 798141
  Show dependency treegraph
 
Reported: 2012-10-08 17:35 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2013-02-01 13:08 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
WIP (1.71 KB, patch)
2012-10-09 07:42 PDT, Olli Pettay [:smaug]
masayuki: review+
Details | Diff | Splinter Review
+tests (6.28 KB, patch)
2012-10-11 06:35 PDT, Olli Pettay [:smaug]
masayuki: review+
Details | Diff | Splinter Review
beta roll-up patch (19.98 KB, patch)
2012-10-17 15:32 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-10-08 17:35:45 PDT
This is a general firefox bug unrelated to socialapi, but was initially discovered via the socialapi and documented in bug 798141.  Here is how you can reproduce the problem without the socialapi.

1. open the bookmarks sidebar
2. right click in an empty area and choose add new bookmark
3. enter about:home for the url (assuming you haven't changed about:home)
4. click "open in sidebar"
5. save bookmark
6. click on bookmark in sidebar

You should have the firefox/google search page.

7. open some google document and edit
8. click in the search box in the sidebar, type something
9. click in the google document, try to type something

You'll see your typing appear in the search box in the sidebar.

It would suck to have that happen when typing a password...but this doesn't seem to happen with form input.  Mark Hammond looked at this using "firefocus addon to firebug" and:

[5:14pm] markh: so there is a firefocus addon to firebug.  If I click on most parts of the page, it reports focus is typically what we expect.  However, if I click in the editable area, it reports both the doc and the window give up focus.
[5:15pm] markh: it doesn't tell me what *gets* it though
Comment 1 Mark Hammond [:markh] 2012-10-09 00:39:50 PDT
The same basic issue can be reproduced with HTML content that cancels the mousedown event.  eg, in the repro above, you can use the following HTML instead of google docs:

<script>
window.addEventListener("focus", function(e) {dump("focus\n");});
window.addEventListener("mousedown", function(e) {e.preventDefault(); dump("mousedown\n"); return false;}, false);
</script>

No actual content is needed, just the <script> block will reproduce it.

With the above mousedown handler, the content window fails to get focus - so even though you click on the window, the sidebar retains focus (and hence all typing is still targeted at the sidebar.)  Note that google docs draws its own caret - even when it doesn't have focus - which makes the issue more obvious.

The problem seems to be the block at http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#3252.  We get all the way to line 3264, but as the document isn't chrome, focus isn't given to the window (and the dump() in the focus event handler isn't seen).

If I comment out lines 3624 and 3626 (ie, so:
  fm->SetFocusedWindow(mDocument->GetWindow());
is called unconditionally), then the problem goes away - the content window does get focus (the dump() can be seen), the sidebar loses focus, and things all work as expected.

Given the comment at the start of that block:

// When the event was cancelled, there is currently a chrome document
// focused and a mousedown just occurred on a content document, ensure
// that the window that was clicked is focused.

it appears that code assumes the only window that could have focus is a chrome document, but in this example that isn't the case - normal HTML content in the sidebar has focus.  I'm not sure what the impact of not performing the check at line 3624 is - hopefully someone with more knowledge than me can weigh in.
Comment 2 Olli Pettay [:smaug] 2012-10-09 03:16:02 PDT
Enn, you'll handle this? Or should I take a look?
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-09 04:00:07 PDT
Neil is away for the next little while, so if you could investigate, Olli, that'd be great.
Comment 4 Olli Pettay [:smaug] 2012-10-09 07:42:37 PDT
Created attachment 669528 [details] [diff] [review]
WIP

I need to write still tests.
Comment 5 Mark Hammond [:markh] 2012-10-09 16:56:55 PDT
Comment on attachment 669528 [details] [diff] [review]
WIP

FWIW, this patch solves the problem in all the repros I have.
Comment 6 Olli Pettay [:smaug] 2012-10-09 17:03:01 PDT
And I need to figure out how the test this all automatically.
I was hoping something simple but apparently no... :/
Comment 7 Olli Pettay [:smaug] 2012-10-10 06:13:35 PDT
Comment on attachment 669528 [details] [diff] [review]
WIP

Still haven't figured out reliable way to test this.
Comment 8 Olli Pettay [:smaug] 2012-10-10 06:21:16 PDT
https://tbpl.mozilla.org/?tree=Try&rev=98ac763a6e29
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-10 20:46:28 PDT
Comment on attachment 669528 [details] [diff] [review]
WIP

If we can use <browser> element in mochitest-chrome, it seems that we can test this.
Comment 10 Olli Pettay [:smaug] 2012-10-11 00:56:32 PDT
Yes, chrome test should work. I was trying to get mochitest working (using new window) but
for some reason it wasn't successful.
Comment 11 Olli Pettay [:smaug] 2012-10-11 06:35:31 PDT
Created attachment 670379 [details] [diff] [review]
+tests

https://tbpl.mozilla.org/?tree=Try&rev=e6a6b69caaf0

It is painful to write focus handling tests.
Comment 12 Olli Pettay [:smaug] 2012-10-11 15:51:33 PDT
https://hg.mozilla.org/mozilla-central/rev/43218e560c4a
Comment 13 David Flanagan [:djf] 2012-10-12 12:15:56 PDT
This patch has broken the Gaia keyboard for in-process apps.  See #800817
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-12 13:23:04 PDT
The plan to fix is to make this behavior pref-able, and fix the test to check cross-frame focus continues to work when the pref is flipped that way.

Sound good?
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-12 13:36:47 PDT
Has any thought been given to potential web-compat issues with this patch?  (I'm not trying to insinuate anything, I honestly have no idea.)
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-12 15:28:54 PDT
How is someone using Social API going to run into this? Will it affect a lot of users?
Comment 17 Shane Caraveo (:mixedpuppy) 2012-10-12 15:42:14 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #16)
> How is someone using Social API going to run into this? Will it affect a lot
> of users?

yes, potentially *lots* of users.  All you need to have is focus in the socialapi sidebar, which for users of that feature, will more likely be open than not.  While I've only repo'd on google docs, it is unknown how many sites might be affected (I didn't look for more).
Comment 18 Boris Zbarsky [:bz] 2012-10-12 20:51:13 PDT
> Has any thought been given to potential web-compat issues with this patch?

And in particular, what do other UAs do?  The Gaia stuff this broke seems like something web pages would be doing to me for sure.
Comment 19 Boris Zbarsky [:bz] 2012-10-12 21:18:46 PDT
I guess the key is that checking for separate .top means this won't bite web content?
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-10-12 21:37:13 PDT
Hmm, I'm not sure what's going on. But I should comment my general idea.

I think that when user clicks a document (or window), the document (or window) should take focus even if the mouse event is consumed. If not so, user may type text in unexpected website. That may be too dangerous in some situations.

I'm not sure why this behavior breaks something in Gaia and Social API Sidebar because ESM still checks whether the clicked document is chrome or not.

But it might be better to check -moz-user-select or -moz-user-focus or something of the target content before moving focus.
Comment 21 Boris Zbarsky [:bz] 2012-10-12 21:41:03 PDT
> because ESM still checks whether the clicked document is chrome or not.

There is absolutely nothing in Gaia that is "chrome" as far as the ESM knows.  There's a virtual keyboard and a textfield.  They are both "content" as far as the ESM is concerned.  So tapping the virtual keyboard steals focus from the textfield.  (Note, all that might be better to discuss in bug 800817.)
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-13 18:45:42 PDT
If this is landed on aurora, bug 800817 needs to come with it.
Comment 23 Olli Pettay [:smaug] 2012-10-15 04:37:34 PDT
Since I'm not really involved with Social API thing, if you need this on some branches, please
ask for approval.
Comment 24 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-16 16:01:11 PDT
So if this is key to Social API that means we need it uplifted to both Aurora and Beta - which means also uplifting bug 800817 to those channels as well - please nominate and provide a risk assessment.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-17 15:32:08 PDT
Created attachment 672551 [details] [diff] [review]
beta roll-up patch

This includes this bug's patch, plus the followup fixes: bug 800817 and bug 802436.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
* longstanding issue with multiple content frames (e.g. sidebars) visible, exposed further by Social API
User impact if declined:
* strange focus behavior, difficulty using e.g. Google Docs and Social at the same time (bug 798141)
Testing completed (on m-c, etc.): 
* has been on m-c since Oct 11, has automated tests
Risk to taking this patch (and alternatives if risky): 
* pretty low risk. Only affects cases where multiple top-level browsing contexts exist in the same window, which for firefox desktop means only sidebars. For b2g, this affected the keyboard, but we've since fixed that (bug 800817)
String or UUID changes made by this patch:
* none
Comment 26 Alex Keybl [:akeybl] 2012-10-17 16:45:00 PDT
(if we see any further regressions from this, however unlikely, we'll strongly consider backing out rather than fixing forward)
Comment 29 Mihai Morar, (:MihaiMorar) 2013-01-03 02:39:45 PST
Works for me for:

FF 17
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0(20121119183901)
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0(20121119183901)


FF 18b7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0(20121231071231)
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0(20121231071231)
Comment 30 Virgil Dicu [:virgil] [QA] 2013-01-03 06:21:05 PST
Verified on Mac OS as well, in addition to Mario's Ubuntu and Windows.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/20100101 Firefox/18.0

No problems while typing in google docs/about:home sidebar. Could previously reproduce the issue reported in comment 0.
Comment 31 Mihai Morar, (:MihaiMorar) 2013-01-23 06:03:10 PST
(In reply to Virgil Dicu [:virgil] [QA] from comment #30)
> Verified on Mac OS as well, in addition to Mario's Ubuntu and Windows.
> 
> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/20100101
> Firefox/18.0

In addition to comment 29, Verified Fixed on FF 17 on Mac 10.8 too.

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