424906 - When loading a page via link-click, initial focus is an invisible view (page not focused)

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

On branch, when you click a link, a page loads, and focus is given to the page after if finishes loading.  On trunk, this is not the case.

STR:
1. Load http://www.google.com.  While you're there, make a bookmark for the "Advanced Search" link
2. Click the link for "Advanced Search"
--> Observe focus isn't in the first text field (no blue focus ring, requires several tabs to get there)
3. Go back to http://www.google.com
4. Click your bookmark for "Advanced Search"
--> Observe focus *is* in the first text field

Repeat with pages that don't have text fields, like cbo and cbo/features, and you'll see the same problem.

I'm aware of the similarities with bug 185385, but 

1) This happens even with FKA off
2) With FKA on, the first bookmark doesn't get focus (in fact, a tab-press seems to focus the first bookmark, or sometimes the second bookmark item, skipping the first altogether), and space does nothing
3) The page doesn't get focus after it finishes loading (as bug 185385 comment 16 seems to indicate)
4) It seems as if focus ends up in an invisible view coterminous (but often "after", in the tab order) the first bookmark
5) Bug 185385 doesn't seem to differentiate between link-click loads and non-link-click loads (in fact, the latest STR there use non-link-click loads), but this bug exclusively affects link-click loads.

It may be that there's been a subsequent Core bug that's covered up bug 185385.  Whatever the case is, though, we need to get to the bottom of this and fix it for 2.0.

Comment 1

[philippe (part-time)]

For 2) above, and with FKA is ON - hide the BM bar, the first tab-press puts the focus on the first close button in the tab bar.

On some sites (and I'm not clear exactly what is causing the issue - possibly iframes), the page is focussed, one can page Up or Down with the space bar. example is http://www.guardian.co.uk/world

Comment 2

[philippe (part-time)]

Two more notes
1. I did not experience this while using 10.4.x PPC.

2. When I migrated to a 10.5/Intel Mac, I took some notes about this:
It appears to be a regression sort of thing, although back then I assumed it to be an incarnation of bug 185385:
regression:
2007080100 (2.0a1pre)  works
2007080201 (2.0a1pre)  fails
[test was paging (spacebar) down a page after following a link, using FAYT on that page, and trying to tab to a link, all with FKA on]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-08-01+00%3A00%3A00&maxdate=2007-08-02+00%3A00%3A00&cvsroot=%2Fcvsroot

Cairo update ??

back then I also noted that it seemed a bit the opposite of bug 280390. Not sure how I came to that conclusion.

Comment 3

[Eiichi]

It seems to be reproducible more simply (10.4.11 Intel).
1. Open a tab for a link.
2. Open a page instead of a tab for the same link in the same window.
3. Not a page but the URL field is focused.

And once this problem occurs, that's the same when I go next or back.

Comment 4

[Sean Murphy]

Looking quickly at this, it seems to occur when Gecko hides a |ChildView| (nsChildView::Show(PR_FALSE)) during the process of loading and displaying a new page.  If the hidden ChildView is the BrowserWindow's firstResponder, responder status is then just transfered up to the window itself as a fallback mechanism, since the hidden ChildView can no longer accept firstResponder and must resign.

So, focus becomes lost when a hidden ChildView is the BrowserWindow's active firstResponder.

Like the above comment, I found that Smokey's STR can be simplified to:

1. Open a new browser window.
2. Go to google.com.
3. Ensure the search text field on the page is the first responder (has focus).

At this point, -[BrowserWindow firstResponder] = ChildView <0x188eae80>.

4. Click on the "Advanced Search" link on the page.
5. During the display of the new page, the above ChildView is hidden (stack frame #6 below) which ultimately causes focus to fall back to the BrowserWindow.  We then notice the focus change away from Gecko and call setBrowserActive:NO.
6. Press the down arrow to try and scroll the page, notice the beep.  Here -[BrowserWindow firstResponder] = BrowserWindow <0x18814d40>.

Stack trace during step 5:

> #0  -[CHBrowserView setActive:] (self=0x1882fd80, _cmd=0x924784fc, aIsActive=0 '\000') at CaminoObj/camino/src/embedding/CHBrowserView.mm:1418
> #1  0x00004a05 in -[BrowserWrapper setBrowserActive:] (self=0x1882fb40, _cmd=0x129b58, inActive=0 '\000') at CaminoObj/camino/src/browser/BrowserWrapper.mm:352
> #2  0x0001f6c2 in -[BrowserWindowController focusChangedFrom:to:] (self=0x188027e0, _cmd=0x12b8c8, oldResponder=0x1a412b00, newResponder=0x18814d40) at CaminoObj/camino/src/browser/BrowserWindowController.mm:4891
> #3  0x0006a06a in -[BrowserWindow makeFirstResponder:] (self=0x18814d40, _cmd=0x9245f7c8, responder=0x0) at CaminoObj/camino/src/browser/BrowserWindow.mm:68
> #4  0x9171dc40 in -[NSView(NSInternal) _setHidden:setNeedsDisplay:] ()
> #5  0x9171d90d in -[NSView _setHidden:] ()
> #6  0x18581257 in nsChildView::Show (this=0x1c5d8940, aState=0) at mozilla/widget/src/cocoa/nsChildView.mm:775

Note: At this point, this.mView = 0x188eae80, which matches the window's firstResponder while on google.com.  aState is the parameter to Show(), and in this case the view is being hidden.

> #7  0x1609d685 in DocumentViewerImpl::Hide (this=0x1c51d380) at mozilla/layout/base/nsDocumentViewer.cpp:1937
> #8  0x1609c7b2 in DocumentViewerImpl::Destroy (this=0x1c51d380) at mozilla/layout/base/nsDocumentViewer.cpp:1443
> #9  0x1609fdc1 in DocumentViewerImpl::Show (this=0x1a42ba80) at mozilla/layout/base/nsDocumentViewer.cpp:1842
> #10 0x160bf0a4 in nsPresContext::EnsureVisible (this=0x1a47c660, aUnsuppressFocus=0) at mozilla/layout/base/nsPresContext.cpp:1460
> #11 0x160ce3bd in PresShell::UnsuppressAndInvalidate (this=0x1adb400) at mozilla/layout/base/nsPresShell.cpp:4290
> #12 0x160ce6e6 in PresShell::UnsuppressPainting (this=0x1adb400) at mozilla/layout/base/nsPresShell.cpp:4350
> #13 0x1609ab04 in DocumentViewerImpl::LoadComplete (this=0x1a42ba80, aStatus=0) at mozilla/layout/base/nsDocumentViewer.cpp:1013
> #14 0x15db2ae0 in nsDocShell::EndPageLoad (this=0x188b9610, aProgress=0x188b9624, aChannel=0x167a9c0, aStatus=0) at mozilla/docshell/base/nsDocShell.cpp:5025
> #15 0x15dd2c85 in nsWebShell::EndPageLoad (this=0x188b9610, aProgress=0x188b9624, channel=0x167a9c0, aStatus=0) at mozilla/docshell/base/nsWebShell.cpp:1013
> #16 0x15da40eb in nsDocShell::OnStateChange (this=0x188b9610, aProgress=0x188b9624, aRequest=0x167a9c0, aStateFlags=131088, aStatus=0) at mozilla/docshell/base/nsDocShell.cpp:4930
> #17 0x15de9366 in nsDocLoader::FireOnStateChange (this=0x188b9610, aProgress=0x188b9624, aRequest=0x167a9c0, aStateFlags=131088, aStatus=0) at mozilla/uriloader/base/nsDocLoader.cpp:1235
> #18 0x15de96b5 in nsDocLoader::doStopDocumentLoad (this=0x188b9610, request=0x167a9c0, aStatus=0) at mozilla/uriloader/base/nsDocLoader.cpp:858
> #19 0x15de98aa in nsDocLoader::DocLoaderIsEmpty (this=0x188b9610) at mozilla/uriloader/base/nsDocLoader.cpp:763
> #20 0x15dea3b5 in nsDocLoader::OnStopRequest (this=0x188b9610, aRequest=0x1a4c3e70, aCtxt=0x0, aStatus=0) at mozilla/uriloader/base/nsDocLoader.cpp:679
> #21 0x179574e1 in nsLoadGroup::RemoveRequest (this=0x188b95a0, request=0x1a4c3e70, ctxt=0x0, aStatus=0) at mozilla/netwerk/base/src/nsLoadGroup.cpp:688
> #22 0x1633f5aa in nsDocument::DoUnblockOnload (this=0x1a8a200) at mozilla/content/base/src/nsDocument.cpp:5643
> #23 0x16349ffc in nsDocument::UnblockOnload (this=0x1a8a200, aFireSync=1) at mozilla/content/base/src/nsDocument.cpp:5592
> #24 0x1634474c in nsDocument::DispatchContentLoadedEvents (this=0x1a8a200) at mozilla/content/base/src/nsDocument.cpp:2845
> #25 0x163523dd in nsRunnableMethod<nsDocument>::Run (this=0x1a482a20) at nsThreadUtils.h:261
> #26 0x0031fc34 in nsThread::ProcessNextEvent (this=0x16662a0, mayWait=0, result=0xbfffe6a4) at mozilla/xpcom/threads/nsThread.cpp:510
> #27 0x002aaa08 in NS_ProcessPendingEvents_P (thread=0x16662a0, timeout=20) at nsThreadUtils.cpp:180
> #28 0x185ad172 in nsBaseAppShell::NativeEventCallback (this=0x18830160) at mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:107
> #29 0x185735f0 in nsAppShell::ProcessGeckoEvents (aInfo=0x18830160) at mozilla/widget/src/cocoa/nsAppShell.mm:305
> ...

Comment 5

[Stuart Morgan]

Attachment: fix [checked in]

If the window is itself the first responder when pageload finishes, we should restore focus back to Gecko.

Comment 6

[Eiichi]

I applied an attachment (id=319512) and tested with my own build, 
but the problem seems not to be resolved.
It is reproducible on 10.4.11 (Intel).

1. Open a tab for a link.
2. Close a new opened tab.
3. Open a page instead of a tab for the same link in the same window.
4. Not a page but the URL field is focused.

Comment 7

[philippe (part-time)]

10.5.2 Intel
This patch does the job nicely. All the case where I was seeing the unfocussed content (page) are now working correctly.

I cannot reproduce the issue seen by Eiichi (I don't remember ever seeing that when I was using trunk on 10.4/ppc).

Comment 8

[Sean Murphy]

Comment on attachment 319512 [details] [diff] [review]
fix [checked in]

r=murph.

I also cannot reproduce the issue in comment #6; perhaps it is only present on 10.4.

Comment 9

[Stuart Morgan]

Comment on attachment 319512 [details] [diff] [review]
fix [checked in]

I'll test 10.4 when I get a chance and see if there's follow-up work needed, but this is an improvement regardless.

Comment 10

[Eiichi]

Attachment: Screenshot

For the records,

Case 1
Turn on "Full keyboard access : Text boxes and lists only"
under "System Preferences > Keyboard & Mouse > Keyboard Shortcuts"
In this case, the issue in comment #6 is reproducible.

Case 2
Turn on "Full keyboard access : All controls"
In this case, not the URL field but Toolbar icon is focused.
See screenshot.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #10)
> Created an attachment (id=320059) [details]
> Screenshot
> 
> Case 2
> Turn on "Full keyboard access : All controls"
> In this case, not the URL field but Toolbar icon is focused.
> See screenshot.

That sounds more like bug 352582.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 319512 [details] [diff] [review]
fix [checked in]

sr=pink

Comment 13

[Stuart Morgan]

Comment on attachment 319512 [details] [diff] [review]
fix [checked in]

Landed on trunk; leaving the bug open for further investigation.

Comment 14

[philippe (part-time)]

(In reply to comment #10)

> Case 1
> Turn on "Full keyboard access : Text boxes and lists only"
> under "System Preferences > Keyboard & Mouse > Keyboard Shortcuts"
> In this case, the issue in comment #6 is reproducible.

I could reproduce this on 10.5.2, but only with those settings in the system prefs.
And the actions must be performed with the mouse.

STR 
1. open page with links
2. cmd-click link too open in new tab
3. go to new tab, close it (with the mouse, that seems to be the key)
4. click same link as in (2) (not cmd-click)

A.R. the focus is on the location bar.
E.R. focus is on the page.

If the link in (2) and (4) had already been clicked before step (2), no bad things happened here.

Comment 15

[Eiichi]

(In reply to comment #14)

> And the actions must be performed with the mouse.

On 10.4 it's reproducible with keyboard shortcut fot close the tab (without the mouse).

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Please retest this in tomorrow (2008-09-20) Cm2-M1.9 nightly builds (especially on 10.4), since bug 152987 and friends have overhauled our keyboard loop.

Comment 17

[philippe (part-time)]

(In reply to comment #16)

comment 14 seems to be fixed now.
No other regression or change to note, so far.

Comment 18

[Eiichi]

I confirmed that this bug has been fixed with my own build on 10.4.
And I'll test this in official nightly builds (2008-09-20) Cm2-M1.9.

Comment 19

[Eiichi]

I retested this in official nightly builds (2008-09-20) Cm2-M1.9, and
I confirmed that this bug has been fixed on 10.4.11.

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

--> FIXED by Stuart's patch + Sean's bug 152987 and friends.