Closed Bug 1206559 Opened 4 years ago Closed 4 years ago

[e10s] window.focus() in child frame doesn't redirect raise window request to parent process

Categories

(Testing :: Mochitest, defect)

defect
Not set
Points:
2

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: xidorn, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

It seems to be unreliable to use waitForFocus in content process. Tests which rely on getting focus via this function is usually stuck after "must wait for focus".
Summary: waitForFocus doesn't work reliably on content process → waitForFocus doesn't work reliably in content process
A very simple example of this issue is layout/forms/test/test_bug564115.html which uses waitForFocus for switching focus back and forth. It reliably gets stuck at the second waitForFocus call because no one else can focus the window for it.
It looks like nsGlobalWindow::FocusOuter doesn't handle the case where someone tries to call window.focus() on a child iframe when in a content process.
Assignee: nobody → enndeakin
Points: --- → 2
It seems adding
> aMessage.target.ownerDocument.defaultView.focus();
to the handler of "SpecialPowers.Focus" message in SpecialPowersObserver works.
Attached patch Send raise request to parent (obsolete) — Splinter Review
It seems there are some other issues make waitForFocus not reliable in content process... I cannot make my test work with the proposed patch. (And the approach I figured out in comment 3 doesn't work now for me as well)

It seems even if the window is raised to the top, it is still possible that the content doesn't get the focus event.
A try run with the patch and enabled for e10s worked ok:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65ab7198a94a
Attachment #8663680 - Flags: review?(bugs)
Comment on attachment 8663680 [details] [diff] [review]
Send raise request to parent

Don't we rather want to fix nsPuppetWidget
http://mxr.mozilla.org/mozilla-central/source/widget/PuppetWidget.cpp?rev=b96e95b7b4b9&mark=258-263#258
And simplify cases like SetActiveWindow handling in 
nsGlobalWindow::FocusOuter (Or perhaps SetActiveWindow should do something only in parent process, though, nsGlobalWindow::FocusOuter does call either 
SetActiveWindow or SendRequestFocus).

Feel free to re-ask review if you think your approach is better, but please explain why. (I'd like to push as much as possible the content process specific stuff to nsPuppetWidget)
Attachment #8663680 - Flags: review?(bugs) → review-
Attached patch Better fixSplinter Review
Attachment #8663680 - Attachment is obsolete: true
Attachment #8667030 - Flags: review?(bugs)
Comment on attachment 8667030 [details] [diff] [review]
Better fix

RaiseWindow isn't used often and this seem to make it work rather nicely.

I think I'd null check mTabChild (since proving it can't be null when SetFocus is called is a bit hard).
So


if (aRaise && mTabChild) {
  mTabChild->SendRequestFocus(true);
}
Attachment #8667030 - Flags: review?(bugs) → review+
Could you land the patch?
Flags: needinfo?(enndeakin)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a67c866d14d07921d6f35e22f45b78846d9500b
Bug 1206559, forward PuppetWidget::SetFocus request to the parent process, r=smaug
https://hg.mozilla.org/mozilla-central/rev/5a67c866d14d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Flags: needinfo?(enndeakin)
Blocks: 1212690
Blocks: 1212692
Summary: waitForFocus doesn't work reliably in content process → [e10s] window.focus() in child frame doesn't redirect raise window request to parent process
See Also: → 1203079
You need to log in before you can comment on or make changes to this bug.