Closed
Bug 224185
Opened 21 years ago
Closed 21 years ago
PresShell make a wrong assumption about focus (for embedding)
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: mpgritti, Unassigned)
References
Details
Attachments
(1 file)
883 bytes,
patch
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
Yeah, that's not good...
Reporter | ||
Comment 2•21 years ago
|
||
As the comment says that case should really never happen. It seem safe to just return in that case.
Reporter | ||
Updated•21 years ago
|
Attachment #135650 -
Flags: review?(bz-vacation)
Comment 3•21 years ago
|
||
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.
Attachment #135650 -
Flags: superreview?(bryner)
Attachment #135650 -
Flags: review?(bz-vacation)
Attachment #135650 -
Flags: review?(aaronlev5)
Comment 4•21 years ago
|
||
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•21 years ago
|
||
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.
Reporter | ||
Comment 6•21 years ago
|
||
>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•21 years ago
|
||
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?
Reporter | ||
Comment 8•21 years ago
|
||
>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.
Reporter | ||
Updated•21 years ago
|
Attachment #135650 -
Flags: superreview?(bryner)
Attachment #135650 -
Flags: review?(aaronlev5)
Reporter | ||
Comment 9•21 years ago
|
||
Closing.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•