511449 - Some pages lose focus when opened in new tab (OS X)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

This bug was spun off from bug 510919 comment #28 and bug 510919
comment #29.  It happens only on OS X, in current trunk and 1.9.2
branch nightlies.

STR:

1) Make sure the following Tabs preference is turned on:

   "When I open a link in a new tab, switch to it immediately."

2) Right-click (or control-click) on the following URL, and choose to
   "Open Link in New Tab".

   http://www.coteindustries.com/articles/html_articles/tabindex.html

   Alternatively, click on this bug's 'URL' link (above) -- which also
   opens this URL in a new tab.

3) Notice that cmd-t, cmd-n and cmd-w all have no effect in the new
   tab.

   The problem disappears when you click somewhere on the tab's
   browser window.

The regression range for this is

firefox-2009-08-13-03-mozilla-central
firefox-2009-08-14-03-mozilla-central

I've confirmed that this regression is caused by my patch for bug
504450.  That patch also landed (at the same time) on the 1.9.2
branch, and caused the same regression there.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

The easiest way to fix this bug would be to back out my patch for bug
504450, but I'm not sure that's the right thing to do.  See bug 504450
comment #37 and following.

Comment 2

[Markus Stange [:mstange]]

Steven, sorry to be bothering you again, but could I ask you to test another thing? What happens when you back out both the patches for bug 504367 and bug 504450 and instead apply the one from bug 443178? Does that fix bug 504450, too?
(I really need to get a 10.5 machine.)

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> What happens when you back out both the patches for bug 504367 and
> bug 504450 and instead apply the one from bug 443178? Does that fix
> bug 504450, too?

I think that's very unlikely.  But I'll try to test later this week.

> (I really need to get a 10.5 machine.)

Yes :-)

Comment 4

[Markus Stange [:mstange]]

(In reply to comment #3)
> I think that's very unlikely.

Hmm, looking at the code another time, I agree... Re-routing the event into the right view has always been unconditional, my patch in bug 443178 only made re-routing into the right window unconditional, too, but that doesn't seem to have been the problem in bug 504450. I take back everything I said :)

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debugging patch

Neil recommended (bug 504450 comment #38) that I try to find a way to
save my patch for bug 504450, instead of simply backing it out.

That turns out to be pretty easy -- instead of not doing anything when
mView is invisible, I make it visible before making it the window's
first responder.  Somehow the OS behaves badly when we don't do this.

The only downside I can foresee is that a ChildView will sometimes be
made visible that shouldn't be.  But if this ever happens I think it
will be a layout bug, and having the wrong ChildView visible will make
the bug more obvious (and easily identifiable).

Neil, what do you think?  This bug shows (I think) that it's not
feasible to follow my suggestion from bug 504450 comment #7 (to fix
that bug by stopping cross-platform code from ever calling
nsIWidget::SetFocus() on an invisible widget).  And in a later comment
I'll show evidence that it's not feasible to fix this bug in
cross-platform code, either.

I've made this patch a debugging patch, to make it easier to test
with.  In particular, this patch also backs out the patch for bug
504367 (which stopped bug 504450 from being reproducible), so that you
can test with the STRs of both bug 504450 and bug 511449 (this bug).

Here's a tryserver build of my debugging patch:

https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla511449-debug/bugzilla511449-debug-macosx.dmg

Since the patch for bug 443178 has now landed on the trunk, I needed
to find the answer to Markus' question from comment #2.  The answer is
"no" -- landing the patch for bug 443178 and backing out the patches
for bug 504367 and bug 504450 doesn't fix bug 504450.  If you need
convincing on this score, here's a tryserver build to test with:

https://build.mozilla.org/tryserver-builds/[e-mail]-backout-504367-504450/backout-504367-504450-macosx.dmg

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stack trace of focusing a hidden nsChildView

Here's a stack trace of when the hidden view is focused using the STR
from comment #0.

It's difficult to interpret (or at least for me it is).  But a couple
of items in it (AutoJSSuspendNonMainThreadRequest::ResumeRequest() and
nsParser::ContinueInterruptedParsing()) suggest that the call to
nsChildView::SetFocus() can happen at any time, and (perhaps) not in
any particular order relative to other events.  So I suspect it'd be
difficult to change the timing of the call to nsChildView::SetFocus(),
so that it never takes place on an invisible nsChildView object.

In other words, I think this shows that it'd be difficult to fix this
bug in cross-platform code.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 395597 [details] [diff] [review]
Debugging patch

I've found that this bug and bug 504450 only happen on OS X 10.5.X and
above -- not on 10.4.X.  Which is evidence that both bugs are to some
extent Apple bugs (and need OS-X-specific fixes).

I've tested my debugging patch on all three platforms.  It fixes bug
504450 and bug 511449 on 10.5.8 and 10.6 (10A432), and doesn't cause
any problems (that I can see) on 10.4.11.

Comment 8

[Neil Deakin]

I don't think we should be showing a view within SetFocus. Instead we should be figuring out why SetFocus is being called on a hidden view.

The stack trace implies that some web page is calling focus() on an html element while is it loading, the child window containing the web page is not yet visible, yet the view for it is visible.

The Focus() method should only be called if docShell->GetVisibility returns true.

That would seem to mean that the widget's state and the view state or the native window state was inconsistent.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

So Neil, do you want to take this bug? -:)

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #8)

> the child window containing the web page is not yet visible, yet the
> view for it is visible.

I'm not sure what you mean by this.  But if "child window" means the
NSWindow associated with an nsCocoaWindow widget, and "view" means a
ChildView associated with an nsChildView widget, it's incorrect.  In
fact just the opposite is true -- what triggers this bug is SetFocus()
being called on an invisible nsChildView/ChildView.  When that
happens, the ChildView's NSWindow is already visible (in my debugging
patch, focus_hidden_window_break() doesn't get called).

> The Focus() method should only be called if docShell->GetVisibility
> returns true.

My patch for bug 504450 did this (or rather its equivalent on OS X)
... but that's what caused this bug.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

>> The Focus() method should only be called if docShell->GetVisibility
>> returns true.
>
> My patch for bug 504450 did this (or rather its equivalent on OS X)
> ... but that's what caused this bug.

This needs a bit of clarification:  My patch for bug 504450 made
nsChildView::SetFocus() only have any effect is nsChildView::mVisible
is PR_TRUE (on OS X).  (Your patch for bug 497565 had already done
this for nsCocoaWindow objects.)

I'm not entirely sure if this is the same as not calling the
nsIWidget::Focus() method when docShell->GetVisibility() returns
PR_FALSE (or whether it's the nsIWidget::Focus() method you were
talking about).

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debugging patch (updated to current code)

A new tryserver build will follow in a few hours.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stack trace of focusing a hidden nsChildView (with STR of bug 511449, updated)

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stack trace of focusing a hidden nsChildView (with STR of bug 504450, updated)

Comment 15

[Neil Deakin]

(In reply to comment #10)
> (In reply to comment #8)
> 
> > the child window containing the web page is not yet visible, yet the
> > view for it is visible.

By child window I mean nsChildView and by view I mean nsIView.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my updated debugging patch from comment #12:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla511449-debug-update/bugzilla511449-debug-update-macosx.dmg

Comment 17

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #10 and further reply to comment #8)

> By child window I mean nsChildView and by view I mean nsIView.

Thanks.

> The stack trace implies that some web page is calling focus() on an
> html element while is it loading, the child window containing the
> web page is not yet visible, yet the view for it is visible.

I've done some more debugging, and found this is true.

nsJSContext::EvaluateString() is called to "evaluate" the following:

    <!--
    document.form1.Element1.focus()
    //-->

(The HTML comment wrapper is ignored ... but I suppose that's because
this "string" is inside a <script> block (you can see it in this bug's
URL's "View : Page Source" window).)

When this happens the nsIView corresponding to "our"
nsChildView/ChildView object (the one that's focused while hidden) is
visible.  But nsChildView::mVisible is false (and [mView isHidden] is
true).

> That would seem to mean that the widget's state and the view state
> or the native window state was inconsistent.

The state (visibility) of "our" nsChildView/ChildView and its
corresponding nsIView are indeed inconsistent.  The "native window"
(the return value of [mView window]) is visible.  But I'm not sure
this is an inconsistency.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

(In further reply to comment #8)

> The Focus() method should only be called if docShell->GetVisibility
> returns true.

Which Focus() method?  nsGenericHTMLElement::Focus()?
nsFocusManager::Focus()?  Or something else?

Can we simply alter this method (whichever it is) to return early if
docShell->GetVisibility returns false?

Comment 19

[Markus Stange [:mstange]]

I've got a patch that fixes this bug and bug 510919. I just need to write a test.

Comment 20

[Markus Stange [:mstange]]

Attachment: v1

I need to test this more and see what the tryserver says.

Comment 21

[Markus Stange [:mstange]]

This patch causes a large number of focus tests to fail.

Comment 22

[Markus Stange [:mstange]]

(In reply to comment #8)
> That would seem to mean that the widget's state and the view state or the
> native window state was inconsistent.

They are, because nsDocumentViewer.cpp calls Show directly on the widget without involving the view. I talked to roc and he said that it's probably not worth fixing that mess now.
Instead, the docshell should ask the widget about its visibility, which has a more accurate idea about it. That's what my patch does, and it seems to work quite well.

However, the test failures are scaring me a little.

This is a log of the test failures on Mac:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252388006.1252395439.27158.gz#err1

Neil, I was able to fix the chrome tests by sprinkling some waitForFocus and executeSoon incantations over them, but I have the feeling that this is not the right way to go. The patch seems to break some assumption about focus changing synchronously. Do you think we should do it anyway?
I can attach a patch with the test changes.

Another way to fix this bug would be to turn around Steven's suggestion; instead of making the view visible in SetFocus, we could focus it when it's made visible. 
However, doing that unconditionally doesn't seem to work. Focus ends up on a view whose view manager doesn't have an event observer, so the events we dispatch into Gecko never get far enough.

What *does* work is to focus only those views in nsChildView::Show() where there has been an attempt to focus them while they were hidden. I'll attach a patch that does that. But it's not a very clean approach, because it breaks the whole concept of "Focus should only be stored in one place". So I think we should still look for a better way to fix this bug.

Comment 23

[Markus Stange [:mstange]]

Attachment: messy delayed focusing

as described in the previous comment

Comment 24

[Neil Deakin]

(In reply to comment #22)
> Neil, I was able to fix the chrome tests by sprinkling some waitForFocus and
> executeSoon incantations over them, but I have the feeling that this is not the
> right way to go. The patch seems to break some assumption about focus changing
> synchronously. Do you think we should do it anyway?

The problem is partially that. I investigated the first test that breaks (for bug 304188) for a bit. It runs on load and now fails because the docShell visibility is false, so various things don't work.

Naturally, before, the docshell visibility was returning true even though it really wasn't.

Can you debug and determine whether the visibility is actually correct? If so, the tests probably just need to wait to be focused.

Interestingly, it's mostly findbar related tests that fail. Do any of these tests fail randomly currently?

Comment 25

[Neil Deakin]

mfinkle tells me this also breaks fennec. It does some weird things with tabs and event retargeting that would be affected by this.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #20)
> Created an attachment (id=399159) [details]
> v1
> 
> I need to test this more and see what the tryserver says.

This patch breaks page focusing in Fennec. Fennec has a collection of <browser>
elements, but they are all hidden. We use canvas and drawWindow to render the
<browser> content to a larger, tiled canvas for actual display.

Since the <browser> elements are hidden, this patch doesn't set focus to the
widgets on the webpage.

Comment 27

[Markus Stange [:mstange]]

(In reply to comment #24)
> Can you debug and determine whether the visibility is actually correct?

Hmm, how would I do that? The "correct visibility" is determined by what [mView isHidden] returns, which is exactly what the docshell asks for, no?

> Interestingly, it's mostly findbar related tests that fail. Do any of these
> tests fail randomly currently?

No, none of them do. At least I haven't found any bugs about them. The only bug that I found is bug 497844 which mentions browser_tabfocus.js.

Comment 28

[Markus Stange [:mstange]]

Attachment: third approach

Here's another idea: Whenever the document viewer changes widget visibility, ask the focus manager to focus the correct widget.

The only problem is that this patch, just like the "messy delayed focusing" patch, causes mochitests to lose focus after the test_handlerApps.xhtml test...

Comment 29

[Markus Stange [:mstange]]

Ah, I see what was happening: test_handlerApps.xhtml opens iCal and never gets focus back to Firefox, but that usually doesn't matter because it's the second-but-last test that's run. However, the test that I added in /widget/ would be run after it and breaks.

So Neil, what do you think about the third approach?

Comment 30

[Neil Deakin]

(In reply to comment #27)
> (In reply to comment #24)
> > Can you debug and determine whether the visibility is actually correct?
> 
> Hmm, how would I do that? The "correct visibility" is determined by what [mView
> isHidden] returns, which is exactly what the docshell asks for, no?
> 

When Show() is being called, what does the OS think is focused, and what does the focus manager think is focused? If different, why the inconsistency?

I'm mainly interested in why this bug occurs. Also, why the bug only occurs on Mac. I'll investigate the latest patch tomorrow but I'm relucant to add focus hacks unless we know why they are needed. The days of focus hacks are in the past in my opinion.

Comment 31

[Markus Stange [:mstange]]

(In reply to comment #30)
> When Show() is being called, what does the OS think is focused, and what does
> the focus manager think is focused? If different, why the inconsistency?

There are two nested widgets at the browser boundary; let's call them outer and inner widget.
When Show is called the first time, in order to hide the outer widget of the old document, focus is on the link that was clicked and on the inner widget of the old document. As soon as the outer widget is hidden, focus goes to the window.

Then updateCurrentBrowser focuses "newBrowser", and the native focus goes to a mysterious inner widget that I haven't quite identified yet.

Now the opened page calls focus() on that textfield, but its widget is still hidden. The focus manager asks the docshell whether it's visible, it reports "yes" (which is a lie), and the focus manager attempts to focus the inner widget of the new page. Because the widget is hidden, this attempt is ignored.

At this point the PresShell of that mysterious newBrowser widget is destroyed, and the event observer on its view manager is reset.

Now the inner widget of the new page is finally made visible. But it doesn't get focus. Instead, focus is still in the mysterious newBrowser widget. If you press Cmd+W, the event is dispatched from that mysterious widget, but it doesn't get far because its view manager doesn't have an observer any more.

> I'm mainly interested in why this bug occurs. Also, why the bug only occurs on
> Mac.

Maybe the other platforms don't dispatch key events from the focused widget?

Comment 32

[Markus Stange [:mstange]]

Attachment: fourth approach

When hidden, focus the next visible superview instead.

Comment 33

[Neil Deakin]

(In reply to comment #31)

OK, great analysis!

> Then updateCurrentBrowser focuses "newBrowser", and the native focus goes to a
> mysterious inner widget that I haven't quite identified yet.

Is this an about:blank document? Mozilla has a habit of creating blank documents and then destroying them again often simply because one accesses things in a certain way.

> 
> Now the opened page calls focus() on that textfield, but its widget is still
> hidden. The focus manager asks the docshell whether it's visible, it reports
> "yes" (which is a lie), and the focus manager attempts to focus the inner
> widget of the new page. Because the widget is hidden, this attempt is ignored.

So the page's widget really is visible?

What would happen if we left the docshell GetVisibility function as is, and just replaced calls from the focus manager to widget IsVisible?


> At this point the PresShell of that mysterious newBrowser widget is destroyed,
> and the event observer on its view manager is reset.

Just to clarify, you're referring to the inner widget here right? And it's being destroyed and is about to be replaced with the, already visible, real inner widget?

Is WindowHidden being called on this destroyed widget?


This analysis makes me think that the way to go here is what I described above:

- don't change docshell GetVisibility
- if there's a widget, check it for visibility instead from within the focus manager.
- we'll still need to look up the hierarchy for hidden views in case of decks. AreAncestorViewsVisible may be all we need here.

Comment 34

[Markus Stange [:mstange]]

(In reply to comment #33)
> (In reply to comment #31)
> 
> OK, great analysis!
> 
> > Then updateCurrentBrowser focuses "newBrowser", and the native focus goes to a
> > mysterious inner widget that I haven't quite identified yet.
> 
> Is this an about:blank document? Mozilla has a habit of creating blank
> documents and then destroying them again often simply because one accesses
> things in a certain way.

Could be! That would explain it.

> > Now the opened page calls focus() on that textfield, but its widget is still
> > hidden. The focus manager asks the docshell whether it's visible, it reports
> > "yes" (which is a lie), and the focus manager attempts to focus the inner
> > widget of the new page. Because the widget is hidden, this attempt is ignored.
> 
> So the page's widget really is visible?

Which widget? The outer widget is already visible, but the inner one is still hidden. With the v1 patch applied, the docshell would report "no" at this point.

> What would happen if we left the docshell GetVisibility function as is, and
> just replaced calls from the focus manager to widget IsVisible?

We'd be asking the very same widget. I've tested it, and I also get the same test failures as with the "v1" patch. And Fennec wouldn't like it.

> > At this point the PresShell of that mysterious newBrowser widget is destroyed,
> > and the event observer on its view manager is reset.
> 
> Just to clarify, you're referring to the inner widget here right?

Yes.

> And it's being destroyed

Not yet... only after about 8 seconds.

> and is about to be replaced with the, already visible, real
> inner widget?

The real inner widget hasn't even been created yet, so no. But the real outer widget is already created and visible.

> Is WindowHidden being called on this destroyed widget?

Since the widget hasn't really been destroyed yet, I can't answer this question :-)
When newBrowser is focused, it's being called on the docshell whose vm's root widget is the newBrowser's inner widget.
When the input field in the new tab is focused, it's being called on the docshell whose vm's root widget is the not-yet-visible new real inner widget.

Comment 35

[Neil Deakin]

So this patch doesn't cause any problems? What nsChildView ends up getting focused instead?

Comment 36

[Markus Stange [:mstange]]

> So this patch doesn't cause any problems? 

The fourth patch? No, that doesn't cause any problems. The second and third patches don't either, but they're not as beautiful.

> What nsChildView ends up getting focused instead?

The outer widget of the new tab. The inner one isn't visible yet, but the outer is.

Comment 37

[Neil Deakin]

Attachment: another approach

I was inspired by the patches here and debugged this a bit more. What's happening:

1. Click on link and a new blank tab opens (about:blank).
2. The tabbrowser switches focus to this new tab.
3. The real url to display in the tab is set and the about:blank window is hidden, but not shown yet.
4. element.focus() is called on the real url page, which hasn't finished loading yet. When it tries to set the widget's focus, it fails because it is hidden.
5. nsGlobalWindow::SetReadyForFocus is called when the page is shown, but returns early because mNeedsFocus has already been cleared at step 4.

Thus, when the window is actually shown, the focus doesn't get adjusted.

The solution is similar to Markus' third patch, but moves the handling entirely into WindowShown. Instead we always call WindowShown and only call EnsureCurrentWidgetFocused when mNeedsFocus is false.

Comment 38

[Neil Deakin]

Markus/Steven: can you try out this patch and see if there are any problems with it?

Comment 39

[Markus Stange [:mstange]]

Sure.

Comment 40

[Markus Stange [:mstange]]

I didn't test much, but up to now everything seems to be working. The fix looks good.

Comment 41

[Markus Stange [:mstange]]

I pushed this to the tryserver together with some other stuff, and it looks like some passwordmgr tests had problems. The test run timed out on both Mac and Linux during test_prompt_async.html. On Windows it timed out during test_selectors_on_anonymous_content.html, but that's probably unrelated.

Mac: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1254286760.1254295025.21392.gz

Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1254286763.1254295018.21311.gz#err0

Windows: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1254286760.1254295542.27018.gz

Comment 42

[Neil Deakin]

I don't see any problems with test_prompt_async.html when run locally.

Comment 43

[Neil Deakin]

Comment on attachment 402585 [details] [diff] [review]
another approach

Actually, that failure is just bug 482175. So this can just be reviewed. Don't think I can test this though.

Comment 44

[Markus Stange [:mstange]]

(In reply to comment #43)
> Don't think I can test this though.

You can, using the test from my v1 patch.

Comment 45

[Neil Deakin]

OK, thanks! I'll include the test from it into my patch when checking in.

Comment 46

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/8727bcd18715

Comment 47

[Joe Drew (not getting mail)]

This is annoying enough that we should block on it.

Comment 48

[Justin Dolske [:Dolske]]

So, this is blocking1.9.2, but the last patch includes an interface change.

Enn: can we get an updated patch to avoid that, or is this a change we have to swallow?

Comment 49

[Neil Deakin]

Attachment: 1.9.2 version which doesn't change interfaces

Comment 50

[Neil Deakin]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7d1b6dbae5d6

Comment 51

[Boris Zbarsky [:bzbarsky]]

This seems to be failing mochitests on 1.9.2:

ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug299673.html | unexpected events - got " : Test with browser.link.open_newwindow = 2\nSELECT(Select1): focus \nSELECT(Select1): change \n : >>> OpenWindow\n: focus top-doc\n: blur top-doc\n: focus popup-doc\nINPUT(popupText1): focus \n : <<< OpenWindow\nSELECT(Select1): blur \n", expected " : Test with browser.link.open_newwindow = 2\nSELECT(Select1): focus \nSELECT(Select1): change \n : >>> OpenWindow\n: blur top-doc\n: focus popup-doc\nINPUT(popupText1): focus \n : <<< OpenWindow\nSELECT(Select1): blur \n"

Comment 52

[Daniel Holbert [:dholbert]]

(In reply to comment #51)
> This seems to be failing mochitests on 1.9.2:

This seems to have caused orange in all 3 cycles since checkin on Windows & Linux.
On Mac, we had 2 orange cycles, but the most recent cycle was surprisingly green.

Comment 53

[Neil Deakin]

A fix was checked in for the issue above.

Comment 54

[Henri Sivonen (:hsivonen) (on leave)]

I saw the test case for this bug (docshell/test/test_bug511449.html) time out when running all of mochitest-plain with the HTML5 parser enabled (bug 547180). However, when I run the test individually, it passes. Any guidance on how to troubleshoot?

Comment 55

[Henri Sivonen (:hsivonen) (on leave)]

Hmm. Maybe a screensaver has activated and taken away the focus.