224185 - PresShell make a wrong assumption about focus (for embedding)

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[Marco Pesenti Gritti]

In CheckForFocus:

  nsCOMPtr<nsIDOMWindowInternal> focusedWindow;
  aFocusController->GetFocusedWindow(getter_AddRefs(focusedWindow));
  if (!focusedWindow) {
    // This should never really happen, but if it does, assume
    // we can focus ourself to keep the window from being keydead.
    focusedWindow = ourWin;
  }

The comment is right for mozilla but not for embedding apps. For example in an
embedded browser you usually have the focus in the location bar when opening a
new window, and mozilla knows nothing about it.
The mozilla focused window is NULL but, because of the above code, CheckForFocus
will grab the focus on the root window, which in embedding case is the web page.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, that's not good...

Comment 2

[Marco Pesenti Gritti]

Attachment: Do not assume there is ever a window focused

As the comment says that case should really never happen. It seem safe to just
return in that case.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135650 [details] [diff] [review]
Do not assume there is ever a window focused

I'm not qualified to review this -- I know next to nothing about how our focus
stuff works. bryner or aaronl would be much better.

Comment 4

[Aaron Leventhal]

I'm actually not sure. I'll put my r= on it though if bryner says it's okay.

Send me a regular email to get my attention if you need me. I'm not checking
bugzilla mail very often at the moment (I hope to get back more into it soon,
we'll see).

Comment 5

[Aaron Leventhal]

Should we leave it the way it is for the non-embedded case? How much have you
tested for regressions there?

The problem is that seemingly reasonable focus fixes often cause
difficult-to-find regressions that don't get noticed right away.

Comment 6

[Marco Pesenti Gritti]

>Should we leave it the way it is for the non-embedded case?

IHMO there is absolutely no reason for the PresShell to try to focus itself on
page loading. I think that check just risk to obscure real bugs.

>How much have you tested for regressions there?

I tested web page loading and xul dialogs on linux. Unfortunately I dont have a
win/mac box, so I cant test there.

Web page loading
----------------

The code inside the if is reached, as expected, only on window creation. Once
there is a focused widget, I couldnt find any way to trigger it again.

I tested 3 cases:

- Homepage blank. The focus is in the location entry as expected
- Web page (www.ximian.com) as homepage. The focus is on the page as expected.
- Web page do a .focus on loading (google.com). The focus is in the search entry
as expected.

XUL Dialogs
-----------

Some dialogs does not trigger the if content at all.
I could find two dialogs that does:

- Page setup dialog. Focus is on Portrait as expected
- Prefs dialog. Focus is on navigator category as expected

Page setup dialog has this hack:

  // Give initial focus to the orientation radio group.
  // Done on a timeout due to to bug 103197.
  setTimeout( function() { gDialog.orientation.focus(); }, 0 );

Even disabling it the PresShell if is called but doesnt seem to actually work
around the problem. In fact the focus is given to the window by the native
widget implementation (so you get at least keyboard working ...). But if I
disable the GOTFOCUS event emitted by the native impl, keyboard doesnt work anymore.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 135650 [details] [diff] [review]
Do not assume there is ever a window focused

It still doesn't make sense to me that in this case the focus controller's
focused window is null.  It should be tracking the most recently focused
window.  How does it become null, exactly?

Comment 8

[Marco Pesenti Gritti]

>It should be tracking the most recently focused
>window.

It's null when no window has been ever focused.

In epiphany, for example, we focus the location entry when opening a new window.
When the user open an url no mozilla window has been focused, so there is not
most recently focused window.

After the discussion on 226708 I'm not really convinced it's worth to risk this
anyway. My opinion is that all embedding clients should use the
nsIWebBrowserFocus api until we have a better alternative. Using that api the
problem is not exposed because the root dom window is not marked active until
mozilla widget get focus.

  PRBool active;
  aFocusController->GetActive(&active);
  if (active)
    ourWin->Focus();

Because of the if (active) check the focus will not be grabbed on the embed widget.

So if you are ok we can close this one.

Comment 9

[Marco Pesenti Gritti]

Closing.