Closed Bug 902715 Opened 11 years ago Closed 11 years ago

window.focus() doesn't work in electrolysis

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27

People

(Reporter: billm, Assigned: dvander)

References

Details

Attachments

(1 file, 2 obsolete files)

v3
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.
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.
Attached patch focus.patch (obsolete) — — Splinter Review 
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.
Assignee: nobody → dvander
Status: NEW → ASSIGNED
Attachment #802004 - Flags: review?(bugs)
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-
Attached patch focus.patch (obsolete) — — Splinter Review 
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 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-
Attached patch v3 — — Splinter Review 

        Attachment #802654 -
        Attachment is obsolete: true

        Attachment #807410 -
        Flags: review?(enndeakin)

    
    

    
  
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+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6cf952dfc31b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: