Closed
Bug 902715
Opened 12 years ago
Closed 12 years ago
window.focus() doesn't work in electrolysis
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: billm, Assigned: dvander)
References
Details
Attachments
(1 file, 2 obsolete files)
|
4.51 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
If I set window.tabs.remote=true, then window.focus() is broken. Here's a simple test case:
<html>
<script>
function run()
{
setTimeout(function(){ window.focus(); }, 2000);
}
</script>
<a href="#" onclick="run()">click</a>
</html>
If you click the link, then quickly click the URL bar, the focus should switch back to the browser element after 2 seconds. With browser.tabs.remote=true, it doesn't.
|
||
Comment 1•12 years ago
|
||
It is actually not quite clear focus should move from browser chrome to content in that case,
but sure, if FF does that right now, we should perhaps do it.
|
Assignee | |
Comment 2•12 years ago
|
||
Yeah, this is kind of annoying for dogfooding. Google.com uses .focus() for their search box.
This patch appears to fix the problem, though I don't the code that well so I'm not sure if it's the right approach.
|
||
Comment 3•12 years ago
|
||
Comment on attachment 802004 [details] [diff] [review]
focus.patch
It isn't. UpdateCanvasFrame is used only to update the rendering of focus (the dotted outline) on the frame.
Somewhere, the code for window.focus() is stopping early before it has a chance to focus the content. When I print out a log, I see no focus manager methods being made when window.focus() is called, suggesting that nsGlobalWindow::Focus is the place to start looking.
Attachment #802004 -
Flags: review?(bugs) → review-
|
|
Assignee | |
Comment 4•12 years ago
|
||
nsGlobalWindow::Focus() tries to find the parent of the global window, which in e10s is the parent process. This patch puts a TabChild check there, and sends a focus message to the parent process. This appears to actually fix the test case in comment #0 this time. google.com's input.focus() doesn't work with this patch but I'll look at that separately.
Attachment #802004 -
Attachment is obsolete: true
Attachment #802654 -
Flags: review?(enndeakin)
|
|
||
Comment 5•12 years ago
|
||
Comment on attachment 802654 [details] [diff] [review]
focus.patch
This patch doesn't appear to to do what window.focus() currently does for a content window. RequestFocus should have similar code to what :Focus() does.
>+ else if (TabChild *child = GetTabChildFrom(this)) {
>+ child->SendRequestFocus();
>+ }
The 'canFocus' flag is ignored here. It should be used to indicate whether the window should be raised on not.
>+TabParent::RecvRequestFocus()
>+{
...
>+ if (!content->OwnerDoc()->GetWindow()->IsActive()) {
>+ return true;
>+ }
This prevents anything from happening when the window is in the background.
> + fm->SetFocus(node, 0);
>+ return true;
This doesn't pass the same flags as the existing code.
Attachment #802654 -
Flags: review?(enndeakin) → review-
|
|
Assignee | |
Comment 6•12 years ago
|
||
Attachment #802654 -
Attachment is obsolete: true
Attachment #807410 -
Flags: review?(enndeakin)
|
|
||
Comment 7•12 years ago
|
||
Comment on attachment 807410 [details] [diff] [review]
v3
>+ /**
>+ * Request that the parent process move focus to the child process.
>+ */
>+ RequestFocus(bool canFocus);
Document the argument here, and possibly call it canRaise instead.
>+bool
>+TabParent::RecvRequestFocus(const bool& aCanFocus)
>+{
>+ nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID);
You can just use nsFocusManager::GetFocusManager() here instead.
Attachment #807410 -
Flags: review?(enndeakin) → review+
|
|
Assignee | |
Comment 8•12 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/265b3d9cf201 for breaking the build on at least Windows.
Bustage looked like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=28176910&tree=Mozilla-Inbound
|
|
Assignee | |
Comment 10•12 years ago
|
||
|
||
Comment 11•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
|
||
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•