The default bug view has changed. See this FAQ.

focus contention with two visible html content areas

VERIFIED FIXED

Status

()

Core
Event Handling
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: mixedpuppy, Assigned: smaug)

Tracking

(Depends on: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17+ verified, firefox18+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

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

Comment 2

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

Comment 4

5 years ago
Created attachment 669528 [details] [diff] [review]
WIP

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.
(Assignee)

Comment 6

5 years ago
And I need to figure out how the test this all automatically.
I was hoping something simple but apparently no... :/
(Assignee)

Comment 7

5 years ago
Comment on attachment 669528 [details] [diff] [review]
WIP

Still haven't figured out reliable way to test this.
Attachment #669528 - Flags: review?(masayuki)
(Assignee)

Comment 8

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=98ac763a6e29
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+
(Assignee)

Comment 10

5 years ago
Yes, chrome test should work. I was trying to get mochitest working (using new window) but
for some reason it wasn't successful.
(Assignee)

Comment 11

5 years ago
Created attachment 670379 [details] [diff] [review]
+tests

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+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/mozilla-central/rev/43218e560c4a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
tracking-firefox17: --- → ?
This patch has broken the Gaia keyboard for in-process apps.  See #800817
Depends on: 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?
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → fixed
tracking-firefox17: ? → +
tracking-firefox18: --- → +
(Reporter)

Comment 17

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

Comment 23

5 years ago
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
Blocks: 798141
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
Attachment #672551 - Flags: approval-mozilla-beta?
Attachment #672551 - Flags: approval-mozilla-aurora?

Updated

5 years ago
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)
I landed the various changesets separately:
https://hg.mozilla.org/releases/mozilla-beta/rev/ee2fdf44ffb3
https://hg.mozilla.org/releases/mozilla-beta/rev/3234f1d43ed8
https://hg.mozilla.org/releases/mozilla-beta/rev/b1f37231391a
https://hg.mozilla.org/releases/mozilla-beta/rev/3b884e78c184
status-firefox17: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/c9c6e9647183
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8f505668b0c
https://hg.mozilla.org/releases/mozilla-aurora/rev/adc8c88be25a
https://hg.mozilla.org/releases/mozilla-aurora/rev/915fe0f56251
status-firefox18: affected → fixed
status-firefox19: fixed → ---
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.
status-firefox18: fixed → verified
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
status-firefox17: fixed → verified
Depends on: 837069
You need to log in before you can comment on or make changes to this bug.