574560 - Focus moves into a dead view after editing an anchor in the url and pressing enter

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: General, defect)

Description

[philippe (part-time)]

STR:
1. load http://wiki.csswg.org/spec/css2.1#issue-101
2. change the anchor number in the url from 101 to 71 and press enter
3. try paging down (or up) with spacebar or page down key.

AR: nothing happens
ER: the focus has been moved (back) to the page and paging scrolls the document.

This reproduces with Camino 2.0.x and trunk builds (Smokey noted on IRC, same with Camino 1.6 and 1.5).

notes:
1. WebKit nightly and Safari 5 appear to suffer from the same issue.
2. With official builds (10.4 sdk), the location bar loses the focus ring
   after pressing enter. With a home-made build with the 10.5sdk the
   location bar remains focussed.
   This discrepancy as been noted before in bug 507171 and bug 533359.

Chrome and Firefox/Minefield transfer the focus back to the page.

Comment 1

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

I wonder if the cause here is that we don't trigger a "pageload" event and that our code for focusing the page only gets triggered by a "pageload" event?

Also, the same thing happens if you give a bookmarklet a bookmark shortcut and invoke it via the shortcut (e.g., 'modi' for Mouseover DOM Inspector; the inspector loads, and follows the mouse, but none of the shortcuts, like 'p' for pause, work until you've first clicked back on part of the page).

In another bug, murph points into BWC's loadingDone:activateContent http://mxr.mozilla.org/camino/source/camino/src/browser/BrowserWindowController.mm#1994 which is (part of?) where the focusing happens.

Comment 2

[philippe (part-time)]

Attachment: minimal test case

testcase in case the url changes

1. follow the link in the page to go to an anchor (#issue-99)
2. in the location bar, manually change the value for the anchor to read #issue-55
   press return

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: focus-loaded-content-v0.patch

focus-loaded-content-v0.patch makes two changes:

1. If BrowserWrapper onResourceLoadingCompleted finishes loading its last resource after BrowserWrapper onLoadingCompleted, then call BrowserWindowController loadingDone to update content focus. This case happens when loading a javascript bookmarklet (like modi "Mouseover DOM Inspector") for the current page.

2. If BrowserWrapper onLocationChanged is called without loading a new page, then call BrowserWrapper onLoadingComplete to update content focus and bookmark visit info. This case happens when navigating to an anchor link the current page (by clicking a relative link or editing the URL in the Location Bar).

I am concerned about calling BrowserWrapper onLoadingCompleted or BrowserWindowController loadingDone without matching onLoadingStarted/loadingStarted calls. A safer (but more verbose) solution might be to extract the "update focus after load" common code into a BrowserUIDelegate method independent of loadingDone.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: implicit-loading-active-v1.patch

Minor interface cleanup: BrowserUIDelegate does not need to expose public method setLoadingActive. BrowserWindowController can toggle the loading state in its loadingStarted/loadingCompleted callbacks.

Comment 5

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

Comment on attachment 480861 [details] [diff] [review]
focus-loaded-content-v0.patch

I cannot describe how awesome this is; thanks, Chris!  I ran a few tests and didn't see anything bad, but focus is full of opportunities to accidentally do bad things :(  This needs to go through either the absent murph or smorgan, though; focus is one of Stuart's favorite things :P so he ought to be able to give guidance on your questions.

A couple of questions that did occur to me, though:

1) What happens when a late-loading resource that currently (Gecko 1.9.x) has a child widget loads; will this keep that iframe/whatever from stealing focus, or will it make more cases do so?  (Thinking of bug 576821 comment 6 and similar here.)

2) When a late load is triggered by some page action (any sort of ajax-y things, like those annoying photo lightboxes, or bug 600187 (dunno if those trigger loads), or something like SnapShots), are we going to trigger more bugs like bug 434266, or is this going to be contained to focus?

(As you might have guessed by now, I don't know have the foggiest idea about the whole pageload sequence; I just know the bugs I've seen reported :P )

Comment 6

[Chris Peterson [:cpeterson]]

After reviewing those other bugs, I am more worried about the side effects my patch. Focus is scary!

With my patch, if onLoadingComplete is called with pending onResourceLoadingCompleted callbacks, then the loadingDone callback is delayed until the last onResourceLoadingCompleted. loadingDone may update focus, so there could be problems for the cases when loadingDone updates focus after onResourceLoadingCompleted instead of onLoadingCompleted. Or this could fix problems because focus might be reset to the browser view AFTER the other resources have finished loading?

I checked the test cases in the bugs you listed. My patch has no effect (positive or negative) on those bugs.

Comment 7

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

After looking at bug 507171 with cl and philippe last night, it made me wonder this morning if this is a special case of that bug (only that in this case, a page has already loaded)?

Comment 8

[Stuart Morgan]

Sorry for the slow response here; I'm not trying to ignore this, I just need to have enough time to really get my head into the focus space (especially considering that I already regressed focus recently). Hopefully within the next week, after the location bar patch is done.

Comment 9

[Stuart Morgan]

Comment on attachment 480862 [details] [diff] [review]
implicit-loading-active-v1.patch

This change is simple enough that I'm going to just r/sr it. Sorry I missed that this was a straightforward change; I should have taken care of this patch long ago.

Comment 10

[Stuart Morgan]

Comment on attachment 480861 [details] [diff] [review]
focus-loaded-content-v0.patch

I don't feel good about this approach; the potential for weird focus stealing seems high. E.g., I think Smokey's concern in comment 5 is very likely right.

I also agree with the end of comment 3, in that I don't think we should muddy the waters of what the loadingDone delegates mean. Changing anchors within a page *shouldn't* update bookmark visits, for example.

Fundamentally I think we want to tackle this from the other direction: the reason we want to move focus isn't related to Gecko callbacks at all, but because the user has an expectation of what happens when they press enter in the location bar, so we should change what that does, rather than what the callbacks do. I think that's basically what Smokey was getting at in comment 7 (it's just a simpler case since the ChildView won't be swapped around).

Comment 11

[Stuart Morgan]

I guess bookmarks that don't cause a page load (comment 3 part 1) would fall under the same idea, that it's about the triggering action, so there's a bit more to a comprehensive fix than just bug 507171, but we could tackle that from the UI side as well.

Comment 12

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

Comment on attachment 480862 [details] [diff] [review]
implicit-loading-active-v1.patch

implicit-loading-active-v1.patch landed as http://hg.mozilla.org/camino/rev/130b8c66710c