Closed Bug 178324 Opened 22 years ago Closed 15 years ago

refactor focus handling

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2

People

(Reporter: blizzard, Assigned: enndeakin)

References

(Depends on 2 open bugs, Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(14 files, 11 obsolete files)

514 bytes, application/xhtml+xml
Details
18.28 KB, text/plain
Details
4.08 KB, text/plain
Details
854.22 KB, patch
Details | Diff | Splinter Review
8.80 KB, text/plain
Details
84.14 KB, image/png
Details
6.71 KB, patch
Details | Diff | Splinter Review
5.09 KB, text/plain
Details
1.98 KB, text/plain
Details
9.23 KB, text/plain
Details
4.11 KB, text/plain
Details
3.80 KB, text/plain
Details
1.68 KB, patch
Details | Diff | Splinter Review
811.02 KB, patch
Details | Diff | Splinter Review
It's time to refactor the way that we handle focus since I'm getting sick and
tired of dealing with this house of cards.  Every time we have a focus issue a
little more code gets added to handle an edge case, making everything just that
much more fragile.  I think it's time we really sit down and figure out how to
fix it for real.
Order of things that need to happen:

o Figure out what layout is really using
o Document the focus capabilities of the various platforms
o Figure out what the new ESM code is going to need to look like

We need to keep in mind embedding and accessibility and i18n issues (input methods).
Oh, yeah, plugins need to be documented, too.
I think it might be a good idea to reorganize the existing ESM by adding three
internal classes: one for managing focus, one for managing mouse in/out, and one
for managing drag/drop.
Actually I'm not very familiar with the focus code and I don't know what warts
blizzard is intent on filing off. What are the specific design choices that you
want to change?
Many have talked about refactoring focus, but it has withstood all such talk. It
would be a great achievement -- I think we would need a really great test suite.

See bug 83552 for the massive numbers of focus bugs that have either been fixed
or are still open.

OS: Linux → All
QA Contact: jrgm → sairuh
There are a few big ones:

o remove activation/deactivation:

X doesn't really have this concept and I think we could save a lot of code, both
for unix and for other platforms, by using focus changes to the toplevel window
instead of activation.

If you are worried about having to check to see whether or not the window can be
activated before raising the window, this can be done by always calling down to
the nsIWidget::SetFocus();

o Don't use focus() in XP code to mean both "set focus to this window" and
"raise this window."

This makes me insane.  We clearly don't always mean raise the window when we
change the focus element.  window.focus() called from web scripts should keep
its semantics, for sure, but other than that I think we need a clear distinction
between these two pieces of functionality.

o Make the ESM focus code resistent to recusion

There are lots of instances where the gtk code will call back into itself from a
focus in or focus out event.  Actually, these callbacks are generally the result
of a call chain like:

toplevel focus in -> send GOTFOCUS event -> ESM -> widget->SetFocus() -> tries
to grab widget focus

anyway, I think there's a lot of room to clean things up here, especially in
this area.

o virtual focus handling:

I would love to have virtual key handling for focus to be contained in the ESM
entirely but I don't think that we can get away with that because of at least
input methods on unix and accessibility requirements, but it would be nice.

o add an event type

We might want to seperate the idea of focus from the widget system into two
concepts:

GOTFOCUS ( as in, the widget system just set focus to this element and this is
where all key events are going right now)
SETFOCUS ( if focus was in this toplevel window, key events _would_ be set here)

There's a lot of room here for discussion though to find the most portable and
concise system.

I'm still hoping there's someone who can describe what layout needs so that we
can start to figure out how the platforms all work.
Aaron, can you tell us what requirements accessibility imposes on any changes? 
Or anything that might improve accessibility from a focus standpoint?
QA Contact: sairuh → rakeshmishra
Here's a list of important focus behaviors wrt accessibility:
(Forgive me for any redundancies).

1. The focus must always be clearly visible, and can be in only 1 place at a time
2. The focus cannot be in a different place from where the selection or caret is
active.
3. The focus cannot get lost or stuck
4. The focus must not stop twice on the same item, unless focus has cycled
through the entire document or window
5. The user must be able to tab navigate through all focusable items in a
document or window
6. The focus must be exposed correctly through DOM events, accessibility events
and through an API that can get the current focused window and element
7. The initial focus for a window or document must follow platform and common
sense standards
8. The focus must follow special platform rules, for example sloppy focus on
certain window managers
9. In a document, the focus must be in sync with the selection. This allows
tabbing from the last found text, for example.
10. The focus behavior should not be erratic or problematic during page loads
11. There should be clear and clean wayz to deal with focus programmatically,
via script or C++
12. Accesskeys, typeaheadfind and navigation by frames are important ways of
moving the focus more conveniently than just using the Tab key.

Again, please also check bug 83552 -- the meta bug for focus issues.
Blocks: focusnav
> 6. The focus must be exposed correctly through DOM events, accessibility events
> and through an API that can get the current focused window and element

I think this is the one issue that needs understanding here.

What DOM events and accessibility events are related to focus that you know about?
Do we correctly handle the notion of 'latent' focus at the moment? Focus changes
in a non-active window should be properly handled, with elements taking 'latent'
focus. On window activation, the latent-focussed widget gets real focus.

Also, do we need to distinguish between key and mouse focus? (see bug 175893).

Another important issue is focus handling for embedders; embedders need to get
into the keyboard loop.
While we're at it, I'll see if there's anything I can contribute from all the
ESM hacking we've done (mostly by Pete Collins) for geometric navigation (i.e.
no mouse available at all; only left/right/up/down, including across frame
boundaries).

This factoring would probably have made our job far easier than it was (er, is,
since we're still trying to get it to work right).
OS: All → Linux
I think we can get away with just the GOTFOCUS event if we do latent focus
properly. When a GOTFOCUS event arrives we determine from that which document
tree is affected, and therefore which latently focused element is now 'really
focused', but we shouldn't ever have to actually change which element that is.
If we got focus from a mouse event then we'll process that event separately to
set the focused element.

Maybe it would make more sense for there to be one ESM per document tree instead
of per document. (I used to think it was the former, but it's the currently the
latter --- right?)
OS: Linux → All
>Also, do we need to distinguish between key and mouse focus?

Yes
> Do we correctly handle the notion of 'latent' focus at the moment? Focus changes
> in a non-active window should be properly handled, with elements taking 'latent'
> focus. On window activation, the latent-focussed widget gets real focus.

This is exactly what I was talking about wrt SETFOCUS/GOTFOCUS and activation. 
Right now from the widget standpoint there is no clear distinction.  It's a
strange mix of activation and .focus() calls.  I think we need to come up with a
real answer on this because it's never worked correctly on unix.  I don't know
about other platforms.

> 
> Also, do we need to distinguish between key and mouse focus? (see bug 175893).

I don't think so since "mouse focus" is really giving focus as a result of a
mouse click and shouldn't be any different than giving focus based on tabbing in
between elements.  I might be mis-reading that but I can't be sure.

> 
> Another important issue is focus handling for embedders; embedders need to get
> into the keyboard loop.

Yep.  This is one of the reasons why I want to hack on this so that we can get
this completely right.
OS: All → Linux
o Document the focus capabilities of the various platforms

We need a table of the focus events, and their sequences in all cases (it varies
by case within one OS) on all platforms to have a hope of making this work.
OS: Linux → All
>> Also, do we need to distinguish between key and mouse focus? (see bug 175893).

>I don't think so since "mouse focus" is really giving focus as a result of a
>mouse click and shouldn't be any different than giving focus based on tabbing in
>between elements.  I might be mis-reading that but I can't be sure.

Look at how iFrame focus ring behavior working on IE on Win32. You need the
distinction.
Can you give me a URL where I can test it and what I should be looking for?  I'm
not totally sure how a mouse can have focus and how it can affect focus in such
a way as it needs to be tracked seperately.

Teach me! :)
>I don't think so since "mouse focus" is really giving focus as a result of a
>mouse click and shouldn't be any different than giving focus based on tabbing in
>between elements.  I might be mis-reading that but I can't be sure.

Also on Mac OS X (using full keyboard access mode), clicking a button does not
give it keyboard focus, but I can still tab to it. Buttons getting focus on
click is a real issue for command dispatching, for example.
> 7. The initial focus for a window or document must follow platform and common
sense standards

to clarify (iirc from UI decisions), initial focus for a browser window or
tabbed browser should be in the content area except for:

a. about:blank - then focus should be in the urlbar.
b. if the web author specified another, particular location (eg, a textfield in
an html form).
> Buttons getting focus on
> click is a real issue for command dispatching, for example.

How do you mean?  Can you explain this a little more?
You should seriously consider adding pre, during, and post versions of each
event. This would help clean up some of the nasty code concerning what elements
can accept focus (which plays into buttons accepting focus like in Simon's
example). Although making more multi event dependent state switches may
not be desireable.

To clarify Simon's example, you click on a button that takes the selection and
makes it bold. But clicking on the button gave it focus so you lost selection.
Can't have that.
> You should seriously consider adding pre, during, and post versions of each
> event. 

Are you talking about layout events here or widget events?
Clicking on a button should not give it keyboard focus, period. For example, say
you have some kind of <htmlarea> (implemented as an <iframe>), above which the
page author has put a row of buttons as a toolbar. If clicking on the Bold
button steals keyboard focus from the <iframe>, then the user has to click back
in the iframe to be able to type again. Or the page author has to mess with lots
of focus() calls.
>Can you give me a URL where I can test it and what I should be looking for?  I'm
>not totally sure how a mouse can have focus and how it can affect focus in such
>a way as it needs to be tracked seperately.

IE draws or doesn't draw focus rectangles around iframes depending on if you tab
into them or click on them. So it is making a behavior distinction on how
something got focused, mouse vs keyboard. In almost all cases the two can
effectively be the same though. 

>> You should seriously consider adding pre, during, and post versions of each
>> event. 
>Are you talking about layout events here or widget events?

Probably needed at the widget, NS_ level to work.
I always thought the iframe thing was a bug (in both IE and Mozilla)....  The
distinction for buttons is there, true, and the same argument could apply
equally well to links...
Simon, that's not true.  On Win32, buttons do receive keyboard focus when 
clicked.  Toolbar buttons do not, but dialog buttons (and form controls in a 
page) do.

Anyway, whether or not controls receive focus should be determined by CSS 
properties (separated into one for mouse activity and one for key activity), 
e.g., like -moz-user-focus but split into two properties).  That combined with 
platform #ifdefs using the preprocessor (or by forking files) should give us 
all the flexibility we need regarding the accepting of focus.  

(We can add new values to the properties if we encounter any unusual cases.)

Note there is also a CSS3 property for explaining what happens with regards to 
selection when a control becomes focused.  If necessary, we should implement 
it.  Off the top of my head I don't remember all the values.
I sure don't mind if there's a css property or variable describing how something
got focused (keyboard, mouse, script).

Just to be clear, the important thing is that there is always only 1 focus, and
that the focus appearance is not misleading. There can't be a mouse focus
separate from the keyboard focus, of course.
> Clicking on a button should not give it keyboard focus, period

OK, so this behavior seems to differ between platforms. Mac OS X does not give
focus on click, Windows does.
>Maybe it would make more sense for there to be one ESM per document tree 
>instead of per document. (I used to think it was the former, but it's the 
>currently the latter --- right?)

That is why we have the focus controller hanging off the window instead of
putting all focus control in the ESM. Focus is a per top level window singleton,
the ESM is not. Changing the ESM to one per document tree/top level window is
another huge chunk of work and probably something you want to keep out of this
round of changes IMO. 
Well, during this round maybe we can move as much focus goop as possible into
the focus controller. Then later we can move mouse event handling into a
corresponding mouse controller. There is some really horrible stuff in ESM mouse
handling to work around the fact that there's an ESM-per-document.
>o remove activation/deactivation:

I take issue with this, we really do need to distinguish between intra and inter
top level window focus events. The resulting behavior is different (think about
widget activation states). Also, embeddors need to distinguish between focus and
activation state. If I activate a window with an embedded gecko, I want gecko to
look active (that look is different per platform), but it won't necessarily have
focus. 
>Well, during this round maybe we can move as much focus goop as possible into
the focus controller.

That sounds reasonable
> Well, during this round maybe we can move as much focus goop as possible into
> the focus controller.

Perhaps a staged approach with tests after reasonable stopping points will help
us keep the code clean and bug-free. There are going to be a lot of regressions
with a focus rewrite, because it's a difficult problem. I think the best way to
keep things under control is to try to check it as it progresses. :-)

Stage 1. Move glop out of esm into nsFocusController
Stage 2. ?
> I take issue with this, we really do need to distinguish between intra and inter
> top level window focus events. The resulting behavior is different (think about
> widget activation states). Also, embeddors need to distinguish between focus and
> activation state. If I activate a window with an embedded gecko, I want gecko to
> look active (that look is different per platform), but it won't necessarily have
> focus. 

Can you give me a few concrete examples here?  The only one that comes to mind
for me is selection.  (Selection, btw, is done completely wrong for unix since
window activation should not affect the selected state.)
Depends on: 83496
Attached file a test for blur being broken (obsolete) —
3 years later....

https://bugzilla.mozilla.org/attachment.cgi?id=225235

mouseover and click on first link. return to original page

now mouseover second link. 

Note: blur event has not been fired.
Please don't attach a testcase for your pet bug to an architecture bug; it makes it impossible to use this bug for whatever it was originally for.  (I really don't care if it's important, I'm not going to look at it here; it's like standing up in the middle of a wedding and screaming that everybody must do something right now to stop global warming.)
re:#36 & #37

Thanks dbaron
new bug is: https://bugzilla.mozilla.org/show_bug.cgi?id=341231

sorry to spoil the party, It's hard to know where focus bugs are and whose working on them...

Might a statement here help elucidate how it is  intended to map out the problem?

for instance if this is an architecture bug, why is it only blocking one bug?
perhaps #341231 might be added?

or:
https://bugzilla.mozilla.org/show_bug.cgi?id=264244 
Boris Zbarsky wrote "We have existing bugs on this." with no link....

~
Assignee: blizzard → mats.palmgren
The keyboard focusing is actually a very sinister security issue.  

I can't count the number of times over the years that I have selected a username/password editable field in Firefox while art and script garbage was still loading in the background, and had keyboard focus ripped out from under me while I was typing, sending my password into some other editable field.  More often, I type the whole email/password into ANY site, while looking elsewhere, look at the screen again, and all the typing was eaten by some button, and some different button has been activated by 'Enter' because focus went elsewhere.  

Let alone all the times filling out forms or typing into that 'Search box' in the corner, and had a loading web page grab my focus while I was typing THERE.

As a rule, if I have CLICKED or otherwise manually navigated to an editable field and ABOVE ALL, if I have started typing into it, the browser should never, ever, EVER steal focus away.  Period.  This has been broken from day one.  

If you want to be 'standard compliant' to an infuriating, pedantic "standard", then stick a checkbox in the settings to turn this behavior on selectively, so most people won't have to suffer.

This patch:
 - moves focus handling into a single class and out of the EventStateManager, FocusController and other places
 - stores the active window and focused frame in this class
 - stores the focused element in the (inner) nsGlobalWindow, or the last element that was focused
 - removes a lot of hacks
 - includes tests

Not tested extensively yet are embedding and accessibility. Some work with ime still to do and some existing tests fail with this patch.
Assignee: mats.palmgren → enndeakin
Attachment #225235 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Blocks: 303428
I gave a try to the build from https://build.mozilla.org/tryserver-builds/2008-12-30_08:15-neil@mozilla.com-focus/ on Vista.

I found one issue with Google Docs: The caret is not visible.

This is reproducible with a designMode enabled document: data:text/html,<script>document.designMode="on"</script>
This is fantastic cleanup Neil. Good work, I hope the patch makes it all the way in!
Comment on attachment 354846 [details] [diff] [review]
refactor focus patch work in progress

I tried to skim the patch but I have a couple of queries:
1. What replaces the code that restores the focus to where it was before print preview?
2. Can you be sure that no C++ callers try to directly focus a XUL textbox?
(In reply to comment #43)
> 1. What replaces the code that restores the focus to where it was before print
> preview?

I'm not sure why I removed that. I've added it back in. I notice though a bug where changing the scale causes a second print preview to occur which resets this. Is there a bug filed for this?

> 2. Can you be sure that no C++ callers try to directly focus a XUL textbox?

Hmmm. I think I had assumed that it would get overridden, but thinking about it, I suppose it would not.
I'm glad to report that this fixes the issue in gmail if you alt-enter to open a url, the focus is no longer pushed to the first element inside the window (which breaks gmail's key press event listeners), and instead leaves it in the correct frame.

I'm assuming we'll be able to easily add something to remove focus from the location bar on pressing.. say escape, and have the browser regain focus? I see in the tryserver build that pressing esc from the findbar correctly has gmail pick up events instead of trunk's behavior of triggering quickfind.
(In reply to comment #45)
> I'm assuming we'll be able to easily add something to remove focus from the
> location bar on pressing.. say escape, and have the browser regain focus?

Yeah, that would be very useful. Actually i have added that in my own extension, but i really think that Escape key behavior should be default in firefox.
(In reply to comment #45)
> I'm assuming we'll be able to easily add something to remove focus from the
> location bar on pressing.. say escape, and have the browser regain focus?

Er, I certainly hope this would NOT be the default, at least not on the Mac, as it's not at all what any other Mac browser does (or has ever done). Escape, when pressed while focus is in a text field, cancels any active edit in that text field and restores the selection to whatever was selected before the edit began.

The location bar and the Find bar are two extremely different pieces of UI, however, and it's not entirely clear which one comment 45 is referring to, since it refers to both in successive sentences.
File a new bug on that, please, if you want or don't want it; it's off-topic for this bug.
QA Contact: rakeshmishra → xptoolkit.widgets
Attached patch newer patch (obsolete) — Splinter Review
Fixes:
 - the caret now appears properly for designmode documents
 - the focus is properly restored when exiting print preview mode
 - focus now works properly when using IME
 - changed the tab switching behaviour to focus the new tab when either
   something in the new tab is focused, the old tab is focused or nothing
   is focused
 - supported the mobile offscreen browser feature
 - fixed up some area element checks so that they check that it is an html element
 - fixed the test for bug 398289 to start after a timeout , as it was failing
   because <dialog> focuses content inside it on load, causing the content to
   scroll.
 - all tests now pass

Only one issue remains that I found today: accesskeys for XUL textboxes do not work

Plugins have been tested and work. Accessibility and embedding has not been tested.
Attachment #354846 - Attachment is obsolete: true
I tried these latest try server build. I crash in this testcase, while pressing the tab key.
I don't see any crash for that testcase. How are you running it? Is the security dialog repeatedly popping up? Is there a stacktrace?
Yeah, well, I added the privileged code, so it crashes automatically.
I'm afraid I'm not able to get a stacktrace, because it's a tryserver build.
I'm on Windows, btw.
OK thanks Martijn, I could reproduce on Windows. The child frame in the testcase has no presshell and a null-check was missing when tab-navigating into child frames.
Attached patch focus patch (obsolete) — Splinter Review
Fixes in this version:
 - fixed crash when tabbing into a frame with no document
 - now handles accesskeys for textboxes, editable menulists and scales properly
   and added a test for this
 - reworked is focusable checks so that special focus/blur methods aren't needed
   for those elements (point 2 of comment 43)
 - fixed a leak when retrieving the focused element in child frames

smaug implied that he'd like to review this.
Attachment #359125 - Attachment is obsolete: true
Attachment #362321 - Flags: review?(Olli.Pettay)
Comment on attachment 362321 [details] [diff] [review]
focus patch

r+ for Cocoa widget changes
Attachment #362321 - Flags: review+
Josh, have you tested the Cocoa widgets changes?

There are a *lot* of bugs they may reopen.
Masayuki, you should know about the changes this patch (attachment 362231 [details]) makes.  They may reopen some of the bugs you've fixed.
I noticed a merge error in the patch in nsChildView.mm. The IME realted changes made by bug 466408 weren't meant to be removed. I'll add those back in subsequent versions.
Neil, did *you* do any testing of the Cocoa widgets changes?
I've tested it extensively. For instance, I tested that Cocoa bugs 354768, 314160, 355138, 358379, 377105, 381448, 403232, 408266, 419338, 432817, 432131, 431429, 428047, 470286, 445531, 413882, 396952, 395465, 404433, 430419, 396832, 402926, 443024, 156583 don't regress with this patch.

The only issue I've noticed since the patch was an issue with ime in certain fields which I will investigate.
(In reply to comment #61)

OK.  And thanks for letting me know.

I will (of course) also dig through old bugs, to find anything you might have missed.  I'll report back here.
I tested a build for over an hour, though I did not test IME.
Neil, a few questions/nits from the changes in the accessibility module:
> already_AddRefed<nsIDOMNode> nsAccessNode::GetCurrentFocus()
> [...]
>+  nsIDOMNode *focusedNode = nsnull;
>+  if (focusedElement) {
>+    CallQueryInterface(focusedElement, focusedNode);

This new code makes use of CallQueryInterface. From a recent pitfall with following constructive discussion, we learned from bz and peterv that it's preferred to limit CallQI to outparams, and use smart pointers in all other cases. Would it be possible to do the above using smart pointers and not using CallQI?

From nsDocAccessible:

>+    nsCOMPtr<nsIDOMDocument> domdocument;

Nit: Our style we use almost consistently would have called this variable domDocument.
(In reply to comment #64)
> Neil, a few questions/nits from the changes in the accessibility module:
> > already_AddRefed<nsIDOMNode> nsAccessNode::GetCurrentFocus()
> > [...]
> >+  nsIDOMNode *focusedNode = nsnull;
> >+  if (focusedElement) {
> >+    CallQueryInterface(focusedElement, focusedNode);
> 
> This new code makes use of CallQueryInterface. From a recent pitfall with
> following constructive discussion, we learned from bz and peterv that it's
> preferred to limit CallQI to outparams, and use smart pointers in all other
> cases. Would it be possible to do the above using smart pointers and not using
> CallQI?
already_AddRefed<nsIDOMNode> nsAccessNode::GetCurrentFocus() is a perfect use case for CallQueryInterface, because it returns already_AddRefed<...>
Though, one could use nsCOMPtr<nsIDOMNode> focusedNode; 
and return focusedNode.forget();
Enn, you're removing nsIFocusEventSuppressorService.
Have you ensured that the change doesn't regress bug 421209 or bug 395609.
See bug 395609 comment #75.
Adding some GTK+ widget people.  The interactions of focus on gtk and embedding can often be very exciting.
(In reply to comment #67)
> Enn, you're removing nsIFocusEventSuppressorService.
> Have you ensured that the change doesn't regress bug 421209 or bug 395609.
> See bug 395609 comment #75.

The testcases in those bugs don't crash. But then with this patch, the gotfocus/lostfocus events aren't used and focus events can only be fired on window activate/deactivate. In addition, the focus system is called when a document is hidden/unloaded (nsFocusManager::FrameHidden) to clear out any focus state. So I don't think those bugs would occur.

That said, I'll add a minor level of extra protection in the next version of the patch to block focus events for docshells that are being destroyed.
Comment on attachment 362321 [details] [diff] [review]
focus patch

I don't think you can simply remove the method swizzling for nsCocoaWindow_NSWindow_sendEvent:. The NS_OBJC_BEGIN_TRY_LOGONLY_BLOCK guard there was specifically added to fix bug 442245, removing it will regress that bug.
(In reply to comment #70)
> (From update of attachment 362321 [details] [diff] [review])
> I don't think you can simply remove the method swizzling for
> nsCocoaWindow_NSWindow_sendEvent:. The NS_OBJC_BEGIN_TRY_LOGONLY_BLOCK guard
> there was specifically added to fix bug 442245, removing it will regress that
> bug.

Are you saying that the default implementation of sendEvent is deficient that it has to be overridden? I would have assumed that the default exception handling would have been correct.
(In reply to comment #71)

I think I know the answer to this one:

It's not that the default implementation of [NSWindow sendEvent:] is
deficient.  It's that the way we've fixed bug 163260 makes the browser
crash on non-fatal Objective-C exceptions.  Since the particular
non-fatal exception that triggers the crash at bug 442245 happens when
a keyboard event is handled (by system code, while it's being
processed by [NSWindow sendEvent:]), [NSWindow sendEvent:] is a
convenient place to install a handler (via method swizzling) to stop
these exceptions from being fatal.

But it occurs to me that, as long as we're only concerned with
non-embedders, we might not have to use method swizzling at all.  In
non-embedders all top-level windows are ToolbarWindow objects.  Maybe
all we'd have to do is override ToolbarWindow's sendEvent: method, and
bracket the call to super with a NS_OBJC_BEGIN_TRY_LOGONLY_BLOCK
try/catch block.
(In reply to comment #73)

Yes, it could.
Comment on attachment 362321 [details] [diff] [review]
focus patch

(Following up comment #62)

I've now tested your patch (with the fix for bug 466408 restored) on
the list of (Minefield-specific) focus-related problems in bug 314160
comment #19.  It passed with flying colors -- I didn't have a single
problem.

I'm truly impressed.

Of course, since your patch is cross-platform and very complex, it
will need to pass many more tests and get many more reviews before it
can be landed.  But as best I can tell it has no problems on OS X.  So
I'm seconding Josh's r+ for the Cocoa widgets changes (with the fix
for bug 466408 restored, of course).
Here's what I tested with -- Neil's feb13 patch with the fix for bug
466408 restored and updated to current trunk code.

And here are some tryserver builds:

https://build.mozilla.org/tryserver-builds/2009-02-20_12:30-smichaud@pobox.com-bugzilla178324-feb13-enndeakin/

They should help with testing.
Attachment #362321 - Flags: review?(emaijala)
Attachment #362321 - Flags: review?(Olli.Pettay)
Attachment #362321 - Flags: review-
Comment on attachment 362321 [details] [diff] [review]
focus patch

Ere, perhaps you could look at at least the Windows part?
> diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
> ...
> -                 onblur="document.getElementById('identity-box').style.MozUserFocus = 'ignore';">
> +                 onblur="setTimeout(function() document.getElementById('identity-box').style.MozUserFocus = '', 0);">
> Why this change?

This line (and the onfocus line above it) are meant so that shift+tabbing to the identity box may be used, by making the identity box only focusable when the url field is focused, but not focusable otherwise. However, with this patch, firing the blur event would cause the -moz-user-focus to change to 'ignore' making it non-focsuable again. It worked before because the focus code wasn't very good at making sure that focusable elements were actually focusable.


> diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> -            this._fastFind.setDocShell(this.mCurrentBrowser.docShell);
> +            try {
> +              this._fastFind.setDocShell(this.mCurrentBrowser.docShell);
> +            } catch (ex) { }
> Why this change?

I think this was remnant from when I hadn't finished the find functionality. I've removed this change.


> diff --git a/content/base/public/nsIContent.h b/content/base/public/nsIContent.h
> --- a/content/base/public/nsIContent.h
> +++ b/content/base/public/nsIContent.h
> You should update the IID

How odd. I must have merged wrong as this was in other versions.


> You've ensured these changes don't cause any new security problems?
> Like, this doesn't expose any non-same-origin element to the non-privileged caller?

It should only ever be returning an element in the same document. But I'll add
a security check and some assertions just to be safe.


> @@ -2921,53 +2421,71 @@ nsEventStateManager::PostHandleEvent(nsP
>                viewMan->GrabMouseEvents(nsnull, result);
>              }
>            }
>          }
>          break;
>        }
>  
>        if (nsEventStatus_eConsumeNoDefault != *aStatus) {
> -        nsCOMPtr<nsIContent> newFocus;
> -        PRBool suppressBlur = PR_FALSE;
> -        if (mCurrentTarget) {
> -          mCurrentTarget->GetContentForEvent(mPresContext, aEvent, getter_AddRefs(newFocus));
> Can you really remove this? The change doesn't break image maps or anything?

I reverted much of this change.

> @@ -4893,68 +3790,18 @@ nsEventStateManager::SetContentState(nsI
> For content state changes you may want to ask a review from dbaron.

OK

> diff --git a/content/events/test/test_bug450876.html b/content/events/test/test_bug450876.html
> +  is(document.hasFocus(), true, "document should be focused");
>    wu.sendKeyEvent('keypress',  9, 0, 0);
> -  is(document.activeElement, document.body, "body element should be focused");
> +  is(document.activeElement, document.getElementById('a'), "body element should be focused");
> +  is(document.hasFocus(), false, "document should not be focused");
> Why these changes?

The focused element in a frame doesn't clear on the old document when shift+tabbing to a new frame.

 
> Why is this code needed now here?
> What if we're in print preview, so that the primary shell does have
> link handler but print preview shell doesn't (if that is a possible case)?

I still need to investigate this


> diff --git a/content/xbl/src/nsXBLPrototypeHandler.cpp b/content/xbl/src/nsXBLPrototypeHandler.cpp
> --- a/content/xbl/src/nsXBLPrototypeHandler.cpp
> +++ b/content/xbl/src/nsXBLPrototypeHandler.cpp
> @@ -451,17 +452,19 @@ nsXBLPrototypeHandler::DispatchXBLComman
>    mEventName->ToString(type);
>  
>    if (type.EqualsLiteral("keypress") &&
>        mDetail == nsIDOMKeyEvent::DOM_VK_SPACE &&
>        mMisc == 1) {
>      // get the focused element so that we can pageDown only at
>      // certain times.
>      nsCOMPtr<nsIDOMElement> focusedElement;
> -    focusController->GetFocusedElement(getter_AddRefs(focusedElement));
> +    nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> +    if (fm)
> +      fm->GetFocusedElement(getter_AddRefs(focusedElement));
> Does this change the behavior, if focused element isn't in this document?

I changed this to use the target's window. Really this whole special case for the space key doesn't belong here though.


> -<body style="height: 100%" onload="onBodyLoad();">
> +<body style="height: 100%" onload="setTimeout(onBodyLoad, 0);">
> Why this change is needed?

Because the focus is updated by the dialog used in the child. (I'm unclear why a dialog is used in the testcase though)
and it adjusts the persistence check. I don't recall the exact reason, but I remember it being correct.


> Could we move Get/SetPopupNode and GetController*() to some other interface
> and kill the whole nsIFocusController?

I plan to do this in a followup bug.


> diff --git a/dom/public/idl/base/nsIDOMWindowUtils.idl b/dom/public/idl/base/nsIDOMWindowUtils.idl
> ...
> +   * Do not use this method. Just use element.focus instead.
> +   *
> Hmm, would it be easy to convert all the callers to use element.focus and
> just remove the method.

Sure, in a follow up bug though.


> diff --git a/dom/public/idl/base/nsIFocusManager.idl b/dom/public/idl/base/nsIFocusManager.idl
> ...
> + * If the frame's reference is to a frame element (iframe, browser,
> + * editor), then the child frame contains the element that is currently
> + * focused. If the frame's reference is to a root element, then the root is
> + * focused. If a frame's reference is null, then no element is focused, yet
> + * the frame is still focused.
> frame's reference? I don't like using "frame" in this context.
> Perhaps DOMWindow?

I updated to use window everywhere. I wasn't sure which term to use. I don't however like having both an 'activeWindow' and a 'focusedWindow' property.


> +  attribute nsIDOMWindow activeWindow;
> I guess we can't unactivate all windows?

The property will be set to null if there is no active window. The focus manager doesn't
provide a means of lowering windows, but in the future I hope to combine this with the
window mediator/window watcher.


> Does activeWindow point to the topmost window (window.top in chrome UI)?
> Based on focusedFrame documentation, yes.

The topmost window yes.


> Should this be readonly attribute, so that setting focus goes always
> via setFocus(...)? Or how is setting focusedElement related to
> setFocus?

It's actually just a shorthand for SetFocus(element, FLAG_RAISE), but I agree that it could be removed. In fact, it's probably better that way and require that the flags be set explicitly.


> +  void clearFocus(in nsIDOMWindow aFrame);
> And perhaps even this could be part of the same setFocus() ?

Not really. None of the arguments are in common and its functionality is quite different.

 
> +  /**
> +   * ...The return value will be null if element is focused.
> This is strange for a method which is called "getFocusedElementForFrame"

That should say 'if *no* element is focused'.


> Can nsIFocusManager handle case like Bug 419059?

I'd consider that bug to be invalid though as accesskeys shouldn't be working for hidden elements. The bug commenters are confusing accesskeys (which don't work when a menu is closed) with key shortcuts (which do).

Anyway, the testcase worked in previous versions, and the testcase still works with this patch in the sense that the link will still get activated, but it will not get focused. Hidden elements cannot be focused anyway so the focus behaviour shouldn't change.

  
> diff --git a/toolkit/content/widgets/menulist.xml b/toolkit/content/widgets/menulist.xml
> -          if (!this.hasAttribute('focused')) {
> -            if (event.originalTarget != this.inputField)
> -              this.inputField.focus();
> -            this.setAttribute('focused','true');
> -          }
> +          this.setAttribute('focused','true');
> I don't understand this change.

The code here was retargeting the focus to the inner anonymous input field. The focus manager does this directly now.


> diff --git a/widget/src/gtk2/nsWindow.cpp b/widget/src/gtk2/nsWindow.cpp
> ...
> +#ifdef USE_XIM
>      if (gFocusWindow) {
> -        nsRefPtr<nsWindow> kungFuDeathGrip = gFocusWindow;
> -#ifdef USE_XIM
> You've made sure you can remove kungFuDeathGrip?

I'll add it back in just in case.


>      if (!gFocusWindow) {
> -        containerWindow->mActivatePending = PR_FALSE;
> +        // XXXndeakin P2 hmmm. is this right?
>          DispatchActivateEvent();
>      }
> When could this happen?

When could what happen? Activating a window upon a button press? I think my comment here is just something I wanted to test more,
but I can remove it now.


> Have you checked that focus handling changes don't allow popups in cases
> where popups are currently disabled?

Yes, and it works the same as far as I can tell.


> +#ifdef MOZ_XUL
> +#include "nsIDOMXULTextboxElement.h"
> +#include "nsIDOMXULMenuListElement.h"
> +#endif
> You have tested that this all compiles with and without XUL?

I will check this somehow.


> Nit, I'd prefer
> if (...) {
>   ...
> } else {
>   ...
> }

Well I don't actually ;)


> +    // don't fire events on the root content
> So why not? What if the root is <input> or similar?

I'll have to think about this one.


+nsresult
+nsFocusManager::GetNextTabbableContent(nsIPresShell* aPresShell,
Have you tested that tabbing works with print preview too?

Yes.
>> +    // don't fire events on the root content
>> So why not? What if the root is <input> or similar?
>
>I'll have to think about this one.

Actually, creating an xhtml document with only an <input> element doesn't work anyway. It creates a squished field which cannot be typed into.
> Why is this code needed now here?
> What if we're in print preview, so that the primary shell does have
> link handler but print preview shell doesn't (if that is a possible case)?

Do you think it would be worth passing an extra prescontext or presshell argument to all of the IsFocusable methods just for this one case?
> +#ifdef MOZ_XUL
> +#include "nsIDOMXULTextboxElement.h"
> +#include "nsIDOMXULMenuListElement.h"
> +#endif
> You have tested that this all compiles with and without XUL?

It seems to compile ok, when I hack up some fixes due to trunk not actually building without XUL currently.
Comment on attachment 362321 [details] [diff] [review]
focus patch

I'm thoroughly impressed with Enn's work on the patch. It does fix e.g. bug 261074 which I couldn't fix in Windows widget code alone. So far I couldn't find any regressions.
Attachment #362321 - Flags: review?(emaijala) → review+
Blocks: 261074
Attached patch focus patch v2 (obsolete) — Splinter Review
Hopefully I fixed all the issues.

Builds of this patch are at https://build.mozilla.org/tryserver-builds/2009-02-24_18:09-neil@mozilla.com-refocus3b/

Smaug and I discussed and we will look at the print preview issue in a followup bug. Focus in print preview seems to work as is, but further investigation is needed.
Attachment #364039 - Flags: review?(Olli.Pettay)
Attachment #364039 - Flags: review?(dbaron)
Comment on attachment 364039 [details] [diff] [review]
focus patch v2

request from smaug to have dbaron review the content state changes
test_focus.xul doesn't run on linux if mouse is not over that window.
When mouse is moved over to that window, tests start to run again.
I use the "focus follows mouse" setting in the window manager.

When the mouse is at the place where test_focus.xul window opens, tests usually
pass, but I did see some failures once. So, the test could be more reliable.
(Other unreliable test is the largemenu, but that is about some other bug)

There are also some leaks when running chrome tests. For example nsFocusManager is leaked. Not sure if the patch causes those leaks.
But in any case, nsFocusManager should participate to cycle collection.

Also running browser-chrome leaks. All tests pass.

Running mochitest leaks and there are 14 failures with
html/content/test/test_bug430351.html
A number of current mochitests already fail if the mochitest window doesn't have focus, so this is not exactly making things worse.  :(
test_focus.xul failure is something new and else than "normal" focus problem.
Mouse is over the main window, because it is used to start the test.
Then a new window is created to run test_focus.xul, but those fails because
apparently that doesn't get the focus the way it expects.
For the chrome tests, I see an nsRunnable and an nsBaseAppShell leaking but nothing else. Is this the leak you mean? Cycle collection would be good to have and I'll add it.

> When the mouse is at the place where test_focus.xul window opens,

Which window do you mean? test_focus.xul itself only loads in the main test iframe. I assume that opening a new window in focus follows mouse mode doesn't get focus immediately unless the mouse happens to be over it.

This test opens a number of child windows in order to test focus switching between windows. I would assume that no other test does this, but I wouldn't imagine the need for window focus for this test is actually something that wouldn't have failed before this patch as well.
(In reply to comment #90)
> I assume that opening a new window in focus follows mouse mode doesn't
> get focus immediately unless the mouse happens to be over it.

Often (perhaps usually) the new window does get focus with "focus follows mouse".  But it depends on the window manager, what hints are provided to the window manager (about the purpose of the windows), and timing of events.  (If the user continues to type in the first window after initiating the other window but before the other window opens, then the window manager will likely prevent the new window from stealing focus.)
Comment on attachment 364039 [details] [diff] [review]
focus patch v2

I redid the Minefield-specific tests from bug 314160 comment #19, and
once again I had no problems.

But this time I also ran all the automated tests listed at
https://developer.mozilla.org/en/Mozilla_automated_testing, and saw a
couple of anomalies:

The 439206-1.html crashtest failed (crashed) consistently (twice in a
row) with this patch, but not at all without it.

The test_bug430351.html Mochitest had a bunch of failures with this
patch, and none without it.

I tested the patch by applying it to "old" code and then updating it
to current code.  The non-patched build I tested (as a control) was
the "same" code without the patch.
> The 439206-1.html crashtest failed (crashed) consistently (twice in
> a row) with this patch, but not at all without it.

Oddly, the 439206-1.html test doesn't crash when I load it
individually -- only when I use -reftest to load it as part of a list
of crashtests.
Comment on attachment 364039 [details] [diff] [review]
focus patch v2

>> The 439206-1.html crashtest failed (crashed) consistently (twice in
>> a row) with this patch, but not at all without it.
>
> Oddly, the 439206-1.html test doesn't crash when I load it
> individually -- only when I use -reftest to load it as part of a
> list of crashtests.

Dumb mistake.

It's the *next* crashtest, 473284.xul, that crashes (though only with
this patch).  It crashes loaded individually, and loaded with -reftest
in a list.
Attachment #364039 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 364039 [details] [diff] [review]
focus patch v2

So I'd like to see the memory leak fixes.
(In reply to comment #85)
> Builds of this patch are at
> https://build.mozilla.org/tryserver-builds/2009-02-24_18:09-neil@mozilla.com-refocus3b/

I'm on Windows XP SP2 (Japanese).

Steps to reproduce:
1. Add Japanese keyboard and Microsoft Natural Input 2002 ver.8.1 (or Microsoft Natural Input 2003)
2. Change intl.enable_tsf_support to true and restart Firefox.
3. Focus location bar, search bar, <input>, <textarea> or somewhere which can be inputted.
4. Bring another window to front.
5. Bring the window of 3. to front.

Actual result:
Microsoft Natural Input can not be selected.

Expected result:
Microsoft Natural Input can be selected.
I don't see any issue with IME as described in comment 97. Enabling the preference and using Japanese Natural Input popups up the IME panel whenever a text entry field is focused and disappears when it is blurred. I'm not using a Japanese OS but I doubt that's the issue.
The bar in the screenshot is "Language Bar", see http://support.microsoft.com/kb/306993

It's displayed by default on Japanese Windows, but I don't know whether it's same on Western Windows.
I have been using the focus patch from attachment 362321 [details] [diff] [review] for a while in a Minefield build on OS/2 (unfortunately, I didn't manage to rebuild with the newer patch yet). I only found one problem, that I thought I should share: I cannot always reproduce it, but there is a high probability that the following causes the problem on my setup:
- go to http://www2.computeruniverse.net/comment.asp?id=90194875&language=deutsch
- open another Minefield window with some other webpage
- scroll down in the first window to the entry fields (below
  "Begründungen")
- click on a field (focus should be in the field)
- click in the other window (focus should move there)
- click back onto the entry field in the first window
--> fairly often the focus does not end up in that field but somehow
    stays in the other window; while the field reacts on mouse-down
    (color changes) it changes color back to white on mouse-up and the
    caret doesn't appear
Peter, can you create a build which has the two debugging defines near the top of nsFocusManager.cpp enabled, and post or send me the log of what happens?

It's possible that os2 needs to change DispatchFocus to DispatchFocusToTopLevel as is done on Windows.
(In reply to comment #100)
> The bar in the screenshot is "Language Bar", see
> http://support.microsoft.com/kb/306993
> 
> It's displayed by default on Japanese Windows, but I don't know whether it's
> same on Western Windows.

I get the ime language bar when a textbox is focused and it gets disabled whenever a non-textbox is focused. But the "Natural Input" choice (with the blue icon) is always disabled on the menu, even in other applications. The other "IME" choice with the red icon works fine. I wonder if there's something else I need to enable/install in order to get this extra menu enabled.
(In reply to comment #103)
> "Natural Input" choice (with the
> blue icon) is always disabled on the menu, even in other applications. The
> other "IME" choice with the red icon works fine. I wonder if there's something
> else I need to enable/install in order to get this extra menu enabled.

Nakano-san: Could you help with this?
(In reply to comment #80)
> > For content state changes you may want to ask a review from dbaron.

So what exactly is it that you'd like me to review?

Based on going through the patch, it looks like you've removed a large number of esm->SetContentState calls from various parts of the code (perhaps all of the existing ones) and replaced them with two calls in nsFocusManager.  You've also removed the "interesting" part of the (aState & NS_EVENT_STATE_FOCUS) code in nsEventStateManager::SetContentState.  This all seems pretty reasonable to me.  Is that the bit that you wanted me to review, or anything else?  (I'm probably not in a position to judge if the old and new sets of calls to SetContentState are equivalent, but the new set sure seems more likely to be accurate.)

The one thing I wonder about is how the :focus pseudo-class ought to interact with the somewhat tree-like nature of focus.  That said, I don't know if there are ever two nodes in the tree in the same document... rather than an iframe being focused in an outer document and something inside that iframe also being focused.  If that's not the case, then it's probably all simple... although I wonder if that really ought to be the case (e.g., should scrollbars from 'overflow:scroll' really behave differently from scrollbars from an iframe?).

Sorry for the delay in getting to this.
(In reply to comment #103)
> But the "Natural Input" choice (with the
> blue icon) is always disabled on the menu,

I found a similar (maybe same) issue without Natural Input.

Steps to reproduce:
1. Add Drawing Pad in Regional and Language Options -> Languages -> Details.
2. Change intl.enable_tsf_support to true and restart Firefox.
3. Focus location bar, search bar, <input>, <textarea> or somewhere which can
be inputted. Drawing Pad should be in the Language Bar, even though it's disabled.
4. Bring another window to front.
5. Bring the window of 3. to front.

Actual result:
Drawing Pad is not in the Language Bar.

Expected result:
Drawing Pad should be still in the Language Bar.
I found another issue.

Steps to reproduce:
1. Add Drawing Pad in Regional and Language Options -> Languages -> Details.
2. Change intl.enable_tsf_support to true and restart Firefox.
3. Click a <input> or <textarea> in a content.
4. Click the Location Bar.
5. Repeat 3, 4, 3, 4, ...

Actual result:
Drawing Pad appears in the Language Bar at only either the content or the Location Bar.

Expected result:
Drawing Pad should appear in the Language Bar at both the content and the Location Bar.
(In reply to comment #104)
> (In reply to comment #103)
> > "Natural Input" choice (with the
> > blue icon) is always disabled on the menu, even in other applications. The
> > other "IME" choice with the red icon works fine. I wonder if there's something
> > else I need to enable/install in order to get this extra menu enabled.
> 
> Nakano-san: Could you help with this?

Maybe, WinXP of en-US is not installed the "Natural Input". Because it is not displayed in the list of "Text Services and Input Languages".
Sakai-san, can you reproduce the bug on current trunk build? I changed the behavior at focus changing in bug 477911. It might help the bug.
(In reply to comment #109)
> Sakai-san, can you reproduce the bug on current trunk build? I changed the
> behavior at focus changing in bug 477911. It might help the bug.

Trunk doesn't have this problem, both current and also before bug 477911.
I want to test current trunk with focus refactoring, but the focus patch v2 can't be merged because of conflict.
Ah, of course, I meant "current trunk with focus refactoring patch"...
(In reply to comment #102)
> Peter, can you create a build which has the two debugging defines near the top
> of nsFocusManager.cpp enabled, and post or send me the log of what happens?
> 
> It's possible that os2 needs to change DispatchFocus to DispatchFocusToTopLevel
> as is done on Windows.

Thanks for the hints, Neil. Hopefully will manage to give those a try on the weekend.
(In reply to comment #109)
> I changed the
> behavior at focus changing in bug 477911. It might help the bug.

I've tested 2009-02-24's trunk + focus patch v2 + bug 477911.
https://build.mozilla.org/tryserver-builds/2009-03-10_01:13-dev-null@hotmail.co.jp-try-9564b123105/
But it still has problem.
I found that IME's transaction is not committed when the window is being deactivated.
I think it should be committed.
(In reply to comment #114)
> I found that IME's transaction is not committed when the window is being
> deactivated.

Also on Mac, IME's transaction is not committed when switch to another window in same process.
More problem, on Windows XP SP2 (Japanese)

Steps to reproduce:
1. Focus <input> or <textarea> in content.
2. Select Natural Input.
3. Start composition with IME.
4. Click Location Bar during IME transaction.

Actual Result:
IME committed string is inserted into Location Bar.

Expected Result:
IME committed string should not inserted into Location Bar.

2009-02-24's trunk + focus patch v2 (Comment #85) fails.
2009-02-24's trunk + focus patch v2 + bug 477911 (Comment #113) fails.

2009-02-24's trunk (without patch) works fine.
2009-03-10's trunk (without patch) works fine.
(In reply to comment #105)
> Is that the bit that you wanted me to review, or anything else? 
Yes
(In reply to comment #117)
> (In reply to comment #105)
> > Is that the bit that you wanted me to review? 
> Yes
There are new test builds at https://build.mozilla.org/tryserver-builds/2009-03-10_12:28-neil@mozilla.com-refocus3c/

Can you check if these fix the IME issues?
(In reply to comment #119)
> Can you check if these fix the IME issues?

Thanks for new test build.
On Windows, all mentioned problems have gone.
But on Mac OS X 10.5.6, comment #115 is still reproduced.
(In reply to comment #120)
> On Windows, all mentioned problems have gone.

But I found another problem on Windows.

1. If intl.enable_tsf_support is changed to true, reset it and restart Firefox.
2. Focus <input>, <textarea>, Location Bar or somewhere which can
be inputted.
3. Select Microsoft IME.
4. Start composition, for eaxample input some text in Hiragana mode.
5. Activate another window.

Actual Result:
IME's transaction is not committed.

Expected Result:
IME's transaction should be committed.
(In reply to comment #119)
> There are new test builds at
> https://build.mozilla.org/tryserver-builds/2009-03-10_12:28-neil@mozilla.com-refocus3c/

Summary:

On Windows, with intl.enable_tsf_support is false (default),
IME composition string is not committed when another window is activated. It should be committed.

On Mac,
IM composition string is not committed when another window in same process is activated. It should be committed.
IM composition string is not committed when another application is activated, but it's not a problem. AFAIK it's a spec of Bug 327003.

I don't see any other problem now.
> On Windows, with intl.enable_tsf_support is false (default),
> IME composition string is not committed when another window is activated. It
> should be committed.
> On Mac,
> IM composition string is not committed when another window in same process is
> activated. It should be committed.
> IM composition string is not committed when another application is activated,
> but it's not a problem. AFAIK it's a spec of Bug 327003.

Can you explain what I should be seeing in these cases in more detail? I don't see any difference in behaviour when switching windows between builds with this patch and those without.
(In reply to comment #123)
> > On Windows, with intl.enable_tsf_support is false (default),
> > IME composition string is not committed when another window is activated. It
> > should be committed.

1. Choose MS-IME (or Microsoft IME).
2. Focus <input>, <textarea> or something.
3. Choose Hiragana in Input Mode icon. Input Mode icon appears as "A", "あ" or something. Hiragana mode is "あ".
4. Type some text. The inputted string should be underlined.
5. Switch to another window. E.g. click on another window.

Result with patch:
The text of 4. is still underlined.

Result without patch:
The text of 4. is not underlined after switching window.

> > On Mac,
> > IM composition string is not committed when another window in same process is
> > activated. It should be committed.

1. Enable Kotoeri in System Preferences -> International -> Input Menu.
2. Focus <input>, <textarea> or something.
3. Choose Hiragana in input menu in the menubar.
4. Type some text. The inputted string should be underlined.
5. Switch to another window in the same application. E.g. Choose File -> New Window by mouse.

Result with patch:
The text of 4. is still underlined.

Result without patch:
The text of 4. is not underlined.

> > IM composition string is not committed when another application is activated,
> > but it's not a problem.

1. Enable Kotoeri in System Preferences -> International -> Input Menu.
2. Focus <input>, <textarea> or something.
3. Choose Hiragana in input menu in the menubar.
4. Type some text. The inputted string should be underlined.
5. Switch to another application. E.g. click on another application.

Result with and without patch:
The underlined text of 4. is still underlined.
> The one thing I wonder about is how the :focus pseudo-class ought to interact
> with the somewhat tree-like nature of focus.  That said, I don't know if there
> are ever two nodes in the tree in the same document...

There is only ever one focused node in the entire application focused at a time. Each window, however maintains a reference to a content node that would be focused if that window was focused. If that node happens to be an iframe and the toplevel window is focused, it means that the the real focus in within that child frame. The node in the child frame is focused (and has had SetContentState(childframeNode) called), but the iframe itself is not actually focused. The references are used to navigate to the real focus and, for example, drill down to find out what to restore the focus to when a window is raised.

> although I wonder if that really ought to be the case (e.g., should
> scrollbars from 'overflow:scroll' really behave differently from
> scrollbars from an iframe?).

I don't follow what you're asking here.
Attached patch focus patch v3 (obsolete) — Splinter Review
This patch addresses the comments and does this following:

 - removed some unnecessary QI usage
 - added cycle collection to the focus manager
 - added some null-checks to the MoveFocus/ClearFocus methods
 - fixed the problem with test_bug430351.html
 - update for new event suppression during synchronous task code
 - move select look and feel flag initializetion code in nsHTMLInputElement so
   that parsing html from xpcshell doesn't fail
 - fix Linux memory leaks by updating the focus window in nsWindow.cpp properly
 - fixed a leak in nsPrintEngine.cpp
 - fix IME issues
 - update command dispatcher code, it was setting the focus on the current
   window instead of the root window
 - added command dispatcher tests
 - added the 'mouse' flag for the context menu focusing code (nsXULPopupListener.cpp)

The patch now passes all tests without leaks on the Linux tryserver.
Attachment #367540 - Flags: review?(Olli.Pettay)
Blocks: 418521
Neil, I've tested your v3 patch on OS X 10.5.6 and found one problem:

Your new test_focus.xul Chrome test has 35 failures.  This is true
whether it's run by itself or with all the other Chrome tests.

All the Minefield-specific tests at bug 314160 comment #19 still pass. 

The crash running the 473284.xul crashtest (stack trace in comment
#95) no longer happens.
OK, thanks. I didn't test the new command dispatcher checks I added with the different cases of the keyboard access pref set. I'll update the test a bit but that should be the only change needed.
test_focus.xul does still depend on whether mouse is at the place where the
new window opens (on linux).

No leaks when running chrome/browser-chrome/mochitest.
(In reply to comment #125)
> There is only ever one focused node in the entire application focused at a
> time. Each window, however maintains a reference to a content node that would
> be focused if that window was focused.

Is it the latter that's used for matching :focus?

> If that node happens to be an iframe and
> the toplevel window is focused, it means that the the real focus in within that
> child frame. The node in the child frame is focused (and has had
> SetContentState(childframeNode) called), but the iframe itself is not actually
> focused. The references are used to navigate to the real focus and, for
> example, drill down to find out what to restore the focus to when a window is
> raised.

So if the parent document has :focus { border-color: green; } and the focus is somewhere inside an iframe, does the iframe match the :focus style?  (What's the behavior before the patch and with?)
Comment on attachment 367540 [details] [diff] [review]
focus patch v3

r-, especially because of delayed event handling.
Attachment #367540 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #130)
> > There is only ever one focused node in the entire application focused at a
> > time. Each window, however maintains a reference to a content node that would
> > be focused if that window was focused.
> 
> Is it the latter that's used for matching :focus?

The former. Only one node will match :focus at any given time. Are there cases where this shouldn't be the case?

> So if the parent document has :focus { border-color: green; } and the focus is
> somewhere inside an iframe, does the iframe match the :focus style?

The iframe does not match, and this is the same behaviour as is currently the case.

The only difference I've noticed is that current code doesn't clear the :focus style when another window is raised, while with this patch, it does. But that's actually a bug in the current code.
> Does this case happen only with <input type="file">? If so, move this
> to nsGenericInputElement::PreHandleEvent. Or do we get focus event also
> with other form elements?

This code is used for all form controls.


> Have you tested that :focus works still correctly in CSS?

It does, but that wouldn't relate to this code anyway.


> Does this cause any changes to 'change' event which <select> fires in its
> ::SetFocus()

I don't see any difference. The change event fires before the blur as is the case currently.


>  nsresult
>  nsHTMLLabelElement::PostHandleEvent(nsEventChainPostVisitor& aVisitor)
>  {
>    if (mHandlingEvent ||
> -      (!NS_IS_MOUSE_LEFT_CLICK(aVisitor.mEvent) &&
> -       aVisitor.mEvent->message != NS_FOCUS_CONTENT) ||
> +      !NS_IS_MOUSE_LEFT_CLICK(aVisitor.mEvent) ||
>        aVisitor.mEventStatus == nsEventStatus_eConsumeNoDefault ||
>        !aVisitor.mPresContext) {
>      return NS_OK;
>    }
> So you handle possible recursion somewhere else?

Not sure what you're referring to here. The for attribute referring to the same node itself?


> +
> +        // make sure that the element is really focused in case an earlier
> +        // listener in the chain changed the focus.
> +        if (editableRoot) {
> +          nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> +          NS_ENSURE_TRUE(fm, NS_OK);
> +
> +          nsCOMPtr<nsIDOMElement> element;
> +          fm->GetFocusedElement(getter_AddRefs(element));
> +          if (!SameCOMIdentity(element, target))
> +            return NS_OK;
> +        }
> Why you move this here?

If I recall, this code is needed to fix bug 246576, yet changing the code yet leaving it where it was caused an issue where the caret wasn't appearing in design mode documents. In a designmode document, the caret should be shown despite even though no content is focused.


> So we don't need activation event hereto set window's state?
> This doesn't regress bug 480768 or bug 480767?

Activation/Deactivation events don't go through the presshell any more, they only go to the webshell which calls the focus manager directly.
(In reply to comment #134)
> > So you handle possible recursion somewhere else?
> 
> Not sure what you're referring to here. The for attribute referring to the same
> node itself?
The reason for mHandlingEvent, Bug 401160. Though that has in-testsuite+
Comment on attachment 364039 [details] [diff] [review]
focus patch v2

OK, r=dbaron on the issues I was asked to review (see comment 80,
comment 86, comment 105, comment 117, comment 125, comment 130, comment
133 -- though primarily comment 105)
Attached patch focus patch v4 (obsolete) — Splinter Review
- addressed comments:
  - changed document.activeElement to just check the owner document
  - renamed ContentInvalid to ContentRemoved
  - added a presshell argument to the delayed events structure
  - add cycle collection for nsGlobalWindow::mFocusedNode
  - several minor code adjustments
- changed mActiveWindow to be an nsPIDOMWindow
- removed an unnecessary docshell SetVisibility call in nsGlobalWindow::Focus
- added a check when lowering a window so that the same window cannot be
  raised while it is happening -- otherwise the OS can get confused as to
  which window is actually in front. This caused an issue where the More Info
  button in the identity popup was opening a window behind the browser.
- added a Focus method to the html label element since it wasn't redirecting
  focus properly. Added a test for this.
- modified the nsHTMLLabelElement so that redirected focus from clicks is
  treated as a key focus. Both this and the previous item were found while
  testing the comment about labels.
- modified the selection code a bit in nsHTMLInputElement
- fixed a Mac leak on the browser-chrome tests. It was caused because the
  window lowered notification never fires when nsIAppStartup::eForceQuit is
  used to quit. This is the changes in nsFocusManager::WindowLowered
Attachment #362321 - Attachment is obsolete: true
Attachment #364039 - Attachment is obsolete: true
Attachment #367540 - Attachment is obsolete: true
Attachment #370650 - Flags: review?(Olli.Pettay)
> Aren't you removing the functionality of CheckAncestryAndGetFrame call here?
> .activeElement should apparently return the frame element, if active element
> is in some subframe.

Passing PR_FALSE to nsFocusManager::GetFocusedDescendant means only get the focus from that window, whereas passing PR_TRUE would be used to get nodes down if child documents. In the former case, if a child is focused, the frame element is returned. So it already does what CheckAncestryAndGetFrame is doing.


> Hmm, this gets the global "focused element", right?
> And you replace mCurrentFocus with GetFocusedContent(). This means
> that mCurrentTargetContent may start to point to some element outside mDocument.
> Does this change the behavior of mousescroll for example? On OSX
> focus may be in one window, but mousescroll is dispatched to another window.
> (that is the behaviour of OSX applications, not only gecko's)
> I guess it doesn't change that behavior, but maybe something else.
> Anyway, something to think about.

I'll change this so that only GetContentState uses this technique, and changed GetFocusedContent to just get the focus in that window.


> What happens if one tries to focus <input type="file" style="display: none;">?
> I guess nothing, and that is the current behavior too?

Elements with 'display: none' aren't focusable. This check is made is nsFrame::IsFocusable, and hasn't been changed.
The current behaviour of:

<input type="file" style="display: none;"><button onclick="this.previousSibling.focus();">Fred</button>

is that clicking the button clears the focus. With this patch, the focus remains on the button. Not sure if there's a reason for one behaviour or the other, but the latter seems more logical to me -- it's also what Safari does. IE throws an exception upon trying to focus a hidden element, so there isn't a compatibiliy issue here.


> Does this actually change the behaviour if someone sets new focused element?
> GetFocusController() returns the controller of the WindowRoot, and each
> window has its own WindowRoot. FocusManager is global.
> So the testcase could be something where there are 2 or more windows and
> SetFocusedElement is called on not-focused-window.

I don't follow this comment. There's one WindowRoot per top-level chrome window. If a window isn't active, SetFocus changes what would be focused in that window, but doesn't change anything in the active window. Similarly, if this is an inactive tab. GetRootFocusedContentAndWindow retrieves the window for that top-level chrome window and clears it.
 
commandDispatcher.focusedElement is the element in the toplevel window or descendant that is focused.

The behaviour should be the same, except that now it isn't incorrect in some cases like before ;)


> Same question here.

My changes here were slightly incorrect as setting the focus manager's focusedWindow was raising and it should not have been. I will fix this.
> This check is made is nsFrame::IsFocusable

You can't possibly be checking for display:none in that method.  display:none means there is no frame.

> There's one WindowRoot per top-level chrome window.

Or top-level content window in the embedding case, no?  Not sure whether that affects the analysis.
(In reply to comment #140)
> > This check is made is nsFrame::IsFocusable
> 
> You can't possibly be checking for display:none in that method.  display:none
> means there is no frame.

True. I was confusing that with visibility with is checked there.

> > There's one WindowRoot per top-level chrome window.
> 
> Or top-level content window in the embedding case, no?  Not sure whether that
> affects the analysis.

Yes, I don't think it affects this case though.
Attachment #370650 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 370650 [details] [diff] [review]
focus patch v4

I'd like to see still the coming changes to ESM.
+nsIContent*
+nsGlobalWindow::GetFocusedNode()
+{
+  FORWARD_TO_INNER(GetFocusedNode, (), NS_OK);
+
+  return mFocusedNode;
+}
> Is it guaranteed that mFocusedNode is in document?

Yes, I'll change the aseertion in SetFocusedNode to use GetCurrentDoc. Also, nsFocusManager::ContentRemoved calls SetFocusedNode(nsnull) when focused content is removed.

> When navigating back to the page, the focus should be in the same element
> where it was before the page was bfcached.

OK, I'll remove that.
Attached patch focus patch v5 (obsolete) — Splinter Review
Attachment #370650 - Attachment is obsolete: true
Attachment #371732 - Flags: superreview?(roc)
Attachment #371732 - Flags: review?(Olli.Pettay)
Comment on attachment 371732 [details] [diff] [review]
focus patch v5

Neil to review toolkit changes (print preview changes for example)
Attachment #371732 - Flags: review?(neil)
Attachment #371732 - Flags: review?(mconnor)
Comment on attachment 371732 [details] [diff] [review]
focus patch v5

mconnor to review browser changes
The changes in this patch from the last one:

 - address all comments
 - changed the recently added patch for bug 481560 as it was relying on firing keys at non-focused windows
 - added a test for focus handling during history navigation
Given that Olli has reviewed this, is going to finish reviewing it, is a super-reviewer, and is generally a better reviewer than me, should I really super-review this?
Attachment #371732 - Flags: superreview?(roc)
Attachment #371732 - Flags: superreview?(Olli.Pettay)
Attachment #371732 - Flags: review?(Olli.Pettay)
Comment on attachment 371732 [details] [diff] [review]
focus patch v5

Ok, if Neil reviews this too and mconnor checks at least the toolkit parts, 
I could sr.
Comment on attachment 371732 [details] [diff] [review]
focus patch v5

should get an accessibility review too
Attachment #371732 - Flags: review?(marco.zehe)
Attached file OS/2 problem log
(In reply to comment #102)
> Peter, can you create a build which has the two debugging defines near the top
> of nsFocusManager.cpp enabled, and post or send me the log of what happens?

Neil, I have _finally_ done this, with your latest patch. The log is attached here. I guess you can already tell this from the log, but all the entries with
   <<ClearFocus begin>>
   <<ClearFocus end>>
   <<SetFocusedWindow begin>>
   Shift Focus: (none) Flags: 0 Current Window: 0x22d4ce80
   New Window: 0x209d2940 Current Element: 0 In Active Window: 0
   In Focused Window: 0
are the ones where the focus didn't arrive in the entry field of the first window. Only the very last entry (after I Shift-reloaded the problematic page) is connected to a successful switch into the entry field.

> It's possible that os2 needs to change DispatchFocus to DispatchFocusToTopLevel
> as is done on Windows.

I have looked at this for some (short) time. I couldn't construct such a function for OS/2 so far. Will look at this again, but there are also many other things about focus throughout os2/nsWindow.cpp which might be interfering.
(In reply to comment #152)
> Created an attachment (id=371931) [details]
> OS/2 problem log

OK, it looks as if clicking the mouse on an window in the background isn't informing of the window being activated properly.

I'm guessing that WM_ACTIVATE works a bit differently on os2 than on windows and that it gets sent to child windows as well (on windows it only gets sent to toplevel windows)

If that's the case, then I think my original suspicion about using DispatchFocusToTopLevel was correct. The change to nsFrameWindow.cpp shouldn't have been done, and it should instead be something like:

      case WM_ACTIVATE:
        DEBUGFOCUS(frame WM_ACTIVATE);
        if (SHORT1FROMMP(mp1) &&
            !(WinQueryWindowULong(mFrameWnd, QWL_STYLE) & WS_MINIMIZED)) {
-           bDone = DispatchFocusToTopLevel(NS_GOTFOCUS, PR_TRUE);
+           bDone = DispatchFocusToTopLevel(NS_GOTFOCUS);
        }
        break;

where DispatchFocusToTopLevel is a function which retrieves the top level window, resets the gJustGotActivate flag and calls DispatchFocus, similar to the Windows DispatchFocusToTopLevel method this patch adds. I don't know how to retrieve the toplevel window on os2 though.
Please don't spend a lot of time trying to make this work on OS/2.
Yeah, to clarify, I don't plan on implementing it myself (as I don't know how and wouldn't be able to test it), but would incorporate any changes needed.
Depends on: 488846
I'm waiting others to review before doing the (hopefully) final sreview.
Comment on attachment 371732 [details] [diff] [review]
focus patch v5

>+  // blur the field to ensure that the popup is closed and that the previous
>+  // search has stopped, then start a new search.
>+  autocomplete.blur();
Out of interest, what does blur() do these days? Does it actually work given that it's the inner input that actually has focus?

>-  synthesizeMouse(document.documentElement, 1, 1, { });
>-
>+  // the first tab focuses the document so skip it
Why this change? Can you be sure where the focus is?
In a SeaMonkey build against mozilla-central with the patch, the form fill popup causes focus to be lost. This is particularly annoying when deleting characters from the end of the field, as the next backspace then goes back in history...
(In reply to comment #157)
> Out of interest, what does blur() do these days? Does it actually work given
> that it's the inner input that actually has focus?

If the element is focused, blur() clears the focus from the document. The focus manager retargets focus/blur events from textboxes to their inputs. See nsFocusManager::GetRedirectedFocus

> >-  synthesizeMouse(document.documentElement, 1, 1, { });
> >-
> >+  // the first tab focuses the document so skip it
> Why this change? Can you be sure where the focus is?

The synthesizeMouse line isn't necessary as the document is already focused -- the tests run 'onfocus'. Before it just didn't work too well.
Neil, mconnor, marcoz: please review this soon! I think my compositor widget-removal work is depending on it...
Comment on attachment 371732 [details] [diff] [review]
focus patch v5

>diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp
>--- a/accessible/src/base/nsAccessible.cpp
>+++ b/accessible/src/base/nsAccessible.cpp
>@@ -1453,17 +1453,16 @@ nsAccessible::TakeFocus()
> 
>   nsCOMPtr<nsIDOMNSHTMLElement> htmlElement(do_QueryInterface(content));
>   if (htmlElement) {
>     // HTML Elements also set the caret position
>     // in order to affect tabbing order
>     return htmlElement->Focus();
>   }
> 
>-  content->SetFocus(GetPresContext());

Why was this fallback removed?

r=me with this explained/fixed-if-needed for the accessibility part.
Attachment #371732 - Flags: review?(marco.zehe) → review+
(In reply to comment #160)
> Neil, mconnor, marcoz: please review this soon! I think my compositor
> widget-removal work is depending on it...
And I want this in before clone-documents-for-printing
(which blocks remove-support-for-multiple-presshells, and that would make 
event handling cleanup easier.)
(In reply to comment #159)
> (In reply to comment #157)
> > Out of interest, what does blur() do these days? Does it actually work given
> > that it's the inner input that actually has focus?
> If the element is focused, blur() clears the focus from the document.
So where does focus go?

> > >-  synthesizeMouse(document.documentElement, 1, 1, { });
> > >-
> > >+  // the first tab focuses the document so skip it
> > Why this change? Can you be sure where the focus is?
> The synthesizeMouse line isn't necessary as the document is already focused --
> the tests run 'onfocus'. Before it just didn't work too well.
So if the document is already focused why does the first tab focus the document?

> seems like the autocomplete panel in navigator.xul needs noautofocus="true"
Any idea why it didn't need it before? Where was noautofocus implemented, and does the default depend on whether the element is a panel or not?
(In reply to comment #163)
> So where does focus go?

The document/window remain focused, but no element is focused, like what happens when clicking a blank area of a web page. The next tab key will just start from the beginning.

> > the tests run 'onfocus'. Before it just didn't work too well.
> So if the document is already focused why does the first tab focus the
> document?

It should probably say 'document root', as in the state where a focus ring appears around the document body when you tab into it. Well, it would do if was an html document. In xul, nothing of note gets drawn.

> Any idea why it didn't need it before? Where was noautofocus implemented, and
> does the default depend on whether the element is a panel or not?

Don't know why it didn't before. It's handled in nsXULPopupManager::FirePopupShowingEvent. It may be that it needs to look at the current focus only within the same document. I'll investigate a bit more, but noautofocus should be used anyway, I would think.
(In reply to comment #164)
> Don't know why it didn't before. It's handled in
> nsXULPopupManager::FirePopupShowingEvent. It may be that it needs to look at
> the current focus only within the same document. I'll investigate a bit more,
> but noautofocus should be used anyway, I would think.

I think this is because nothing is focused within the document the popup is within. (The existing code is wrong I think.) But focus should be cleared when the panel opens unless it has the noautofocus attribute, so that focus can move to the panel. In this case, the autocomplete panel doesn't need focus directly and wants it kept on the page, so it should be using noautofocus.
Blocks: 385609
(In reply to comment #159)
> The focus manager retargets focus/blur events from textboxes to their inputs.
> See nsFocusManager::GetRedirectedFocus
Isn't that a little delicate? What if someone wants to create a new widget which wraps an existing focusable element?

> > >-  synthesizeMouse(document.documentElement, 1, 1, { });
> > >-
> > >+  // the first tab focuses the document so skip it
> > Why this change? Can you be sure where the focus is?
> The synthesizeMouse line isn't necessary as the document is already focused --
> the tests run 'onfocus'. Before it just didn't work too well.
Sorry, I'm not following - I still can't see how the document can already have focus yet the first tab focuses the document.

(In reply to comment #165)
> > but noautofocus should be used anyway, I would think.
> I think this is because nothing is focused within the document the popup is
> within. (The existing code is wrong I think.)
OK, makes sense.
(In reply to comment #166)
> (In reply to comment #159)
> > The focus manager retargets focus/blur events from textboxes to their inputs.
> > See nsFocusManager::GetRedirectedFocus
> Isn't that a little delicate? What if someone wants to create a new widget
> which wraps an existing focusable element?

Indeed, which is why I plan on changing it very soon afterwards.

> Sorry, I'm not following - I still can't see how the document can already have
> focus yet the first tab focuses the document.

Before pressing tab, nothing in the document is focused, but the document still receives key events. After pressing tab, the document root is focused (in html a focus ring appears around the entire document).
Blocks: 490465
Blocks: 490847
> Before pressing tab, nothing in the document is focused, but the document still
> receives key events. After pressing tab, the document root is focused (in html
> a focus ring appears around the entire document).
So that's a deliberate change from the current behaviour? Currently when you  follow a link, the focus moves to the new document (but the focus ring does not appear around the document because the focus was not moved by the tab key), and tabbing takes you to the first tabbable element in the new document.
Attachment #371732 - Flags: review?(mconnor) → review?(gavin.sharp)
Comment on attachment 371732 [details] [diff] [review]
focus patch v5

mconnnor says Gavin agreed to do this
Blocks: 327102
Blocks: 453377
Comment on attachment 371732 [details] [diff] [review]
focus patch v5

(Still no answer to comment #168.)
This blew up on me when I tried to find in page on a bugzilla attachment details page. Steps to reproduce problem:
1. Edit a bugzilla attachment
2 [review]. Click Edit attachment as comment
At this point focus is dead. Worse is to come though if you built SeaMonkey using comm-central on mozilla-central plus this patch.
3. Since focus is dead, use the mouse to open the find dialog (Edit - Find in this Page)
4. Try to find something.
5. Watch find crash as it fails to find a frame for the focused element.
Attachment #371732 - Flags: review?(neil) → review-
Attached patch focus patch v6 (obsolete) — Splinter Review
Addresses comments. Changes in this version:

- address accessibility comments (nsAccessible.cpp)
- modify utilityOverlay.js focusElement() so that focus() isn't called on a window but instead on its frame. This was causing additional work to raise the window earlier than it should which have which affected the performance tests.
- fix up the comment in test_tabindex.xul
- had to fix up the newly added ime tests added in bug 460059 to not use nsDOMWindowUtils::Focus and adjust because ime state shouldn't change for designMode documents since they don't allow focusing html elements
- adjusted the input type="file" focus() method to focus the button instead of the file. This was also an issue with the ime tests from bug 460059 but also happens to fix bug 453377 as well.
- had to fix up the download manager test test_select_all.xul as it was failing on linux
- addressed Neil's comment 168 about the first tab on a document (change in nsFocusManager::DetermineElementToFocus)
- addressed Neil's comment 170. Null-pointer crash in nsWebBrowserFind.cpp and change to nsPresShell::HandleEvent to use the root node if the currently focused content no longer has a frame
- Masayuki pointed out that the documentation for nsDOMWindowUtils::Focus says that passing null should clear the focus so added this, even though the method is deprecated.
- removed a stray line left over from DocumentViewerImpl::Close
Attachment #371732 - Attachment is obsolete: true
Attachment #376622 - Flags: superreview?(Olli.Pettay)
Attachment #376622 - Flags: review?(Olli.Pettay)
Attachment #371732 - Flags: superreview?(Olli.Pettay)
Attachment #371732 - Flags: review?(gavin.sharp)
Attachment #376622 - Flags: review?(gavin.sharp)
Attachment #376622 - Flags: review?(neil)
Comment on attachment 376622 [details] [diff] [review]
focus patch v6

Some oddities observed while testing this patch:
* A quick "click" on the taskbar is insufficient to switch between separate windows of the same application. (It's not easy to reproduce because your click has to be short and precise but I can't reproduce it at all on a build without the patch.)
* Calling focus() on a window raises the window but kills focus (same effects as edit attachment used to have, but without the find crash).
* The find dialog focuses the page that you're finding in, when it should retain focus.

>+function focusElement(aElement)
There seem to be three callers of this:
* Focusing the URLbar on a timeout (which may have allowed the window to lose activation) which looks as if it should be replaced with a simple focus call.
* Focusing the active tab, which maybe should be renamed focusContent() ?
* And one rather sneaky use, which is to transfer focus to the active tab of another window, but only if that window didn't have a popup. I don't think your replacement caters for that!

>+  // if a content window, focus the <browser> instead as window.focus()
>+  // raises the window
>+  if (aElement instanceof Window)
>+    getBrowserFromContentWindow(aElement).focus();
This interestingly seems to focus the previously focused content element, i.e.
* Focus somewhere in content
* Press Ctrl+L
* Press Ctrl+Shift+J
* Evaluate top.opener.getBrowser().selectedBrowser.focus()
I can see that being useful :-)

>+      // otherwise, if there is no focused content, just use the root content
>+      if (!mCurrentEventContent)
>+        mCurrentEventContent = mDocument->GetRootContent();
>+      mCurrentEventFrame = nsnull;
>+
>+      // if the focused content has no frame, use the root content as well.
>+      // This ensures that key events still get sent properly if a frame goes
>+      // away while the frame is focused.
>+      if (!GetCurrentEventFrame())
>+        mCurrentEventContent = mDocument->GetRootContent();
>+      mCurrentEventFrame = nsnull;
Can't you write this as
if (!mCurrentEventContent || !GetCurrentEventFrame())
  mCurrentEventContent = mDocument->GetRootContent();
since if the root content doesn't have an event frame then it's not going to help to get the root content again ;-)
(In reply to comment #172)
> * A quick "click" on the taskbar is insufficient to switch between separate
> windows of the same application. (It's not easy to reproduce because your click
> has to be short and precise but I can't reproduce it at all on a build without
> the patch.)

Can't reproduce this myself. What platform/os is this?

> * Calling focus() on a window raises the window but kills focus (same effects
> as edit attachment used to have, but without the find crash).

How do I reproduce this?

> * The find dialog focuses the page that you're finding in, when it should
> retain focus.

Modified nsWebBrowserFind.cpp to fix this

> * And one rather sneaky use, which is to transfer focus to the active tab of
> another window, but only if that window didn't have a popup. I don't think your
> replacement caters for that!

Modified the code here to hopefully handle this last case

> Can't you write this as
> if (!mCurrentEventContent || !GetCurrentEventFrame())
>   mCurrentEventContent = mDocument->GetRootContent();
> since if the root content doesn't have an event frame then it's not going to
> help to get the root content again ;-)

Fixed
Attachment #376740 - Flags: review?
Attachment #376740 - Flags: review? → review?(neil)
Comment on attachment 376740 [details] [diff] [review]
this patch applies on top of the other one and fixes most of Neil's issues

This fixes the find bug; I can't test the other code. I was also unable to reproduce my odd taskbar issue.
Attachment #376740 - Flags: review?(neil) → review+
I tripped over another crash today. As I recall, I was dismissing an alert while a page was loading in the background, and when the previous window activated the focused element's document no longer had a window to raise.
Attached file crash stack
I crashed in a build with this patch today, here's the stack. It's the only crash I've seen since applying it several days ago.
I have windowless IFRAMEs working pretty well ... except for focus. Please please let's get this landed!
Blocks: 83496
No longer depends on: 83496
Comment on attachment 379612 [details]
crash stack

>bug Thread 0 Crashed:
>0   libgklayout.dylib                 0x12234f4d nsFocusManager::SetFocusInner(nsIContent*, int, int) + 221

Is it possible that newWindow is null here:
nsCOMPtr<nsIDocShell> newDocShell = newWindow->GetDocShell();

GetCurrentWindow() may return null.
Blocks: 481468
Blocks: 477300
Blocks: 482736
Blocks: 461102, 441158
Blocks: 396490
Attachment #376622 - Flags: superreview?(Olli.Pettay)
Attachment #376622 - Flags: superreview-
Attachment #376622 - Flags: review?(Olli.Pettay)
Attachment #376622 - Flags: review-
Comment on attachment 380503 [details]
comments

nsXULCommandDispatcher::GetSuppressFocusScroll(PRBool* aSuppressFocusScroll)
> {
>+  return NS_OK;
> }
Shouldn't this return a value of some sort?
Comment on attachment 376622 [details] [diff] [review]
focus patch v6

I no longer have a tree to which this applies. Would you mind posting the latest update please?
Attachment #376622 - Flags: review?(neil)
Blocks: 495625
Blocks: 495624
> +  // XXXndeakin P3 accessibility shouldn't be caching the focus
> Please file a bug and add the bug number here.

Filed bug 495624.

> Either change the name of the method to not contain word "Element", or at least
> file a new bug to do that.

I'll file a new bug. Some code or extension may be using this method.

> +  // If the legend isn't focusable, focus whatever is focusable following
> +  // the legend instead, bug 81481.
> Is there a testcase for this? Even just manual test?

Yes, in window_focus.xul

> +  // XXXndeakin even though this is only used for key events which should be
> +  // going to the focused frame anyway, this doesn't seem like the right way
> +  // to determine if something is an editor.
> Please file a new bug to fix this. Add bug number here.

Filed bug 495625.

> Why this change? If something needs now a timeout which wasn't needed before, > that sounds like a bug.

See comment 49.
Blocks: 495672
Blocks: 299673
Attached patch focus patch v7 (obsolete) — Splinter Review
Changes:

browser_tabfocus.js - focused the window in the new browser_tabfocus.js test
before starting the test. An earlier test for some reason still has a window
open and it was interfering with this one.
nsDocument::GetActiveElement - changed to use NS_WARNING
nsDocument::SuppressEventHandling - update for latest event suppression changes
nsXULCommandDispatcher::GetSuppressFocusScroll - set return value correctly
nsFocusManager::SetFocusInner - check for null newWindow
nsIPresShell.h - update for latest event suppression changes
nsPresShell.cpp::nsDelayedFocusBlur - remove, update for latest event suppression changes 
PresShell::HandleEvent - update for latest event suppression changes 
PresShell::FireOrClearDelayedEvents - remove, update for latest event suppression changes
browser_passwordmgrdlg.js - changed to use focus events rather than load events

plus the changes from attachment 376740 [details] [diff] [review].
Attachment #376622 - Attachment is obsolete: true
Attachment #376740 - Attachment is obsolete: true
Attachment #380854 - Flags: superreview?(Olli.Pettay)
Attachment #380854 - Flags: review?(Olli.Pettay)
Attachment #376622 - Flags: review?(gavin.sharp)
Attachment #380854 - Flags: review?(gavin.sharp)
Attachment #380854 - Flags: superreview?(Olli.Pettay)
Attachment #380854 - Flags: superreview+
Attachment #380854 - Flags: review?(Olli.Pettay)
Attachment #380854 - Flags: review+
Comment on attachment 380854 [details] [diff] [review]
focus patch v7

This will probably cause some regressions, but better to notice them as early as possible.
it would be nice to get this on trunk when the tree is no longer restricted 

it fixes a ton of bugs in my view
Actually, I think it should land during the restriction, or a closed tree, to keep the noise to a minimum, to find any possible reggressions on the 'tree'.
Indeed, landing while the tree is in the restricted state it is now (we'd need special permission since this isn't a 1.9.1 blocker) would help a lot with tracing regressions since we carry nightlies much longer than hourlies.
anybody care to contact one of the devs or whoever to get such permision
The patch still needs review (from Gavin) before that can happen.  And the patch in the bug this bug depends on needs to land.

Once this is set to land, I do think we should close the tree for a day to let it land.  Please post to .planning (and cc .platform) to schedule that once it's clear when this will be ready?

If this is ready before we've reopened m-c, probably best to just land before reopening.  If not, we shouldn't hold the tree closed for days waiting on this; better to close it as needed when this patch is ready.
We'll get this on mozilla-central once we've frozen for RC1 on mozilla-1.9.1. Not ideal for everyone, but we need to focus on releasing software. We can keep the tree closed for this landing only if you'd like. bz's gonna ask around in the newsgroups.

There is no way this is going onto 1.9.1, not that I think any serious-thinking person has proposed that.
Blocks: 322588
Patch v7 crashes on Linux when enabling caret browsing, STR:
1. press F7, click OK => crash

#6  in nsFocusManager::UpdateCaret 
1622      nsCOMPtr<nsIDocShell> focusedDocShell = mFocusedWindow->GetDocShell();
(gdb) p mFocusedWindow
$1 = {mRawPtr = 0x0}


Also, there's a new warning when clicking on text:
WARNING: NS_ENSURE_TRUE(aContent) failed: file layout/base/nsFrameManager.cpp, line 335
Blocks: 496307
> Nit, this could be nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(parent);
return window.forget();

Will fix this.


> This is not what happens currently. We don't dispatch focus or blur events
> either if window is blurred because some other window is focused.
> Only focus and blur events which are caused by some script are dispatched
> immediately even if the document is suppressed.
> So, is the comment just a bit wrong or are you actually changing the behavior.
> And if you're changing the behavior, is there a reason for that, or is it
> just a bug?

Discussed with smaug, and it's just the comment that is misleading which I'll adjust. The comment only applies to the focus event, blur always passes non-zero for aFocusMethod.


> +  // XXXndeakin this doesn't seem right. It should be checking for this only
> +  // on the nearest ancestor frame which is a chrome frame. But this is
> +  // what the existing code does, so just leave it for now.
> File a bug for this.

Filed bug 496307


> #6  in nsFocusManager::UpdateCaret 
> 1622      nsCOMPtr<nsIDocShell> focusedDocShell =
> mFocusedWindow->GetDocShell();
> (gdb) p mFocusedWindow
> $1 = {mRawPtr = 0x0}

I added a null check here.

> Also, there's a new warning when clicking on text:
> WARNING: NS_ENSURE_TRUE(aContent) failed: file layout/base/nsFrameManager.cpp,
> line 335

I'll investigate this.
Blocks: 496309
(In reply to comment #193)
> > #6  in nsFocusManager::UpdateCaret 
> > 1622      nsCOMPtr<nsIDocShell> focusedDocShell =
> > mFocusedWindow->GetDocShell();
> > (gdb) p mFocusedWindow
> > $1 = {mRawPtr = 0x0}
> 
> I added a null check here.

Why is it null?
(In reply to comment #175)
> I tripped over another crash today. As I recall, I was dismissing an alert
> while a page was loading in the background, and when the previous window
> activated the focused element's document no longer had a window to raise.
I tripped over what I think was the same crash again today. In this case I was dismissing a slow script alert caused by a jquery unload handler. I crashed at nsPIDOMWindow::GetDocShell (this = 0x0) called from
nsFocusManager::IsWindowVisible (aWindow = 0x0) called from
nsFocusManager::CheckIfFocusable (aContent = <xul:browser>, aFlags = 0x0)
I've just noticed that accelerators for radio buttons don't move focus...
> > Also, there's a new warning when clicking on text:
> > WARNING: NS_ENSURE_TRUE(aContent) failed: file layout/base/nsFrameManager.cpp,
> > line 335
> 
> I'll investigate this.

I'll add a check for this in nsFocusManager::Blur so that GetPrimaryFrameFor isn't called with a null argument.

> I tripped over what I think was the same crash again today. In this case I was
> dismissing a slow script alert caused by a jquery unload handler. I crashed at
> nsPIDOMWindow::GetDocShell (this = 0x0) called from
> nsFocusManager::IsWindowVisible (aWindow = 0x0) called from
> nsFocusManager::CheckIfFocusable (aContent = <xul:browser>, aFlags = 0x0)

Perhaps another case where a window isn't available. I'll add a null-check if avoid the crash.
(In reply to comment #196)
> I've just noticed that accelerators for radio buttons don't move focus...

Should be an easy enough fix. It's another case where code was assuming that non-focusable elements could be focused. I'll file a followup bug though since it's a minor issue.
Comment on attachment 380854 [details] [diff] [review]
focus patch v7

I looked at the documentation first, some minor comments for nsIFocusManager.idl:

- there are mixed references to node/element and frame/window, which is slightly confusing. I guess the frame/window distinction is important, but it's not immediately obvious to me.
- "one application level focus" -> "one application-wide focused element" ?
- "if its window was active" -> "if the window was active" ?
-"the focus is moved to the window currently associated with that nsIDOMWindow" is phrased awkwardly - could just remove that sentence, I think?

- for activeWindow's comment, "focuses whatever is currently the current node in the current child window" -> "focuses the child window's focused node"?
- for moveFocus, "relative to aElement" -> "relative to aStartElement"

- for getFocusedElementForWindow, "then this element will have the focus" is confusing - I think it's trying to say "the returned element will be the application-wide focused element", right?
- for getFocusedElementForWindow, there's another "the element will be" - probably clearer as "the return value will be" ?
- for getFocusedElementForWindow, perhaps add "or to aWindow if aDeep is false" to the last paragraph?

some typos elsewhere in the documentation:
"was is an"
"heirarchy"
"focusued"

A lot of test files use the |let someFunc = function () {}| syntax rather than named functions. Can you change them to just be function someFunc(){} instead?

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+            var fm = Components.classes["@mozilla.org/focus-manager;1"].
>+                       getService(Components.interfaces.nsIFocusManager);
>+            var focusedBrowser = fm.getFocusedElementForWindow(window, false, {});

focusedBrowser should probably be focusedChromeElement.

>diff --git a/browser/base/content/test/browser_bug481560.js b/browser/base/content/test/browser_bug481560.js

> function test() {
>   waitForExplicitFinish();
> 
>+  // focus the url field so that it will can ensure the focus is there when
>+  // the window is refocused after the dialog closes
>+  gURLBar.focus();

Why is that relevant to this test?

Also not sure I understand why this test needs to use a focus event rather than just onload as it was before, can you explain?

>diff --git a/browser/base/content/test/browser_tabfocus.js b/browser/base/content/test/browser_tabfocus.js

>+function test() {

>+  var urlbar = document.getElementById("urlbar");
>+  urlbar.focus();

nit: can just use gURLBar here

>+    // blurring an element in the current tab should clear the active focus
>+    expectFocusShift(function () button1.blur(),
>+                     browser1.contentWindow, null, true,
>+                     "focusedWindow after blur in focused tab");
>+    is(fm.getFocusedElementForWindow(browser1.contentWindow, false, {}), null, "blur in focused tab");

This is() check seems redundant, expectFocusShift already tests that right?

>+    // switching focus to chrome when it has no focused element should clear the focus
>+    expectFocusShift(function () fm.clearFocus(window),
>+                     window, null, true,
>+                     "focusedWindow after switch to chrome with no focused element");

comment doesn't seem to match what you're testing exactly. Is there any way for a user to trigger the equivalent of clearFocus()'s behaviour?

seems like clearFocus' documentation should mention that it sets focus to the passed in window if it is an ancestor of the current focused window.

>+var _browser_tabfocus_test_lastfocuswindow = "";

nit: why = ""?

>+function expectFocusShift(callback, expectedWindow, expectedElement, focusChanged, testid)

>+  var focusedElement = fm.focusedElement;
>+  is(focusedElement ? getId(focusedElement) : "none",
>+     expectedElement ? getId(expectedElement) : "none", testid + " focusedElement");

why can't this compare focusedElement and expectedElement directly?

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

>+  // Focus the content but don't raise the window, since the URI we just loaded
>+  // may have resulted in a new frontmost window (e.g. "javascript:window.open('');").
>+  if ("getBrowserFromContentWindow" in w) {

When would this check fail? We already assume above that w has a gBrowser property, and getTopWin only returns navigator:browsers.

>diff --git a/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js b/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js

Same question here - why does this need to use a focus event now?

>diff --git a/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp b/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp

>+      nsCOMPtr<nsIDOMElement> focusedElement;

unused?

>diff --git a/toolkit/content/tests/widgets/test_focus_anons.xul b/toolkit/content/tests/widgets/test_focus_anons.xul

>+  gExpectedBlur = gExpectedFocus;
>+  gExpectedFocus = document.getAnonymousNodes($("scale"))[0];
>+  synthesizeKey("c", accessKeyDetails);
>+
>+  gExpectedBlur = document.getAnonymousNodes($("scale"))[0];

can use gExpectedFocus here, no?

>diff --git a/toolkit/content/widgets/menulist.xml b/toolkit/content/widgets/menulist.xml

>       <handler event="focus" phase="capturing">
>         <![CDATA[
>-          if (!this.hasAttribute('focused')) {
>-            if (event.originalTarget != this.inputField)
>-              this.inputField.focus();
>-            this.setAttribute('focused','true');
>-          }
>+          this.setAttribute('focused','true');
>         ]]>
>       </handler>

Why is this change needed? The textbox binding has similar code - does it need changing too?

>diff --git a/toolkit/themes/pinstripe/global/menulist.css b/toolkit/themes/pinstripe/global/menulist.css

> menulist[editable="true"] {
>-  -moz-user-focus: normal;

I have no idea how to evaluate the significance of this change. Seems like this has been in pinstripe since forever, but not on other platforms... what practical effect does it have?

r=me on all the toolkit/ and browser/ parts, but I didn't review the typeaheadfind changes very carefully (I'm assuming Neil looked at them).
Attachment #380854 - Flags: review?(gavin.sharp) → review+
(In reply to comment #199)
> >+  // focus the url field so that it will can ensure the focus is there when
> >+  // the window is refocused after the dialog closes
> >+  gURLBar.focus();
> 
> Why is that relevant to this test?

This was an issue on Linux. The content area was focused, so the chrome window didn't receive a focus event.

> Also not sure I understand why this test needs to use a focus event rather than
> just onload as it was before, can you explain?

The window isn't focused until the focus event fires, which can happen after the load event fires. Tests that rely on the window being focused should use the focus event instead.

> why can't this compare focusedElement and expectedElement directly?

So that if it fails, the element ids appear in the log rather than the less useful object name.

> >       <handler event="focus" phase="capturing">
... 
> Why is this change needed? The textbox binding has similar code - does it need
> changing too?

> > menulist[editable="true"] {
> >-  -moz-user-focus: normal;
> I have no idea how to evaluate the significance of this change. Seems like this
> has been in pinstripe since forever, but not on other platforms... what
> practical effect does it have?

Both the editable menulist related changes are there to just treat the input as focusable and never the menulist itself. Currently, for example, there's a bug where shift+tab doesn't work when an editable menulist is focused.
Attached patch focus patch v8Splinter Review
- addressed all of Gavin's and Olli's comments.
- added a null-check before calling GetPrimaryFrameFor in nsFocusManager::Blur. This fixes the FrameManager.cpp warning.
- adjusted the test to account for element.localName returning lowercase values now from bug 468708.
- added a null-check to account for Neil's comment 198.
Attachment #380854 - Attachment is obsolete: true
Checked in! (http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3)

Thanks all!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 497565
Depends on: 497633
Blocks: 140346
Depends on: 497867
Depends on: 497878
Depends on: 498075
Depends on: 498143
No longer depends on: 498143
Depends on: 498250
Blocks: 452478
Flags: in-testsuite+
Depends on: 497934
Depends on: 498609
Depends on: 498143
Depends on: 499912
Depends on: 500224
it seems this has caused the regression in bug 500275
Depends on: 500275
Depends on: 499438
Blocks: 494373
Depends on: 503047
Depends on: 503114
Depends on: 502383
Depends on: 504183
Depends on: 504224
Depends on: 503724
Depends on: 504586
Depends on: 503714
Depends on: 498805
Depends on: 503627
Depends on: 504450
Depends on: 506173
Depends on: 506754
Looks like this caused build bustage in bug 507056
Depends on: 507056
Depends on: 507592
Depends on: 508477
Depends on: 509298
Depends on: 375190
Blocks: 375190
No longer depends on: 375190
Blocks: 275621
Blocks: 263545
Depends on: 512059
Depends on: 512058
Depends on: 512561
Blocks: 469139
Depends on: 516076
Depends on: 517819
No longer depends on: 517866
Is bug 504922 related to this refactoring of the focus handling? 
If so, would this be fixed in the Firefox 3.5.x branch?
Blocks: 306058
Is bug 370031 covered by this?
Blocks: 370031
Depends on: 521951
Depends on: 524813
Blocks: 527099
Blocks: 529151
Depends on: 536481
Depends on: 536579
Depends on: 536926
Depends on: 549909
Blocks: 550878
No longer blocks: 550878
Depends on: 550878
Depends on: 551434
Depends on: 546068
Depends on: 554635
Depends on: 544146
Target Milestone: --- → mozilla1.9.2
Depends on: 566542
Depends on: 590077
Depends on: 550709
Depends on: 601344
Depends on: 565575
Depends on: 567240
Depends on: 723940
Depends on: 738781
Depends on: 621944
Comment on attachment 382498 [details] [diff] [review]
focus patch v8

>@@ -10025,30 +9905,30 @@ NS_IMETHODIMP nsDocShell::EnsureFind()
>     // we promise that the nsIWebBrowserFind that we return has been set
>     // up to point to the focused, or content window, so we have to
>     // set that up each time.
> 
>     nsIScriptGlobalObject* scriptGO = GetScriptGlobalObject();
>     NS_ENSURE_TRUE(scriptGO, NS_ERROR_UNEXPECTED);
> 
>     // default to our window
>-    nsCOMPtr<nsIDOMWindow> rootWindow = do_QueryInterface(scriptGO);
>-    nsCOMPtr<nsIDOMWindow> windowToSearch = rootWindow;
Both the window to search and the root window used to default to the *content* window...

>+    nsCOMPtr<nsIDOMWindow> windowToSearch(do_QueryInterface(mScriptGlobal));
>+
>+    nsCOMPtr<nsIDocShellTreeItem> root;
>+    GetRootTreeItem(getter_AddRefs(root));
...but now the root window is the *chrome* window. Oops.

In fact, it appears that the whole meaning of the code block has changed; as far as I can tell, the old code wanted to retrieve the per-window focused frame (OK, so it didn't work when calling EnsureFind on a background tab, but we never did that), which I believe is what nsFocusManager::GetFocusedDescendant does, although I don't see any obvious way to call it, while the new code only works if the chrome window has focus (again, we generally don't call EnsureFind on a background window either).
Neil: file a new bug. Commenting in a bug that's been closed (and uncommented in!) for 3 years is unhelpful, and unlikely to result in any action. You should know better.
(In reply to Justin Dolske [:Dolske] from comment #209)
> Neil: file a new bug. Commenting in a bug that's been closed (and
> uncommented in!) for 3 years is unhelpful, and unlikely to result in any
> action. You should know better.

... And let us know the bug number!
Depends on: 1146881
Depends on: 1328043
Regressions: 1643153
You need to log in before you can comment on or make changes to this bug.