Closed Bug 799299 Opened 12 years ago Closed 12 years ago

focus contention with two visible html content areas

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
firefox17 + verified
firefox18 + verified

People

(Reporter: mixedpuppy, Assigned: smaug)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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
OS: Mac OS X → All
Hardware: x86 → All
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.
Enn, you'll handle this? Or should I take a look?
Neil is away for the next little while, so if you could investigate, Olli, that'd be great.
Attached patch WIP (obsolete) — Splinter Review
I need to write still tests.
Assignee: nobody → bugs
Comment on attachment 669528 [details] [diff] [review]
WIP

FWIW, this patch solves the problem in all the repros I have.
And I need to figure out how the test this all automatically.
I was hoping something simple but apparently no... :/
Comment on attachment 669528 [details] [diff] [review]
WIP

Still haven't figured out reliable way to test this.
Attachment #669528 - Flags: review?(masayuki)
Comment on attachment 669528 [details] [diff] [review]
WIP

If we can use <browser> element in mochitest-chrome, it seems that we can test this.
Attachment #669528 - Flags: review?(masayuki) → review+
Yes, chrome test should work. I was trying to get mochitest working (using new window) but
for some reason it wasn't successful.
Attached patch +testsSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=e6a6b69caaf0

It is painful to write focus handling tests.
Attachment #670379 - Flags: review?(masayuki)
Attachment #670379 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/43218e560c4a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
This patch has broken the Gaia keyboard for in-process apps.  See #800817
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?
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.)
How is someone using Social API going to run into this? Will it affect a lot of users?
(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).
> 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.
I guess the key is that checking for separate .top means this won't bite web content?
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.
> 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.)
If this is landed on aurora, bug 800817 needs to come with it.
Since I'm not really involved with Social API thing, if you need this on some branches, please
ask for approval.
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.
Attachment #669528 - Attachment is obsolete: true
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
Attachment #672551 - Flags: approval-mozilla-beta?
Attachment #672551 - Flags: approval-mozilla-aurora?
Attachment #672551 - Flags: approval-mozilla-beta?
Attachment #672551 - Flags: approval-mozilla-beta+
Attachment #672551 - Flags: approval-mozilla-aurora?
Attachment #672551 - Flags: approval-mozilla-aurora+
(if we see any further regressions from this, however unlikely, we'll strongly consider backing out rather than fixing forward)
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)
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.
QA Contact: virgil.dicu
(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.
Status: RESOLVED → VERIFIED
Depends on: 837069
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: