Closed Bug 264235 Opened 20 years ago Closed 20 years ago

context menus broken

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: tracy, Assigned: roc)

References

Details

(Keywords: qawanted, regression, smoketest)

Attachments

(4 files, 3 obsolete files)

try this v2
17.91 KB, patch
Details | Diff | Splinter Review
printf log
2.08 KB, text/plain
Details
show/resize patch
1.22 KB, patch
mikepinkerton
: review+
roc
: superreview+
Details | Diff | Splinter Review
resize check for (0,0) v1.1 (diff -w)
2.16 KB, patch
mikepinkerton
: review+
roc
: superreview+
Details | Diff | Splinter Review 
seen on Firefox Mac trunk build 2004-10-13-07-trunk
(not seeing this on Window or Linux trunk builds from today)

-context click (ctrl+click or right click) on any item

expected results:  the associated context menu appears

tested results: no context menu

perhaps this is in some way related to bug 263992 ?
Problem is also occuring in Mozilla 1.8a5 build 2004100705 for Mac OS X (Should
I report as seperae bug?)
I'll change Product to Browser. Thanks Scott.
Component: Menus → XP Toolkit/Widgets: Menus
Product: Firefox → Browser
If this is happening in 2004-10-07, it's not related to bug 263992.

Can someone with a mac pin down the day this regressed?
Assignee: firefox → nobody
Keywords: qawanted
QA Contact: bugzilla
For me it is working in the build 2004101105 (which I am using right now), and
it is broken in the later builds.
Blocks: 238493
Right, working up to the 11th and then broken on the 12th and 13th on both
Firefox and Mozilla
Sounds like a regression from bug 238493, then.
Assignee: nobody → roc
Component: XP Toolkit/Widgets: Menus → Layout: View Rendering
QA Contact: ian
Also broken on 10/14 Mozilla daily build.
This is also still broken on Firefox Mac trunk build 2004-10-14-07-trunk
*** Bug 264554 has been marked as a duplicate of this bug. ***
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041016
Firefox/0.9.1+

This is still in, but backing out patch 161548 from bug 238493 causes the problem
to vanish.

I will see whether I can narrow the regression down.
O.K. The patch I backed out touched four directories: gfx, view, layout and 
widget.

Re-doing those parts that touched gfx, layout and widget has not caused the bug 
re-appear.

Continuing ...
We know what the problem is. See bug 264245.
I may have narrowed the problem down to 
nsViewManager::ProcessWidgetChanges(nsView* aView) 
and I am certainly in a position to make a patch.

However ...

This bug is "context menus broken" it is marked as Mac OS, and in comment 3,
help was requested.

Bug 264245 is "form popup (combobox) appears in wrong place after scroll",
and appears to need far more and deeper analysis. (Bug 264245 is marked
as blocking this bug).

Maybe this bug is now actually resolved in the sense that work/discussion 
is going on elsewhere. In case submitting a patch to hide the problem
(spackle) might be viewed as an unfriendly act, I shall refrain and 
just watch.



Ben, thanks for the help but the work to fix this is going on in bug 264245. If
you could apply the patch I submitted there (attachment 162372 [details] [diff] [review]) and report on
whether that fixes the problem on Mac, that would be extremely useful!
O.K. I have CVS update'd my tree and applied the patch, waiting for compile ...
I don't think anyone would ever view submitting a patch as an unfriendly act! 
(Though it may get denied review for various reasons, of course.)
Yes. The pop-up menu problem not in after your patch is applied
Well, I've applied the patch with Mozilla, and it crashes at startup.
Config: MacOSX 10.3.5
I have this warning in the console with a debug build:
WARNING: dependent window created without a parent, file nsWindowCreator.cpp,
line 128
With gdb, I have:
Program received signal EXC_BAD_ACCESS, Could not access memory.
0x01287b60 in nsMacWindow::StandardCreate(nsIWidget*, nsRect const&,
nsEventStatus (*)(nsGUIEvent*), nsIDeviceContext*, nsIAppShell*, nsIToolkit*,
nsWidgetInitData*, void*) ()

Try the new patch I just posted in the bug.
No crash this time, but the context menus are still broken, sorry.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041018
*** Bug 264950 has been marked as a duplicate of this bug. ***
Attached patch try this (obsolete) — — Splinter Review 
Try this patch instead --- Thanks!
Attached patch try this v2 — — Splinter Review 
ugh, that won't work ... try this
Attachment #162697 - Attachment is obsolete: true
*** Bug 265245 has been marked as a duplicate of this bug. ***
The patch doesn't work, I'm sorry. The context menus are still missing.
Are you able to debug this for me?

The context menu just doesn't show up at all? Or does it show up in the wrong
place (possibly offscreen)? Can you see it if you activate Expose?
I'll try to do my best.
Expose doesn't show popup windows, so I can't use it to know if the popup is
missing or offscreen.
I see no context menu at all.
Does the Mac have some kind of window system inspection tool that developers can
use to see all the windows that have been created and what their status is?
Not sure if there's an app, there's a DebugPrintWindowList function useable from
gdb. I haven't used it myself though.
Well, I have found that the Quartz Debug application can list the windows, but
the list is static. To refresh you need to switch to Quartz Debug, and doing
that dismiss popup windows.
I hit the assertion.

###!!! ASSERTION: Popups should not be hooked into nsIWidget hierarchy:
'!aInitData || aInitData->mWindowType != eWindowType_popup || !aParent', file
/Volumes/Source/mozilla/libxml/mozilla/widget/src/mac/nsMacWindow.cpp, line 281
Attached file printf log — 
Put some printfs in nsMacWindow.cpp for the trunk with this patch, and compared
against Moz 1.7.  As you can see, they are very different.  I don't know why we
get all those Move calls; I even get them when moving my mouse across the page.
 roc, any idea why those are there?

As for the context menu, it's created in the wrong location and sized to 0,0. 
Then later on it is moved to the correct location (I think), and sized to the
correct size.  Maybe this is causing it to not appear?
Strange, I don't assert. (I have checked that the patch is really applied)
Javier, it looks like it is being positioned and sized correctly. Can you dig
deeper to see where it actually is placed according to Quartz, and why Quartz
isn't displaying it?
Hmm, seems nsMacWindow::Show() isn't being called with the new patch (was called
just fine in 1.7).  Still investigating why.
Sorry, nsMacWindow::Show() is getting called, but none of the Mac funtions to
actually show the window are getting called.  This patch fixed it:

   // we need to make sure we call ::Show/HideWindow() to generate the 
   // necessary activate/deactivate events. Calling ::ShowHide() is
   // not adequate, unless we don't want activation (popups). (pinkerton).
-  if ( bState && !mBounds.IsEmpty() ) {
+  if ( bState ) {
 #if TARGET_CARBON
     if ( mIsSheet && parentWindowRef ) {
         WindowPtr top = GetWindowTop(parentWindowRef);


This line was checked in by pinkerton back in the day, but I saw no reference as
to why the mBounds check was added in the bugs.  Pinkerton, do you remember any
of this?
one of two reasons:
- gecko loves to create and show windows of no size, which on mac show up on the
screen. 
- a bug on early versions of OS X that crashed when you showed a window with no
size.

either are just as likely candidates. what was my checkin comment?
Javier, is it possible to do the WindowShow call when we Resize() to a non-empty
rect?
I will look into why these extra Move and Resizes are happening, but they
*should* work, and if they only fail to work on Mac, then Mac's bound to get
broken again later.
Pinkerton, your comment was "remove special-case activation code for hidden
window, let the OS do it. punts hidden window offscreen on mac so it can be
visible yet of non-zero width/height for osx. bugs 70355/70388."

roc, I don't think we want to show the window during Resize(), since Mozilla
sometimes resizes the window before wanting to display it, and then somewhere
down the line calls Show().  At least, that's what it looks like to me.
Some more printf output:

MOZILLA 1.7:
::StandardCreate() mWindowPtr=0x129a3650
::Show(bState=FALSE) mWindowPtr=0x129a3650 mBounds=0 0 0 0
Set mBounds to 2 2 0 0
Set mBounds to 2 2 187 210
::Resize(aW=192, aH=215) mWindowPtr=0x129a3650
Set mBounds to 0 0 192 215
::Show(bState=TRUE) mWindowPtr=0x129a3650 mBounds=0 0 192 215

TRUNK W/ PATCH:
::StandardCreate() mWindowPtr=0x122a93f0
::Show(bState=FALSE) mWindowPtr=0x122a93f0 mBounds=0 0 0 0
::Resize(aW=0, aH=0) mWindowPtr=0x122a93f0
::Show(bState=TRUE) mWindowPtr=0x122a93f0 mBounds=0 0 0 0
::Resize(aW=192, aH=233) mWindowPtr=0x122a93f0
Set mBounds to 0 0 192 233

So with the new patch, we are getting a Resize to 0,0 before the call to
Show(TRUE), with the actual Resize coming afterwards.
So can you record what the show state is according to Mozilla (whether the last
Show call was PR_TRUE or PR_FALSE), and then in Resize if the size is non-zero
and the show state is PR_TRUE, do WindowShow?
Flags: blocking1.8a5?
Blocks: 264229
roc, I have no doubt that the Mac code can be changed, I'm just wondering why we
would need to.  Why does Resize get called twice now (and out of order), where
previously it was called once with the correct width and height, right before
Show()?
I don't know yet. But the sequence of calls, while suboptimal, is not invalid
... and I just want to get the tree reopened before I start poking around and
changing things again.
This is a patch against the previous patch.  Basically, I created a new member
variable (mShown) that is only set when the window is actually made visible. 
Then in Resize(), I check if the window is supposed to be visible (mVisible ==
TRUE) but hasn't been shown yet (mShown == FALSE).  If that's the case, then I
call Show(TRUE).
Comment on attachment 162930 [details] [diff] [review]
show/resize patch

peterv, what do you think of this?
Attachment #162930 - Flags: review?(peterv)
*** Bug 265653 has been marked as a duplicate of this bug. ***
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041023
Firefox/0.9.1+

After I have done a CVS update, the buggy behaviour is back in. Should it be?

It is very awkward trying to test a browser without toolbar menus, URL
completion et cetera!

Incidentally, I am seeing these messages:
###!!! ASSERTION: couldn't set a property on the window, event handling will
fail: 'err == noErr', file ../../../../src/widget/src/mac/nsMacWindow.cpp, line 503

Is this likely to be significant of anything?

Ben, yes, we're currently waiting on peterv to review an additional fix to get
Mac working properly.
Blocks: 264612
*** Bug 265778 has been marked as a duplicate of this bug. ***
Could someone please review the patch for Bug #264235? It's been waiting a week to be reviewed, and 
that's left Mac users of Mozilla without a new daily build that supports contextual menus since Oct. 12.
i'd like an answer to the question raised in javier's comment #45. why is resize
now called twice? 
I don't know, but there's nothing wrong with Resizing it twice.
Comment on attachment 162930 [details] [diff] [review]
show/resize patch

r=pink
Attachment #162930 - Flags: review?(peterv) → review+
Attachment #162930 - Flags: superreview?(roc)
Comment on attachment 162930 [details] [diff] [review]
show/resize patch

I think there may be a problem if we Resize to (0,0) to hide the window, but I
won't hold this up for that.
Attachment #162930 - Flags: superreview?(roc) → superreview+
Checked in to trunk.  roc, what problem with resizing to (0,0)?  I put an
assertion right before the Show() call in Resize(), and it got hit quite a bit,
but I didn't notice any adverse effects.  I did not check in the assertion.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
I thought that the Mac would barf on a (0,0) sized "visible" popup, so we'd have
to hide it, but if not, great. Thanks!
Well, you didn't really test it because we currently don't resize anything to
(0,0) while it's showing. If something changes to make that happen, this might
break.
Attached patch resize check for (0,0) (obsolete) — — Splinter Review 
Ah, I see what you're saying.  How about this patch then?
Reopening in order to get resize to (0,0) check reviewed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch resize check for (0,0) (diff -w) (obsolete) — — Splinter Review 
It turns out the system call ::SizeWindow will not do anything if width and
height are 0.  So if that's the case, we just hide the window.
Attachment #163853 - Flags: superreview?(roc)
Attachment #163853 - Flags: review?(pinkerton)
Blocks: 265811
Comment on attachment 163853 [details] [diff] [review]
resize check for (0,0) (diff -w)

Thanks Javier. This is great.
Attachment #163853 - Flags: superreview?(roc) → superreview+
Blocks: 266293
Comment on attachment 163853 [details] [diff] [review]
resize check for (0,0) (diff -w)

Pinkerton pointed out a problem with this patch: if we resize a visible window
to (0,0), the call to Show(false) will change the window to not visible
(mVisible == false).  So if we later resize to non-zero, it will not display.
Attachment #163853 - Attachment is obsolete: true
Attachment #163853 - Flags: superreview+
Attachment #163853 - Flags: review?(pinkerton)
Attachment #163758 - Attachment is obsolete: true
Ok, I think this should do it.	This is the short, hacky patch.  A cleaner
version would require me to do something like in gtk2/nsCommonWidget.cpp; but
that's somewhat involved.
Comment on attachment 163862 [details] [diff] [review]
resize check for (0,0) v1.1 (diff -w)

r=pink
Attachment #163862 - Flags: review+
Comment on attachment 163862 [details] [diff] [review]
resize check for (0,0) v1.1 (diff -w)

One last time, roc!
Attachment #163862 - Flags: superreview?(roc)
Attachment #163862 - Flags: superreview?(roc) → superreview+
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Flags: blocking1.8a5?
Javier, does that patch fix the various dependencies of this bug?  That is, is
it safe to start making other viewmanager changes now?  ;)
*** Bug 264229 has been marked as a duplicate of this bug. ***
*** Bug 264612 has been marked as a duplicate of this bug. ***
*** Bug 265811 has been marked as a duplicate of this bug. ***
*** Bug 266293 has been marked as a duplicate of this bug. ***
bz: change viewmanager at will!
Did the fix for this bug cause another bug. I downloaded the Suite nightly for
the Mac (10/29/04) and it was too buggy to use. I experienced numerous quits or
hangs which caused corruption in my profile (fortunately I back up regularly)
and caused the OS to become unstable and once the OS itself froze (usually never
does this). Upon reverting to an earlier stable nightly I had to replace my
profile with my backup before Mozilla would launch. I filed a bug for this issue
in case the fix for this bug is not the cause: bug 266806 "The Mozilla
Application Unexpectedly Quits or Hangs".
Blocks: 266055
Blocks: 266069
(In reply to comment #75)
> Did the fix for this bug cause another bug. I downloaded the Suite nightly for
> the Mac (10/29/04) and it was too buggy to use. I experienced numerous quits or
> hangs which caused corruption in my profile (fortunately I back up regularly)
> and caused the OS to become unstable and once the OS itself froze (usually never
> does this). Upon reverting to an earlier stable nightly I had to replace my
> profile with my backup before Mozilla would launch. I filed a bug for this issue
> in case the fix for this bug is not the cause: bug 266806 "The Mozilla
> Application Unexpectedly Quits or Hangs".

Well, on Mac running OS X 10.3.5 with build (Mozilla/5.0 (Macintosh; U; PPC Mac
OS X Mach-O; en-US; rv:1.8a5) Gecko/20041030) all the bugs seem to be
resolved....i can see the context-sensitive menus, fill in forms with pull-down
menus, and although i've only been using it for a couple of minutes, no major
crash so far........
*** Bug 266131 has been marked as a duplicate of this bug. ***
I also thought all the bugs were fixed. Then I used the app for more than 20 -
30 minutes and the longer I used the application the more unstable it became
until it came to the point that I felt it was safer to revert to a known stable
nightly of 1.8a4 than risk regular crashes and corruption to my profile.

The instability creeps up gradually. At first the application appears stable and
all bugs appear fixed. The longer the application is used the more unstable the
application becomes and the more frequent the crashes appear.

BTW I am using a B/W G3 with MacOS X 10.3.5 if this makes any difference.

I still suspect that the fix for this bug may have created new bugs causing the
application to develope instability over time.
There is no proof that the instability you experienced is caused by this patch.
If you think there is a problem of stability in the recent nightlies, create a
new bug, and use talkback to collect information about the crashes.
verified on Mac trunk build 2004-11-10-06-trunk
Status: RESOLVED → VERIFIED
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: