Closed Bug 57841 Opened 24 years ago Closed 22 years ago

window.focus and window.blur not functioning correctly

Categories

(Core :: DOM: Core & HTML, defect, P3)

x86
Windows NT
defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: wcd, Assigned: danm.moz)

References

()

Details

(Keywords: dom0, topembed+, Whiteboard: [adt2])

Attachments

(5 files, 4 obsolete files)

window.blur fails to hide the window.  Frequently window.focus will fail to 
bring a pop-up window hiding behind the browser into focus. The latter often 
takes a complex series of actions to precipitate. I'm working on a test harness 
that will reveal it more consistently.

A test harness is available at the URL indicated: The HTML is as follows:

1) Click CREATE
2) Click BLUR - this should hide the pop-up window; it doesn't
3) Click the browser to hide the pop-up window.
4) Click FOCUS - this should reveal the pop-up window; it doesn't some of the 
time.

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta http-equiv="Content-Script-Type" content="text/javascript">
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta http-equiv="Content-Language" content="en-gb">
<base target="_self">
<title>Focus Bug</title>
<SCRIPT type="text/javascript">
<!--
var myWIN;
function Create()
{
 myWIN = window.open
("http://CyberPortals4U.com/NS6b3_Bugs/FocusBug2.htm", "Bug", "height=520,width=
640"); 
}
function Blur()
{
 myWIN.blur();
}
function Focus()
{
 myWIN.focus();
}
// -->
</SCRIPT>
</head>

<body link="#0000FF" vlink="#0000FF" alink="#0000FF">

<a name="TOF"></a>
<table border="0" cellpadding="0" cellspacing="4" width="100%">
   <tr>
      <td width="100%" bgcolor="#CC3300"><font size="4" 
color="#FFFFFF"><b><i>&nbsp; FOCUS BUG</i></b></font></td>
   </tr>
   <tr>
      <td width="100%"><b><font size="4"><a href="javascript: Create
();">CREATE</a></font></b></td>
   </tr>
   <tr>
      <td width="100%"><b><font size="4"><a href="javascript: Blur
();">BLUR</a></font></b></td>
   </tr>
   <tr>
      <td width="100%"><b><font size="4"><a href="javascript: Focus
();">FOCUS</a></font></b></td>
   </tr>
</table>
</body>
</html>
seeing this with win98 2000110204 trunk --> js engine
Assignee: akhil.arora → rogerl
Status: UNCONFIRMED → NEW
Component: Java APIs for DOM → Javascript Engine
Ever confirmed: true
QA Contact: rajendra.pallath → pschwartau
Browser, not engine ---> DOM 0
Assignee: rogerl → jst
Component: Javascript Engine → DOM Level 0
QA Contact: pschwartau → desale
Reassigning to danm.
Assignee: jst → danm
changing summary to not have nsbeta3 in it
Summary: NS6b3: window.focus and window.blur not functioning correctly → window.focus and window.blur not functioning correctly
Target Milestone: --- → Future
note: seems related to bug 53350, bug 62270
Keywords: dom0
window.focus() & window.blur() methods main purposes are getting defeated here.
If it was was working wrong way it was different, but main purpose of methods 
are not working.

Nominating for nsbeta1. Adding keywords mozilla0.9.1, nsbeta1
This is a 4.x compatibility issue.

Any idea who can put some traction on this?
Keywords: 4xp
Build 2001092103 Win95: top.blur() on form button click event does not work.

window.blur() is definitely broken. Still P3 normal future?
Please try the latest simple testcase in bug 101140. You should be able to hide
the window with window.blur() after modifying it accordingly.
Blocks: 104166
a very simple 'test-case' to demonstrate some of the issues with window.blur() 
is the following:  javascript:window.blur()

On both 4.x and IE, the browser window is moved to the 'back' of the window 
z-order while retaining focus...

On mozilla nothing happens...

-- rick
Adding kw from related bugscape bug:
http://bugscape.netscape.com/show_bug.cgi?id=9911
Keywords: edt0.9.4, topembed
Target Milestone: Future → mozilla0.9.7
Some comments:
  (1) When I try Rick's 2001-11-29 23:13 testcase, I get slightly different
results. Navigator 4.x does indeed move the window to the back of the z-order,
but it doesn't retain focus. Focus goes to the now topmost window. (Open two
browser windows, type javascript:blur() in the urlbar of the topmost one, hit
return, type "help me, Spock" and the text goes into the other window's url.) I
personally kind of don't like the idea of a window keeping focus when it's not
on top. Maybe it's my bias, but my Linux box does that to me and makes me a
little nuts. So in the coming patch, I went with just finding another window to
focus, as it seems to me that Nav 4 did.
  (2) I'm unsure what the big deal with this bug is. I think it's expected to
solve some other issues and maybe require an embedding API change. The upcoming
patch requires no API change and implements blur() as did Nav 4, but I think its
capacity to solve other serious problems will be limited to miracles on the
order of growing hair on healthy mice. But maybe I'll be pleasantly surprised.
Keywords: patch
Attached patch do blur the way Navigator 4 do (obsolete) — Splinter Review
Before this patch, the blur implementation focused the next window up the
containment hierarchy. If you're at the top, it does nothing. I scratched my
head a bit and then ripped that all out. This patch teaches Mozilla to
implement nsIWebBrowserChromeFocus (previously implemented only in embedding
clients) and then uses that in DOMWindow.blur.
hey dan,

i think that there's a of problem with the approach that you've taken...

mainly, i 'believe' that the semantics of nsIWebBrowserChromeFocus are supposed
to allow the embeddor to give focus to the next 'piece of chrome' outside of
mozilla.  this does not necessarily imply a different window.

for example, if mozilla were embedded within a 'form', the nsIWebBrowserChrome
notifications should allow the embeddor to give focus to the 'previous' or
'next' form element adjacent to the embedded browser window.  this allows
'seamless' tabbing into and out of embedded mozilla...

this is why i beleive that we will end up whacking some interface to get the
semantics that we require to fix this problem.

also, because nsIWebBrowserChromeFocus is implemented by the embeddor, i believe
that your patch will *only* fix the blur problem in mozilla not in embedded
situations).  at best, calling javascript:blur() will do nothing in other
applications... but at worst, it could end up focusing some other 'windowing
element' within the same embedded window (but outside of mozilla :-()...

does this make sense, or have i been hittin' the crack pipe too much lately?

-- rick
Uh, didn't he add FocusNextWindow to the nsIWebBrowserChrome interface?
Personally I don't see a problem with that.
Attached patch alternate blur implementation (obsolete) — Splinter Review
Alright then, here's another one that uses nsIEmbeddingSiteWindow2 rather
than nsIWebBrowserChromeFocus. Note the "2". The original is frozen, so this
one's yes! a second version. Yay! It looks like the original but has a "blur"
method.
  Note also my changes to nsISupportsImpl.h. I ran into problems while
implementing an object that supports two similar interfaces (not actually
nsIEmbeddingSiteWindow1 and 2, but the same problem applies) that suggested
some additions to the nsISupports declaration macros.
  Better? Worse?
Whiteboard: got a patch. ETA 12/5/01
Dan, you can solve the behavior you're not liking on your Linux box by playing
w/ your window manager settings (one of the top 3 reasons I think Linux is a
better OS than windows).

I just got off the phone w/ rpotts and we both have some good headaches going
about our nice little nest of focus/chrome/geometry interfaces we've cooked up.

Some suggestions:
1. To solve this immediate problem for 0.9.4 customers, let's add a blur method
(alongside its peer setFocus()) on MOZILLA_0_9_4_BRANCH's
nsIEmbeddingSiteWindow.idl which is not frozen (ahhh, literal semantic games,
what fun).
2. Let's leave the problem unsolved on the trunk, but, actively persue a
solution. Vidur and Rick have some ideas about nsIEmbeddingSiteWindow
refactoring (into a new iface name of course, like dan has done here w/ his
latest patch) on the trunk, things like implicitly handling visibility, and
focus. And perhaps some refactoring of nsIEmbeddingSiteWindow, nsIWebBrowserChrome. 
3. If we're cool w/ #1 and #2, let's file a bug on the nsSupportsImpl.h stuff
you uncovered, dan, and treat that as a separate bug that can be fixed
independent of all of this craziness.

As it stands right now, we've criss-crossed a few interfaces (one of which we
froze) nsIEmbeddingSiteWindow (FROZEN on the trunk), nsIWebBrowserChrome, and
nsIWebBrowserChromeFocus. The first two seem to have hands in eachother's cookie
jars.
Whiteboard: got a patch. ETA 12/5/01
  The 0.9.4 branch? Great galloping methuselah! You know I still have bugs with
keywords like nsbeta+. I tend to ignore that field. Seriously? 0.9.4? Sigh.
  Allow me to reiterate and paraphrase by way of understanding. I should apply
my patch to 0.9.4, not the trunk, and without reving nsIEmbeddingSiteWindow
(snicker). Then I should make this bug dependent on a new bug 113662 against
Rick and Vidur for refactoring our window interfaces, and another new bug 113664
for beefing up the nsISupports macros. And then push this bug off on the trunk.
OK then.
  (Did I mention that nsIEmbeddingSiteWindow has been frozen?)
Status: NEW → ASSIGNED
Depends on: 113662, 113664
Target Milestone: mozilla0.9.7 → mozilla0.9.8
yup, good 'ol 0.9.4. 

You've got it all, but, I want to be clear regarding what hits 0.9.4. on 0.9.4,
nsIEmbeddingSiteWindow is not frozen (it's only frozen on the trunk IIRC), so,
let's just add the blur method to it directly (w/ out rev'ing the iface at all).
Then, leave this bug open (doing all the bug dancing you describe) to be solved
differently on the trunk. I think we're saying the same thing and that you're
just taking stabs at me for 0.9.4 still being alive and over the semantics of
whether or not ice is a liquid or a solid ;-) (or a gas.?).
Well, not for being alive, certainly. But I do think it's like resorting to time
travel to solve a bad plot problem, changing an at-the-time unfrozen 0.9.4
interface. If you can't solve a problem with explosives, try grousing, I always say.
Here's the 0.9.4 version. Note I've moved the macros that I put in
nsISupportsImpl.h in the original patch to the one file where I need them for
this 0.9.4 one-off.
hey dan,

your macros NS_IMPL_QUERY_PROXY_BODY et. al. violate the COM/XPCOM assumptions
about QI.  mainly, that any interface pointer that is returned can be
subsequently QI'ed to any other interface that is supported by the object.

that's the whole purpose of aggregation (and why it's difficult ;-))

for the branch (especially since we need it by tomorrow) i don't think it's a
big deal... but for the trunk we should come up with a better way for
nsContentTreeOwner to implement nsIEmbeddingSiteWindow...

-- rick
Comment on attachment 60557 [details] [diff] [review]
0.9.4 branch version of above patch, but without reving nsIEmbeddingSiteWindow

let's get this bug over with :-)

Other than the macros, everything looks fine -- and we can deal with that on
the trunk...
Attachment #60557 - Flags: superreview+
>your macros NS_IMPL_QUERY_PROXY_BODY et. al. violate the COM/XPCOM assumptions
>about QI.  mainly, that any interface pointer that is returned can be
>subsequently QI'ed to any other interface that is supported by the object.

Damn. That was the whole point of the macros. We should talk offline about this;
I thought I'd nailed it.
sr=hyatt, but bryner should review.
hey dan,

you're absolutely right... the macros are correct!  i guess i missed the details
of NS_IMPL_QUERAY_TAIL_PROXYING :-(

all i can say is that it's been a long day :-(  sorry man...

-- rick
approving for 0.9.4 checkin. please add "fixed0.9.4" to the keyword field once
this hits 0.9.4; thanks.
Keywords: edt0.9.4edt0.9.4+
This is probably ok for the branch (r=bryner), but we need to decide moving
forward whether we want to have focus API's on docshell.  We probably want to
avoid having nsGlobalWindow call directly through to nsIEmbeddingSiteWindow (dom
-> embedding seems like a bad dependency).
Keywords: fixed0.9.4
so, just so we have a record of this, why AREN'T you inheriting the new
interface from nsIEmbeddingSiteWindow? I mean, do we really need a whole new
inner object and a second interface? (not to worry, I'll still be reviewing bug
113664, but I'm really curious why you took this approach)

It's valid C++ to declare a class with competing virtual methods

class A {
public:
  virtual int a() = 0;
};
class B {
public:
  virtual int a() = 0;
};
class AB : public A, public B {
public:
  virtual int a() {}
};

That'll work, right? The problem lies in the macros which declare the interfaces.

class AB : public A, public B {
public:
  NS_DECL_A
  NS_DECL_B
};

expands to

class AB : public A, public B {
public:
  virtual int a();
  virtual int a();
};

or something like that. This is one of those things that makes a compiler go
"hmmm." So strictly speaking, the nsIEmbeddingSiteWindow / nsIBaseWindow issue
is probably only with the declaration macros.
No I understand that, I'm just curious why you didn't make the new interface
derive from the old interface, then you'd share all the old interface methods.

i.e.

interface nsIEmbeddingSiteWindow : nsISupports {
...
};

interface nsIEmbeddingSiteWindow2 : nsIEmbeddingSiteWindow {
  void blurWindow();
};

then you wouldn't need the overhead of the inner object, etc. 
Oh. Yeah, my second patch in this bug wants to be rewritten to do just as you
say. nsIEmbeddingSiteWindow2 can be made without all this ruckus. This patch
needs the nsISupports macro additions not so much because of the new version of
nsIEmbeddingSiteWindow but because nsContentTreeOwner has to support both that
and nsIBaseWindow. I'm positioning the macros as an aid to interface versioning,
but they're also useful for general interface collisioning, as happened here.
Attached patch updated blur implementation (obsolete) — Splinter Review
Updated trunk version of the like-Navigator-4 blur implementation. The
nsISupports macro additions have been moved to bug 113664. The only real issue
with this patch is whether we want to put focus methods on the docshell
interface. (See Brian's comment 2001-12-06 12:03).

Brian, I just need a way to get from the DOM window to the application's window
code. Via the docshell is the best way I could think of. I'm totally willing to
take some other path if you can think of one.
Attachment #60446 - Attachment is obsolete: true
This should make Brian happier. There's a precedent in GlobalWindow for
skipping past the docshell straight to its treeowner, and for #including
embedding .h files (not necessarily bad but ... anyway we're already there). So
this patch is much like the previous (now obsolete) one except that it leaves
nsIDocShell.idl unchanged. So no new focus-oriented methods appear on the
docshell. Better?
Attachment #62778 - Attachment is obsolete: true
Comment on attachment 63431 [details] [diff] [review]
implement blur without touching docshell.idl

r=bryner
Attachment #63431 - Flags: review+
also fixed on trunk now.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
This patch fixes the Blur() function in an embedding scenario. 

However, it ends up treating Focus()
[http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1844]
and Blur() differently. 
Blur()
[http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1878]
calls the DocShell that ends up calling the nsIEmbeddingSiteWindow::Blur.
Focus() just calls widget->SetFocus
[http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1865]
which walks up the window tree and activates the topmost (aka app window)
[http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#1890]. 
This fails in the embedding case because the app window is already active, what
needs to be activated is the window hosting
the browser, at the same level as Blur() is being called. 

The correct browser window is focused, but its frame is not activated.

Can a Focus method be added to nsIEmbeddingSiteWindow ?
Reopening per Gustavo's comments....focus is still busted.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Removing Fixed KW fixed 094 per comments #37 & #38
Keywords: fixed0.9.4
Will plus when fix is in hand
Keywords: edt0.9.4+edt0.9.4
What is the ETA for this fix?
I understand Gustavo's comment in a general way, but without having seen the
problem directly, I'm uncertain exactly what needs doing. It sounds like we
just need to give nsIEmbeddingSiteWindow(2) a chance to hook in at the
beginning of nsGlobalWindow's Focus implementation.

We can't defer completely to a top-level window's implementation, since focus
applies to a particular document (and there we are, already in the correct
document). So I'm thinking that keeping nsGlobalWindow's Focus method unaltered
except for an early callout to nsIEmbeddingSiteWindow(2) is the solution. An
uncomfortable dissimilarity still remains between nsGlobalWindow's Focus and
Blur methods. But I think that's because Focus is naturally more specific; the
methods aren't identical twins.

Good?
Oh ... we're also talking 0.94 branch here too, aren't we? Yikes. Well,
equivalent thing on 0.94, then.
Attachment #60274 - Attachment is obsolete: true
:-), yea, remember, 0.9.4 has an unfrozen nsIEmbeddingSiteWindow.
Dan, In looking at your patch for comment 42, I believe a similar patch for the
0.9.4 branch would do the trick. 

It is important that the current functionality in focus stays: It should still
restore the app window (if iconisized), make the top most window focused, etc.

We just need the extra notification so that the MDI frame of the browser window
can be activated and brought to the top of all other MDI frame windows.
Comment on attachment 64592 [details] [diff] [review]
add nsIEmbeddingSiteWindow2::Focus

r=saari
Attachment #64592 - Flags: review+
Alright, here's what I actually want to check in. Same as before, but I decided
'twas better to use the somewhat oddly named SetFocus method aready on
nsIEmbeddingSiteWindow than to add nsIESW2::Focus and have two similarly named,
similar methods on the same interface. That means I had to make some objects
support both interfaces (no problem, just interface gruntwork) and I'm pretty
sure I want to gut the implementation of SetFocus already on that interface (I
had added it for good measure when I made the aggregated nsIESW2
implementation. See comments in the method for why I felt it best to rescind
it.)
Attachment #64592 - Attachment is obsolete: true
Like above, but the 0.9.4 version. And while I was there, !HEY! has no one
noticed that the (*#% 0.9.4 branch doesn't even build any more? Not if you have
LDAP turned on, anyway. Had to modify two makefiles to get it to (*&#%(*%#
build, for petesake.
Patch (id=64942) looks good. Can you check it in to the branch ASAP?
I'd like to. Not getting much joy in the review arena, though.
Comment on attachment 64942 [details] [diff] [review]
DOMwindow::Focus calls embeddingwindow->focus (0.9.4 branch)

okay, r=saari
Attachment #64942 - Flags: review+
Ah, guilt. Thanks, saari. And Gustavo for verifying the patch's applicability.
(Oh, and I'm plussing the edt0.9.4 keyword -- it was Valeski who approved the
checkin to the 0.9.4 branch.)
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
Re-opening this one for review :( 

I show the blur test still failing in all mfcembed builds, here is a summary: 
Blur test fails in:
proprietary embed client, build from 4/08 using 3/27 pull. (see bugscape bug 9911)
mfcembed 03/27 099
mfcembed 03/15 094
mfcembed 3/25 trunk)
Blur test sucessful in: 
NS 03/27 099 
Mozilla 03/25 trunk
Steps to repro
1.launch client and navigate to
http://browsertest.web.aol.com/tests/javascript/javascpt/
2. click 'blur and focus method' link under the 'windows' section
3. click blur. 
Result: Pop up remains in front of other windows
Expected result: pop should move behind other windows
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
cc'ing chak 
looks like this went into 0.9.4, but, not the trunk way back when. can we get
this onto the trunk?
Keywords: topembedtopembed+
Target Milestone: mozilla0.9.8 → mozilla1.0
Jud: the patch in this bug allows DOMWindow::Blur to bubble up to the parent
window in MDI apps. That's checked and functional in both in 0.94 and on the
trunk. The problem with mfcEmbed noted in comment 53 is that a full
implementation of the Blur method requires help from the embedding app, which
mfcEmbed doesn't yet provide. This is strictly an issue with mfcEmbed, not with
the blur mechanism in general. Off to a new bug 137640 with you.
Status: REOPENED → RESOLVED
Closed: 23 years ago22 years ago
Resolution: --- → FIXED
I've opened http://bugzilla.mozilla.org/show_bug.cgi?id=137785 in response to
Dan's last comment. Thanks for clarification.
I'm seeing this in the trunk and mozilla 1.0.0:

http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#2344

calls, uses treeOwner to QI for the nsIEmbeddingSiteWindow2, in the embedding
case, treeOwner is implemented by
http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp#135

which does not support nsIEmbeddingSiteWindow or nsIEmbeddingSiteWindow2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Gustavo: right you are. This patch will fix that without changing
Mozilla-the-app. It requires that the nsIWebBrowserChrome object sent to
nsDocShellTreeOwner's SetWebBrowserChrome() method support
nsIEmbeddingSiteWindow2. This is as it should be; at some point the
implementation of this method has to defer to the app. This patch just provides
the hook so the app isn't cut out of the picture.
Comment on attachment 79540 [details] [diff] [review]
Blur() can find the necessary interface for embedded apps, too

sr=scc
Attachment #79540 - Flags: superreview+
Attachment #79540 - Flags: review+
Looks like Jud already reviewed, but r=jst too in any case...
Keywords: adt1.0.0
Attachment #79540 - Flags: approval+
Please check this into the trunk and get this tested and update the bug with the
results.  Will this only affect embedded apps or will this affect the client as
well?
Whiteboard: [Need Impact] [ETA Needed]
adt2/nsbeta1+ per ADT triage.
Keywords: nsbeta1+
Whiteboard: [Need Impact] [ETA Needed] → [adt2] [ETA Needed]
Checked in to trunk just now. I gather from comment 62 that we don't want this
on the 1.0 branch yet? As for testing, well, I've tested my build :). The patch
will *affect* Mozilla, but not in any visible way. GetInterface of
nsIEmbeddingSiteWindow2 will find the same object that QueryInterface did, so it
still uses the same implementation. (Though as I mentioned above, prior to this
patch it was not hooked up at all for embedded apps.)

The patch affects the ability to find the correct object to implement blur. Any
failure should manifest itself merely as a failure to blur. Jennifer's test in
comment 53 is a fine test (which of course Mozilla still passes).
adding adt1.0.0+.  Please check into the branch as soon as possible and add the
fixed 1.0.0 keyword.
Keywords: adt1.0.0adt1.0.0+
fixed trunk and 1.0 branch.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED
Whiteboard: [adt2] [ETA Needed] → [adt2]
Build ID 20020419 branch and trunk on winxp

Provided URL is not valid any more.
Testcase passed by creating a copy of a Testcase on local webserver and per
comment #1 

---------
As per Comment #10 from rick

>a very simple 'test-case' to demonstrate some of the issues with window.blur() 
>is the following:  javascript:window.blur()
>On both Open two brwoser windows window1 and window2, the browser window is
>moved to the 'back' of the window z-order while retaining focus...

rick: is this suppose to fix ..?

These are the findings from testing:
open two browser windows lets call them window1 and window2
when javascript:window.blur() is executed in window2, 
window2 goes behind of window1 in z-order on both 4.x and IE.

On today&#8217;s build window2 will not goes behind window1
when javascript:window.blur() is executed in window2, however
window1 moves to the front of window 2 but only for a second. Window 2 gets back
to the front immediately.
focus blur events working as designed per
http://browsertest.web.aol.com/tests/javascript/javascpt/testpgs/window/blurfocus
test case.
Tested with testcase at http://bubblegum/desale/testing/window-focus.html

verified1.0.0 on 05-22-18-1.0.0 & also Verified on trunk on 05-23-08-trunk.
Status: RESOLVED → VERIFIED
Keywords: verified1.0.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: