Closed Bug 305032 Opened 19 years ago Closed 18 years ago

[splitwindow] edit commands not available to subsequent instances on startup (cut, copy, paste, select all)

Categories

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

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: scratchfury, Assigned: mrbkap)

References

Details

(Keywords: fixed1.8, regression)

Attachments

(12 files, 2 obsolete files)

1.75 KB, text/plain
Details
2.55 KB, text/plain
Details
2.27 KB, text/plain
Details
1.29 KB, patch
Details | Diff | Splinter Review
4.57 KB, patch
Details | Diff | Splinter Review
2.45 KB, text/plain
Details
4.59 KB, text/plain
Details
2.40 KB, patch
bryner
: superreview+
Details | Diff | Splinter Review
5.93 KB, patch
bryner
: superreview+
Details | Diff | Splinter Review
5.77 KB, patch
Details | Diff | Splinter Review
930 bytes, patch
bryner
: superreview+
Details | Diff | Splinter Review
10.96 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050816 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050816 Firefox/1.0+

When I open up Deer Park Alpha 2 and open up another instance, I am unable to
use any edit commands such as copy, paste, cut, delete, and select all. If I
change the focus to another application and go back, all the commands now work.
 It is only on subsequent instances.


Reproducible: Always

Steps to Reproduce:
1.Open an instance of Deer Park Alpha 2.
2.Open up another instance of Deer Park Alpha 2.
3.Try to perform any edit commands.

Actual Results:  
No edit commands could be used through menus or keyboard shortcuts.

Expected Results:  
Edit functions should be available at startup.
Keywords: regression
Depends on: 258285
Is it certain that this is a regression from bug 258285?
Could be bug 306235. Is this still broken on trunk?

What do you mean open another instance?
- Do you have MOZ_NO_REMOTE set?
- Are you minimized at the time?
- Are you using Ctrl+N or clicking on a desktop icon?
I can reproduce it on Windows XP clicking a shortcut icon on the desktop:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050901
Firefox/1.6a1
(I have about:blank as home page if it matters.)
The problem does not occur when I use CTRL+N.

It does not occur in SeaMonkey 2005-08-25-05 trunk Windows XP.
(In reply to comment #2)

> - Do you have MOZ_NO_REMOTE set?
I don't know where to set that, so I'll say no.

> - Are you minimized at the time?
No.

> - Are you using Ctrl+N or clicking on a desktop icon?
I'm clicking on a desktop icon or the Quick Launch toolbar.  It doesn't happen
with a new window ever.  Also, I've discovered that new tabs will have the same
problem until I click on another window to fix it.
 

Attached file Trace 1
This is basically the same problem as in bug 306076 -
we do the UpdateCommands() to early for it to have an effect.

Here's a trace (without other changes to the code) that I hope you can follow.
As you can see we don't get any further than nsGlobalWindow::UpdateCommands()
because we have no XUL doc to dispatch to...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file Trace 2
... ok so let's return an ERROR for that and check the result with NS_SUCCEEDED

in the focus controller before setting mNeedUpdateCommands=PR_FALSE.
That delays it long enough for the XUL doc be created but it still doesn't
work...
Attached file Trace 3
... then I made nsXULCommandDispatcher::UpdateCommands() return an ERROR in
case we don't fire at least one DOM handler, which delayed it even further ...
("Firing HandleDOMEvent" near the end) - but it still doesn't work ...
It was just an attempt to find a somewhat general solution to the problem but
it doesn't seem to work...

What worries me with all these traces is that they all end with:

[034466B0] SuppressFocus incremented to 1. The reason is
nsPresContext::EnsureVisible Suppression.
[034466B0] SuppressFocus incremented to 2. The reason is Activation Suppression.
[034466B0] SetFocusedWindow mCurrentWindow=03446520 win=03446520 (0 -> 0)
[034466B0] SuppressFocus decremented to 1. The reason is PresShell suppression
on Web page loads.

that is we don't unsuppress the focus handler for the new window!
(I cut the last trace a bit short so you don't see it there, but it's the same)
Is this a trunk bug only? I still can't duplicate it.

Can people supply the exact keystrokes/mouseclicks necessary to repro?
Attached patch diffSplinter Review
FWIW, if I force unsuppression in the pres shell then it works...
(also with an unchanged nsFocusController)
I'm not suggesting we should do this of course.
(In reply to comment #11)
> Is this a trunk bug only? I still can't duplicate it.

I haven't tested branch builds.  This is how I reproduce it:

1. create a shortcut to firefox on the desktop
2. start firefox
3. double-click the shortcut (a new window comes up, with kbd focus in urlbar)
4. type some text then CTRL+A  => nothing is selected
The nsWebShellWindow.cpp part of the checkin for bug 258285 was supposed to fix
the problem where focus suppression gets stuck on after launching a new Firefox
from the OS. I had to back that part of the fix out yesterday (on 8/31/05)
because it regressed bug 306235.

This does sound very similar to bug 306235.
Attached patch diff2Splinter Review
This works... although I can't explain exactly why yet though ;-)
If we suppress focus even in case the window is null then
we get very different behaviour during startup.
(In reply to comment #11)
> Is this a trunk bug only? I still can't duplicate it.
Yeah, trunk bug only.  The bug is not in the branch.

I wonder if it is a regression. Is this affected by recent nsWebShellWindow changes?
(In reply to comment #18)
> Yeah, trunk bug only.  The bug is not in the branch.

Hmm, we can't really know for sure until bug 306235 is checked in on the branch.
Yep, I rolled back to 1.427 of nsWebShellWindow.cpp and the bug disappeared, so
once bug 306235 goes in this will most likely also be a branch bug.
Flags: blocking1.8b4?
I'm not going to checkj in bug 306235 if it's going to cause a regression. Let's
resolve this. I'll note in bug 306235 that it can't go in until we figure out
this bug.
Flags: blocking1.8b4?
I'm confused about one thing.

This bug was filed for build Gecko/20050816 but the nsWebShellWindow change
should have fixed it in the next build.

So why was it marked as dependent on bug 258285? That's actually the bug that
would have fixed this, then we broke it again yesterday by backing out the
nsWebShellWindow part of the fix.

Maybe it's just later at night but I guess I'm missing something.
Ironically, Mats and I have both found that the nsWebShellWindow changes in bug
258285 should have fixed this, not broken it -- at least according to the steps
used in comment 13, with about:blank as your home page.

On the other hand, those changes to break bug 306235, and possibly the
reporter's original steps for this bug which I can't reproduce yet (what was the
home page set to)?

One way or another, the nsWebShellWindow changes seem to fix some things and
break others. At the moment we have to choose what we want to break.
> On the other hand, those changes to break bug 306235, and possibly the
> reporter's original steps for this bug which I can't reproduce yet (what was the
> home page set to)?

Basically, run firefox.exe and then run it again through a shortcut, the command
prompt, the Run command, or clicking on the file in Explorer.  The second window
that pops up will have the bug.  You have to stay in the second window because
as soon as you change the focus to another window and come back again, the bug
is gone.

My homepage is http://www.mozilla.org/projects/deerpark/ but changing to
anything else, including about:blank, doesn't fix the bug.
Here is my timeline for the bug:
Trunk builds 8/16 and earlier (when you reported it): broken
Trunk builds 8/17 through 8/31 - fixed
Trunk builds 9/1 through now: broken

Branch builds 8/17 and earlier: broken
Trunk builds 8/17 through now: fixed

So the timing of this bug filed made it look like it was a regression of bug
258285 when in fact that fixed it. It was only a regression bug once we backed
out the nsWebShellWindow.cpp part of bug 258285, in order to fix bug 306235. Odd
but true.
No longer depends on: 258285
Depends on: 306235
Assignee: nobody → aaronleventhal
Status: NEW → ASSIGNED
Attachment #194668 - Flags: superreview?(bryner)
Attachment #194668 - Flags: review?(mats.palmgren)
(In reply to comment #26)
> Here is my timeline for the bug:
> Trunk builds 8/16 and earlier (when you reported it): broken
> Trunk builds 8/17 through 8/31 - fixed
> Trunk builds 9/1 through now: broken

I downloaded most of the builds and can confirm that this is the case.


> Branch builds 8/17 and earlier: broken
> Trunk builds 8/17 through now: fixed

I think you meant to say:
 Branch builds 8/16 and earlier: broken
 Branch builds 8/17 through now: fixed

which I also tested and can confirm.
Blocks: 306924
Comment on attachment 194668 [details] [diff] [review]
Smaller nsWebShellWindow.cpp change than original bug 258285, which still fixes this bug but does not cause bug 306235

Sorry Aaron, I think we need to take a step back and address
all the focus regressions at once. See my new patch in bug 306076.

If that patch doesn't work for some reason then I think we
should consider backing out the nsFocusController and
nsEventStateManager to the state they had before bug 249136.
Attachment #194668 - Flags: review?(mats.palmgren)
Comment on attachment 194668 [details] [diff] [review]
Smaller nsWebShellWindow.cpp change than original bug 258285, which still fixes this bug but does not cause bug 306235

Mats, for now we want the smallest change that will fix each set of problems.
We need to keep them separate so we can more easily follow any potential
regressions.

This fix is very small. I suggest we take it.

If we want to make a more ambitious set of focus fixes in the trunk, we should
do it after 1.5 ships. For now let's just solve our regressions.

I have patches to fix each focus regression out there on the branch.
Attachment #194668 - Flags: review?(mats.palmgren)
Flags: blocking1.8b4?
Flags: blocking1.8b4? → blocking1.8b4+
Attachment #194668 - Flags: superreview?(bryner) → superreview+
1) Reverse esm reordering of NS_GOTFOCUS handling from bug 258285,
2) Reverse webshellwindow changes from bug 258285,
3) Always update commands in when focus suppression turned off if
mCurrentElement
4) Remove built in limitation in mail compose code that prevents a 2nd command
update after doc has loaded and knows it's an editor
Attachment #195040 - Attachment is obsolete: true
Attachment #195055 - Flags: superreview?(bryner)
Attachment #195055 - Flags: review?(bryner)
Attachment #195055 - Flags: superreview?(bryner)
Attachment #195055 - Flags: superreview+
Attachment #195055 - Flags: review?(bryner)
Attachment #195055 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
I can confirm that this patch fixes the compose window and the keybord
minimize/maximizing issues in Thunderbird. 
Comment on attachment 195055 [details] [diff] [review]
Changes explained in comment

Aaron, can you check this patch (or the updated one you just posted) into the
branch? I'm going to respin bits for Firefox and Thunderbird on the branch once
this is in so  QA can start using bits with this patch ASAP. Thanks.
Attachment #195055 - Flags: approval1.8b4+
Do you want me to wait so we can see how it goes on trunk? I'm also waiting to
make sure the perf numbers don't change -- I don't think they will.
Attachment #195064 - Flags: review?
(In reply to comment #36)
> Do you want me to wait so we can see how it goes on trunk? I'm also waiting to
> make sure the perf numbers don't change -- I don't think they will.

We know we have to take this fix tonight for the beta and we thought it best if
we got more eyes on it by respinning the branch builds for QA since that's where
most of us are looking. I've got my finger on the respin trigger :)
Keywords: fixed1.8
I restarted the builds so we will get new nightlies with this fix. 
This bug managed to stay in. Looking to see what happened to the fix for it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: fixed1.8
I don't think this is a regression. I believe it has been broken for a very long
time. A recent checkins temporarily fixed it but it was not the right fix for it.

I am looking at a proper fix but we should not treat this as a regression unless
someone can find when it worked prior to 8/15.
Keywords: regression
Today's respin is still broken as Aaron mentioned. It does works for me with my
Deer Park Alpha 2 build though: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8b3) Gecko/20050712 Firefox/1.0+

So this regressed (was fixed and came back again) sometime after that, and
considering this is a Beta 1 following Alpha 2 (which was the last "unofficial"
release), this should be considered a regression.  Whether we should consider it
a blocker for Beta 1, I don't know...probably not this late in the game. 

Although we have a timeline, there still seems to be confusion about what might
have introduced this again and how we plan to fix this particular issue (I have
already verified that Aaron's recent checkin fixed bug 306235).
> Branch builds 8/16 and earlier: broken

Okay, can someone help get us a regression window and find out when it broke
prior to 8/16 ?

Meanwhile I'm looking at a fix. But I need to know what broke it.

Keywords: regression
Whiteboard: [needs review mats]
I think my splitwindow hunch might be correct.

7/30: works
7/31: broken
I wonder if the ourGlobal for NS_LOSTFOCUS and NS_DEACTIVATE should also use the
inner window, since they are used to fire DOM blur events. What might have
broken with changing those to use the outer window?
Attachment #194668 - Flags: review?(mats.palmgren)
Comment on attachment 195113 [details] [diff] [review]
Before splitwindow we used to fire the DOM focus event on the inner window

r=jst, but I honestly don't understand our focus code well enough to know the
answer to your question in comment #45 :(
Attachment #195113 - Flags: review?(jst) → review+
See also bug 307355 for the fact that arrow keys don't work. That's also
originally a splitwindow regression which we temporarily fixed in an incorrect way.
Summary: edit commands not available to subsequent instances on startup → [splitwindow] edit commands not available to subsequent instances on startup
(In reply to comment #46)
> (From update of attachment 195113 [details] [diff] [review] [edit])
> r=jst, but I honestly don't understand our focus code well enough to know the
> answer to your question in comment #45 :(
> 

Why did we make the changes in esm for splitwindow?
What will we break if we reverse those changes?
We need to understand that because those changes caused this bug and I also
believe bug 307355.
So we need to either justify those changes or remove them.
Assignee: aaronleventhal → jst
Status: REOPENED → NEW
Blocks: splitwindows
Attachment #195113 - Flags: superreview?(bryner) → superreview+
Comment on attachment 195113 [details] [diff] [review]
Before splitwindow we used to fire the DOM focus event on the inner window

Bz thinks this is not the right fix, and that:
- focus should always use the outher widnow (everything except js compilation
and execution should be using the outer window)
- That problem could be that the outer window is null or that something in
nsGlobalWindow is busted
- If we revert the esm changes in general we'll break esm stuff that compares
window pointers
- HandleDOMEvent does forward to the inner
- If it happens that the current inner for this doc is no longer or not yet the
document's script global, that might be an issue here
not going to make b4. We want to get this trio of bugs for b5 though. We'll need
to release note this group of bugs.
Flags: blocking1.8b5+
Flags: blocking1.8b4-
Flags: blocking1.8b4+
I'm still seeing this bug with Fx 1.4 branch: Mozilla/5.0 (Windows; U; Windows
NT 5.1; en-US; rv:1.8b4) Gecko/20050907 Firefox/1.4

Clicking the desktop icon to open another Fx browser window causes the Edit |
Copy  to not be available with text selected.  Switching to the first window
then back to the second window corrects the problem for that session/window. 
Each new window opened via the desktop icon will again exhibit the problem.
This worksforme with a branch build from MOZ_CO_DATE="Thu Sep 1 03:00:00 CDT
2005" on Linux and with a trunk build from Sep 2.

Do I need to be looking at a more recent build?
The bug is very likely Windows-only, given what Aaron and I observed with the 
ordering of NS_GOTFOCUS and NS_ACTIVATE.
(In reply to comment #52)
> This worksforme with a branch build from MOZ_CO_DATE="Thu Sep 1 03:00:00 CDT
> 2005" on Linux and with a trunk build from Sep 2.
> 
> Do I need to be looking at a more recent build?

Before splitwindow checkin (7/30): fixed
After splitwindow checkin (7/31 - 8/16): broken
Branch builds 8/17 - 9/6: fixed, but that regressed other bugs so we backed out
that change to deal with the core regression which is splitwindow related
9/7: broken
BTW: I just tested the "Before splitwindow we used to fire the DOM focus event
on the inner window" patch locally and this didn't fix the problem...
The checkin for this on Sept 6 caused Tp regression bug 307741.  Was that
checked into branch too?
Depends on: 307741
Whiteboard: [needs review mats]
*** Bug 307991 has been marked as a duplicate of this bug. ***
ahh... so opening a second instance is what caused this... bug voted on!
Blocks: 308869, 308909, 309179
Summary: [splitwindow] edit commands not available to subsequent instances on startup → [splitwindow] edit commands not available to subsequent instances on startup (cut, copy, paste, select all)
This bothers me. A lot. So I guess I get to debug it.
Assignee: jst → mrbkap
No longer blocks: 308909
*** Bug 308909 has been marked as a duplicate of this bug. ***
*** Bug 310029 has been marked as a duplicate of this bug. ***
And when you thought you'd seen it all...

With a lot of help from bryner, bz and I tracked this down to what seems to be
the root cause of the problem. The cause of the focus problems is that we're
getting an WM_SETFOCUS (= NS_GOTFOCUS) without a corresponding WM_ACTIVATE (=
NS_ACTIVATE). A bunch of debugging last week, showed that the reason for this
confusion was that when we were tearing down the about:blank content viewer, its
root view's widget (window) had Window's focus. Thus, when we destroyed it,
Windows looked for a new window to give focus to and decided on the outer
window, sending it a WM_SETFOCUS, but no WM_ACTIVATE (since it was already
activated).

In pre-splitwindow builds, we didn't have this problem because by the time the
about:blank content viewer was going away, the new content already had focus
(thanks to CheckForFocus called from PresShell::UnsupressAndInvalidate()). After
splitwindow, however, CheckForFocus was terminating without activating the new
content viewer. This ended up being because there was a chrome window stealing
focus before anything else could have a say, messing up the logic in CheckForFocus.

More investigation turned up that nsGlobalWindow::Activate was the culprit here,
activating the chrome window in post-splitwindow builds, where it was erroring
out early pre-splitwindow. Finally, we arrive at the root cause of the problem:
pre-splitwindow, there was no presshell when nsGlobalWindow::Activate was called
whereas post-splitwindow builds did have a presshell. The culprit is the call to
nsGlobalWindow::GetDocument(nsIDOMDocument**) in nsWindowSH::NewResolve, which
creates an about:blank document too early, allowing the chrome window to take focus

We need to figure out who gets to fix/work around this problem (whether it's
focus, widget, or DOM).
Severity: minor → normal
So to be precise, we're calling NewResolve because we create a new array object
on the new window when attaching window.arguments.  Perhaps we can avoid doing
that somehow?
Attached patch Like so, say (obsolete) — Splinter Review
mrbkap, does this make things happy?
Attachment #197586 - Attachment is obsolete: true
Comment on attachment 197592 [details] [diff] [review]
Actually compiling

>Index: dom/src/base/nsGlobalWindow.cpp
>@@ -1634,38 +1634,62 @@ nsGlobalWindow::SetScriptsEnabled(PRBool
>+  // Resolving it for window.arguments on the outer window should be
>+  // fine, I think.

This should be fine, since, aiui, the outer window is still in the scope chain
*below* all of the standard classes.

r=mrbkap
Attachment #197592 - Flags: superreview?(jst)
Attachment #197592 - Flags: review+
Comment on attachment 197592 [details] [diff] [review]
Actually compiling

Looks good. Great work finding this guys! sr=jst
Attachment #197592 - Flags: superreview?(jst) → superreview+
Component: General → DOM
Flags: review+
Product: Firefox → Core
Version: unspecified → Trunk
Comment on attachment 197592 [details] [diff] [review]
Actually compiling

Requesting 1.8b5 approval.  This should fix at least some of the focus
regressions we've seen on Windows, possibly all.
Attachment #197592 - Flags: approval1.8b5?
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Blocks: 307375
Blocks: 307355
Attachment #197592 - Flags: approval1.8b5? → approval1.8b5+
Comment on attachment 197592 [details] [diff] [review]
Actually compiling

Restoring lost r+.
Attachment #197592 - Flags: review+
*** Bug 307355 has been marked as a duplicate of this bug. ***
*** Bug 307375 has been marked as a duplicate of this bug. ***
Fix checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
*** Bug 310313 has been marked as a duplicate of this bug. ***
*** Bug 310423 has been marked as a duplicate of this bug. ***
Using Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8b5) Gecko/20050928
Firefox/1.4,

still seeing the problem. I have a text area on a forum, and I can’t use the
cursors and copy/paste controls at all. Typing works (but it did before, too).


~Grauw
(In reply to comment #76)
> Using Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8b5) Gecko/20050928
> Firefox/1.4,
> 
> still seeing the problem. I have a text area on a forum, and I can’t use the
> cursors and copy/paste controls at all. Typing works (but it did before, too).
> 

The fix to the branch was checked in on 9/28. You need a newer build to see it.
Confirm that it don't fix the problem with Mozilla/5.0 (Windows; U; Windows NT
5.1; en-US; rv:1.9a1) Gecko/20050928 Firefox/1.6a1

Here a quick exemple:
1)Install Adblock
2)Do a Ctrl+Shift+P to get the preferences
3)Unable to Paste, navigate with the arrow keys.

If you change to another application and go back to Firefox the Paste and
navigation with arrow keys works.
That sounds like a separate bug.  Please file it, mark it as blocking bug  
296639, and cc me on it?
done, filed Bug 310458
*** Bug 310462 has been marked as a duplicate of this bug. ***
(In reply to comment #77)
> The fix to the branch was checked in on 9/28. You need a newer build to see it.

The filedate of the installer of the build I used was 29-9-2005 0:33:00. So, it
was a build from after the checkin.


~Grauw
If you're still seeing a problem with builds from today (started building today,
not made to ftp server today), please file a separate bug and cc me, ok?
*** Bug 310655 has been marked as a duplicate of this bug. ***
*** Bug 310878 has been marked as a duplicate of this bug. ***
That last checkin caused bug 310552....
Depends on: 310552
*** Bug 311078 has been marked as a duplicate of this bug. ***
*** Bug 311368 has been marked as a duplicate of this bug. ***
*** Bug 311682 has been marked as a duplicate of this bug. ***
*** Bug 311804 has been marked as a duplicate of this bug. ***
Depends on: 312003
I don't think the Thunderbird change to MsgComposeCommands.js was ever checked
in on the branch when https://bugzilla.mozilla.org/attachment.cgi?id=195055 was
checked into the branch on 09/07. That's causing Bug 308822.
It looks like the first patch checked in for this bug (by Aaron), caused bug 312227.
This bug is fixed.  I'm certain the recent dupes are dupes of bug 312227, not
this bug.  Bug 312227 has all the same symptoms as this bug, but it is looking
less serious than it really is because all of its dupes are getting eaten by
this bug, and it may not get fixed for 1.5 as a direct result.
*** Bug 309179 has been marked as a duplicate of this bug. ***
No longer blocks: 309179
This bug still exists even in 1.5.0.4 release.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I see this only if about:blank is the home page. FF trunk (Does not happen in suite.)

similarly odd behaviors 
- paste is restored in second instance if one returns to the first instance, paste in address bar, return to second instance, paste fails, but click in address bar and paste works (command edit menu tracks this behavior as well)
- start a third instance, paste fails in address bar in same manner as second window, etc
Guys, this bug is long fixed. If you can trace the regression range of what you're seeing to split window landing, please file a new bug and make it block bug 296639. Thanks.
Status: REOPENED → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: