Last Comment Bug 281709 - Under at least Windows XP, shadows are not drawn under menus on first appearance
: Under at least Windows XP, shadows are not drawn under menus on first appearance
Status: RESOLVED FIXED
: fixed1.8, regression
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Hixie (not reading bugmail)
:
Mentors:
: 282059 (view as bug list)
Depends on: 282359 310483 311146
Blocks: 244366 312563
  Show dependency treegraph
 
Reported: 2005-02-09 13:15 PST by Francois Ingelrest
Modified: 2016-10-20 14:46 PDT (History)
31 users (show)
asa: blocking1.8b3-
mtschrep: blocking1.8b5+
asa: blocking1.8rc1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Random hack #1 to test (3.21 KB, patch)
2005-02-10 20:24 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Random hack #2 to test (3.17 KB, patch)
2005-02-10 20:27 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Image of shadows missing from sub menu and context menu (34.06 KB, image/png)
2005-02-11 07:51 PST, Dan
no flags Details
callstack (12.90 KB, text/plain)
2005-02-11 15:01 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
callstack2 (5.98 KB, text/plain)
2005-02-12 12:05 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Random hack #3 to test (2.41 KB, patch)
2005-02-20 15:30 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Random hack #4 to test (2.17 KB, patch)
2005-02-20 15:33 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Random hack #3 that might compile (4.04 KB, patch)
2005-02-20 19:31 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Random hack #4 that might compile (3.80 KB, patch)
2005-02-20 19:32 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Random hack #5 -- now with Get/SetClassLong (4.04 KB, patch)
2005-02-23 10:32 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Buffer view visibility changes like resizes (1.55 KB, patch)
2005-09-24 10:58 PDT, James Ross
roc: review+
roc: superreview+
Details | Diff | Splinter Review
gray content area (86.10 KB, image/png)
2005-09-29 03:22 PDT, Vidar Haarr (not reading bugmail)
no flags Details
fall back patch for the branch.... (1.17 KB, patch)
2005-10-03 14:54 PDT, Scott MacGregor
no flags Details | Diff | Splinter Review
possible patch (26.25 KB, patch)
2005-10-03 18:20 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Compiling on Windows (26.26 KB, patch)
2005-10-03 21:35 PDT, Blake Kaplan (:mrbkap)
pavlov: review+
darin.moz: superreview-
brendan: superreview+
brendan: approval1.8b5+
Details | Diff | Splinter Review
One possible patch (6.75 KB, patch)
2005-10-19 18:35 PDT, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
roc: superreview+
asa: approval1.8rc1+
Details | Diff | Splinter Review
Same as diff -w (3.67 KB, patch)
2005-10-19 18:36 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
backout of mrbkap's patch (16.14 KB, patch)
2005-10-19 19:01 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Another approach (11.81 KB, patch)
2005-10-19 19:02 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Fixed "another approach" patch (12.02 KB, patch)
2005-10-20 07:35 PDT, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description Francois Ingelrest 2005-02-09 13:15:53 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050209 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050209 Firefox/1.0+

When I open a menu from my bookmarks toolbar for the first time since FF was
launched, there is no drop shadow (drawn by the OS). If I close it and re-open
it, the shadow will then be drawn, and will always be drawn for that particular
menu until FF is closed.

Reproducible: Always

Steps to Reproduce:
1. Close FF
2. Launch it
3. Open a menu from the bookmarks toolbar
4. Close the menu
5. Re-open it

Actual Results:  
After step #3, there is no shadow under the menu

Expected Results:  
The shadow should always be drawn

You must have enabled drawing drop shadows in the configuration of Windows.

It does not seem to happen under win2k (
http://forums.mozillazine.org/viewtopic.php?p=1220505#1220505 )
Comment 1 Ger Teunis 2005-02-09 14:26:50 PST
If I open the bookmarks menu, this menu has an shadow.
The sub-menus of the bookmarks-menu do not.

Windows XP in Luna/candybar mode.
Running "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050209
Firefox/1.0+"
using Fx's Default theme.
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-09 17:23:09 PST
I can't see this bug, using a 2005-01-19 Firefox trunk build, but I can see the
bug, using 2005-01-20 Firefox trunk build.
This might have something to do with the fix for bug 244366...
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2005-02-09 19:35:55 PST
It might, yes. I thought I fixed the remaining menu issues in bug 279308,
though... :(

I'm really somewhat at a loss for what's going on here.  How do these shadows
get drawn, exactly?  Are we doing it via -moz-appearance, or is it an OS thing?

Also, do these shadows get drawn for SeaMonkey (say in the classic theme)?  If
so, is the bug present there too?
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-10 10:09:39 PST
The bug is also present in current Seamonkey trunk builds, in the classic theme.
This bug is also seen for the regular menu items ("File", "Edit", etc)
I also see no dropshadow anymore for tooltips and context menu. For these kind
of popups the dropshadow doesn't ever appear, not even when the tooltip/context
menu is reopened. 
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2005-02-10 10:12:12 PST
Ok... so who actually draws the dropshadows?
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-02-10 13:01:32 PST
I think Windows is doing it in response to some window style we set via Win32.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2005-02-10 13:03:35 PST
OK.. any idea where said window style would be set?
Comment 8 Dean Tessman 2005-02-10 17:51:55 PST
If aInitData of a window being created has the mDropShadow flag set, then the
window is created as WindowPopupClassW.  See:

http://lxr.mozilla.org/mozilla/source/widget/src/windows/nsWindow.cpp#1552

nsWindow::WindowPopupClassW() creates a window with the CS_XP_DROPSHADOW flag
set.  (Aside: it should be just CS_DROPSHADOW, to be consistent with the
constant name used by Windows.)

http://lxr.mozilla.org/mozilla/source/widget/src/windows/nsWindow.cpp#4826

After that, it's up to Windows to draw the shadow.
Comment 9 Dean Tessman 2005-02-10 18:16:45 PST
mDropShadow is set in nsMenuPopupFrame::Init().

  widgetData.mDropShadow = !(tag && tag == nsXULAtoms::menulist);

So basically, any XUL menu popup that isn't a <menulist> will get a drop shadow.

http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsMenuPopupFrame.cpp#221
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2005-02-10 20:24:10 PST
Created attachment 174013 [details] [diff] [review]
Random hack #1 to test
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2005-02-10 20:27:54 PST
Created attachment 174014 [details] [diff] [review]
Random hack #2 to test

Could someone who can see this in a build they compiled themselves try these
two patches and tell me whether one (or both together, but neither on its own)
helps?

The first patch just needs a rebuild in layout/base and layout/build; the
second needs one in view/src and layout/build.
Comment 12 Dan 2005-02-11 07:43:31 PST
In addition to the shadow not appearing the first time a submenu is displayed, 
the shadow *never* appears for context menus.
Comment 13 Dan 2005-02-11 07:51:33 PST
Created attachment 174043 [details]
Image of shadows missing from sub menu and context menu
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-11 11:02:46 PST
Ok, random hack #1 seems to fix the shadow bug for menus, but not for the
context menu and tooltips.
But the issue with context menu and tooltips has a different regression period:
Works in:
20041011 07:23am Firefox build
Fails in:
20041012 07:18am firefox build
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20041011+07%3A00%3A00&maxdate=20041012+07%3A23%3A00&cvsroot=%2Fcvsroot

Random hack #2 doesn't seem to fix anything.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2005-02-11 12:49:15 PST
OK, if random hack #1 helps, that means someone is violating our invariants
about nor reflowing during frame construction...  Could you back that patch out,
then put a breakpoint at
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&rev=3.808&mark=4940#4939>
(the marked line would do fine), see what the callstack is that gets us in
there, and attach said callstack to this bug?

The context menu and tooltips issue sounds like fallout from bug 238493. To
test, remove the early return at
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/view/src/nsView.cpp&rev=3.210#370>.

Sounds like the general problem is that someone is (incorrectly) expecting
something to happen synchronously at all times.
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-11 15:01:21 PST
Created attachment 174098 [details]
callstack

This is the callstack made, based on comment 15. I hope I did it right.

(In reply to comment #15)
> The context menu and tooltips issue sounds like fallout from bug 238493. To
> test, remove the early return at
>
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/view/src/nsView.cpp&rev=3.210#370>.

Yes, that helps for the context menu and tooltips issue.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2005-02-11 23:18:12 PST
> This is the callstack made, based on comment 15. I hope I did it right.

Hmm... That looks like it's triggered by the (synchronous!) onpopupshowing
event.  Specifically, see
<http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/popup.xml#151>
and
<http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/popup.xml#151>.

Does anything else hit that breakpoint?  Or just this code?  If you comment out
the body of the popupshowing method, do you hit the breakpoint at all?  Does the
bug still appear? If the answers are "no" and "yes" respectively, does then
applying the random hack #1 still help?  If the first answer is "yes", then
what's the stack?  Or the stacks if there is more than one?

It sounds like we should file a separate bug, dependent on this one, for the
context menu issue...
Comment 18 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-12 12:05:32 PST
Created attachment 174166 [details]
callstack2

(In reply to comment #17)
> Does anything else hit that breakpoint?  Or just this code?  If you comment
out
> the body of the popupshowing method, do you hit the breakpoint at all?  Does
the
> bug still appear? 
It doesn't seem that anything else is hitting the breakpoint. When I remove the
js code of the popupshowing event handler, then I don't hit the breakpoint at
all (before that, I hit the breakpoint 14 times). The bug doesn't appear
anymore in that case. 
But when I shift between menu-items (between File, Edit, View) while the popup
is open, I hit the breakpoint again. In that case, the bug is showing up again.

I've attached the callstack of that instance.

> It sounds like we should file a separate bug, dependent on this one, for the
> context menu issue...
I filed bug 282059 for this.
Comment 19 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-12 12:11:27 PST
Forgot to mention, the breakpoint is also still triggered for bookmark folders
in the bookmarks toolbar menu, when the js code of the popupshowing event
handler is removed. The bug is also still showing in that case.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2005-02-12 20:15:02 PST
Er... could some win32 widget person tell me why commenting out the XBL handler
makes the shadow show up?
Comment 21 neil@parkwaycc.co.uk 2005-02-13 03:49:32 PST
It's  not some weird layout reentrancy bug caused by setting the widths then?
Comment 22 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-13 07:35:59 PST
Sorry for the confusion.
The behavior I mentioned in comment 18 is also present in the current Mozilla
trunk builds:
- when opening the menu directly, the dropshadow is visible. But when shifting
between menu-items, the menu-popup is not vible for the first time.
- When opening folders in the bookmarks menu or bookmarks toolbar, the drop
shadow is not visible for the first time.
So "callstack" is worthless. "callstack2" might be useful.
Comment 23 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-13 09:09:02 PST
When I have removed the code for the popupshowing event handler _and_ I have my
build patch with "Random hack #1", I don't hit the breakpoint at all, not with
shifting between menu-items, nor for opening folders in the bookmarks toolbar menu.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2005-02-15 11:09:16 PST
I filed bug 282359 on Martijn's first stack...  It may be that my proposed fix
for that would fix the second stack too, and maybe even this bug.

Dean, could it cause problems if the popup window is shown at a size of (0,0)
and then later resized to its actual size?  Does Resize() need to do anything to
notify the OS that the size of the window has changed so it can change the size
of the drop-shadow?
Comment 25 Dean Tessman 2005-02-15 20:21:39 PST
(In reply to comment #24)
> Dean, could it cause problems if the popup window is shown at a size of (0,0)
> and then later resized to its actual size?  Does Resize() need to do anything to
> notify the OS that the size of the window has changed so it can change the size
> of the drop-shadow?

Not that I know of, but I'll check.  Any chance of forcing it to open at a size
of something like (500,500) and seeing if that has weird effects on the drop shadow?
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2005-02-15 20:26:22 PST
Sure.  In nsMenuFrame::OpenMenuInternal, at the place where it does 

  815       menuPopup->SetBounds(state, nsRect(0,0,mLastPref.width,
                                 mLastPref.height));

stick in a random width/height, then do the "hide view, sync view with frame,
unhide view" thing (the unhide happens in ActivateMenu, but you could just copy
the code from there...

Then go through the real sizing this code does.  Alternately, just change the
mLastPref.width/mLastPref.height here to 9000x9000 (twips, so about 500px) and
assume that when the reflow happens for real it'll get resized back (I suspect
it will).
Comment 27 Dean Tessman 2005-02-15 20:34:50 PST
That'll have to wait till I get my build machine re-created on my new box.  Can
someone else on XP take a run at bz's suggestion?

I didn't find much about resizing, but this does sound similar to our problem:
http://tinyurl.com/6g6lc
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2005-02-15 20:38:06 PST
Yeah, I bet the problem is that the "second layered window" simply doesn't
resize properly when the menu resizes.

Do we need to add support to win32 widgets to actually hide and reshow the
widget on move/resize if the widget has the right flags to need this shadow thing?
Comment 29 Dean Tessman 2005-02-15 21:01:04 PST
That would flicker a heck of a lot, wouldn't it?  There's got to be a better way
than that.

What about removing and re-adding the CS_DROPSHADOW style flag?
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2005-02-15 21:06:12 PST
Dunno.  Someone would have to test....
Comment 31 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-17 04:45:37 PST
The proposed fix for bug 282359 seems to fix this bug, indeed. 
Comment 32 neil@parkwaycc.co.uk 2005-02-17 06:20:53 PST
Which one? There are two ;-)
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2005-02-17 08:55:03 PST
Dean, I don't see a way to dynamically change the class of an existing window
offhand...  The closest I've code to finding something that would remove and
readd the dropshadow is SystemParametersInfo (which would globally disabled and
reenable dropshadows, if I read it right):
<http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/systemparametersinfo.asp>
Comment 34 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-17 09:19:45 PST
(In reply to comment #32)
> Which one? There are two ;-)
Oops! I didn't see the second one (wasn't CC-ed).
Ok, the second one also seems to resolve this bug, but in my build, the
menu-items can't be closed anymore after they've been opened. It might be be
that my build is a bit too old (two weeks, I guess). The "   
mCreateHandlerSucceeded = PR_FALSE;" part I had to apply by hand.

Comment 35 Dean Tessman 2005-02-17 10:55:31 PST
bz: I wasn't suggesting changing the class, just removing and adding the
specific style using GetWindowLong/SetWindowLong.
Comment 36 Dean Tessman 2005-02-17 10:57:21 PST
Other ideas, down at the Windows API level, are:

1. RedrawWindow().  Either invalidate and redraw the entire window, or just the
frame with RDW_FRAME.

2. UpdateWindow()
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2005-02-17 11:01:49 PST
Dean, any chance of just writing up a patch that Martijn or someone could test?
 Or talking me through it via email or irc if you have no tree at all?
Comment 38 Dean Tessman 2005-02-17 11:44:54 PST
Sure.  Email would work, as I'm on a business trip right now.
Comment 39 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-18 01:23:48 PST
(In reply to comment #32)
> Which one? There are two ;-)
Ok, I now tried the second patch with an updated build and it works fine (and
fixes this bug).
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2005-02-20 15:30:34 PST
Created attachment 174913 [details] [diff] [review]
Random hack #3 to test
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2005-02-20 15:33:34 PST
Created attachment 174915 [details] [diff] [review]
Random hack #4 to test

So... could someone try backing out all other patches for this bug and bug
282359 from their tree and try these two?  First try random hack #3; if that
doesn't help, back it out and try random hack #4?  After applying, you just
need to rebuild in widget/; no need to rebuild the whole tree.
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2005-02-20 19:31:57 PST
Created attachment 174940 [details] [diff] [review]
Random hack #3 that might compile
Comment 43 Boris Zbarsky [:bz] (still a bit busy) 2005-02-20 19:32:40 PST
Created attachment 174941 [details] [diff] [review]
Random hack #4 that might compile
Comment 44 Dean Tessman 2005-02-20 22:26:39 PST
> Random hack #3 that might compile

+    style = style & ~CS_DROPSHADOW;
+    ::SetWindowLong(mWnd, GWL_STYLE, style);
+    // might need a RedrawWindow() call in here... or something?
+    style |= CS_DROPSHADOW;

Probably want to change these two to CS_XP_DROPSHADOW as well.  Sorry, bz,
forgot about that different name in my email to you. 

Comment 45 Boris Zbarsky [:bz] (still a bit busy) 2005-02-20 23:02:58 PST
OK, I got vnc access to an XP machine and did some testing:

1)  By default, the popups in question do NOT have CS_DROPSHADOW in the style
    (according to GetWindowLong).  Apparently this method doesn't see styles set
    on the window class?
2)  If I set the CS_DROPSHADOW style any time aInitData->mDropShadow is set
    (using SetWindowLong in StandardWindowCreate), then GetWindowLong does see
    the style.  Removing and resetting the style in DidResize() per the patches
    I was trying has no effect (perhaps because it's always in the class
    style?).
3)  If I set the CS_DROPSHADOW style as in item #2 and then take the approach in
    "random hack #4" (hide and reshow windows with that style set) then the bug
    is "fixed".  Given that I was doing this over vnc, I could pretty clearly
    see a flicker as the menu comes up the first time.

Where does that leave us?  I guess we could store whether we actually have the
dropshadow window class, so the flicker thing would only be an issue on WinXP...
 And if we work on fixing callers not to put up menus till they're at the final
size, the flicker will appear pretty rarely.

That said, if we can figure out a way to make the shadow reappear without hiding
and reshowing the popup, that would be nice... Perhaps we should be using
SetWindowLong all along, instead of putting the style in the window class?  Or
will it not work then?
Comment 46 Dean Tessman 2005-02-20 23:44:20 PST
Disclaimer: All of these ideas are total shots in the dark.  I really need to
get my build machine functional, hopefully in the next week or so.

1. bz, you're right.  The style isn't on the window, it's on the class.  Any
difference if you use Get/SetClassLong(GCL_STYLE)?  You might have to add
subsequent calls to InvalidateRect(hWnd, null, FALSE) then UpdateWindow()?

2. If that doesn't work, try changing GWL_STYLE in hack #3 to GWL_EXSTYLE.

3. You can try forcing an update on the window.  I haven't used it before, but
what about RedrawWindow(hWnd, null, 0, flags), where flags is one of:
  a. RDW_VALIDATE
  b. RDW_FRAME | RDW_INVALIDATE
  c. RDW_FRAME | RDW_INVALIDATE | RDW_UPDATENOW

4. Send a WM_NCPAINT to the window?
Comment 47 Boris Zbarsky [:bz] (still a bit busy) 2005-02-23 10:26:54 PST
OK, with bug 282359 fixed the "first appearance" issue should be solved.  We
should still pursue Dean's suggestions, since they are absolutely needed to fix
bug 282059.
Comment 48 Boris Zbarsky [:bz] (still a bit busy) 2005-02-23 10:32:46 PST
Created attachment 175341 [details] [diff] [review]
Random hack #5 -- now with Get/SetClassLong

Could someone test this?
Comment 49 Dean Tessman 2005-02-23 11:22:14 PST
> Random hack #5 -- now with Get/SetClassLong

If you're testing this randomness, change the three occurrences of GWL_STYLE to
GCL_STYLE before compiling.

+  LONG style = ::GetClassLong(mWnd, GWL_STYLE);
+    ::SetClassLong(mWnd, GWL_STYLE, style);
+    ::SetClassLong(mWnd, GWL_STYLE, style);
Comment 50 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-02-23 13:33:36 PST
(In reply to comment #48)
> Random hack #5 -- now with Get/SetClassLong
> Could someone test this?
Doesn't work for me (I also tried hack #3 and hack #4, they didn't work also,
but I couldn't figure out the various suggestions for alterations).
I've also tried the suggestion in comment 49, using GCL_STYLE instead of
GWL_STYLE. This also doesn't fix the bug. (I have a debug build that doesn't
have the fix for bug 282359 yet).
Comment 51 Boris Zbarsky [:bz] (still a bit busy) 2005-04-12 09:53:34 PDT
Nominating for blocking status... I think we really want to get this resolved;
otherwise our context menus look really weird....
Comment 52 Asa Dotzler [:asa] 2005-07-11 17:20:18 PDT
transferring nomination to 1.8b4. If we don't get it there, we don't get it for 1.1
Comment 53 James Ross 2005-09-23 20:36:49 PDT
*digs himself out of widget, view and layout*

(In reply to comment #15)
> Sounds like the general problem is that someone is (incorrectly) expecting
> something to happen synchronously at all times.

bz's comment was right on the ball, and the suspect is
nsPopupSetFrame::DoLayout. This is what happens when opening a popup (not
top-level menu, they are handled so different it almost isn't funny):

[3adf740] nsMenuPopupFrame::Init: SetViewVisibility(hide)
[29b0e08] nsViewManager::SetViewVisibility: SetVisibility(0)
[29b0e08] nsViewManager::SetViewVisibility: DONE
[29b0e08] nsViewManager::SetViewVisibility: UpdateView/a
[29b0e08] nsViewManager::SetViewVisibility: DONE
[3adf740] nsMenuPopupFrame::Init: DONE
[3adf740] nsMenuPopupFrame::Init: CreateWidget
[de05a4] nsWindow::CreateWindowEx: style = c2000000, ex_style = 88, parent = 0
[39f2e30] nsView::SetViewVisibility: SetVisibility(0)
[de05a4] nsWindow::Show: state = 0
[39f2e30] nsView::SetViewVisibility: DONE
[3adf740] nsMenuPopupFrame::Init: DONE
[3adf740] nsMenuPopupFrame::Init: MoveToAttributePosition
[3adf740] nsMenuPopupFrame::Init: DONE
[2ad86b0] nsPopupSetFrame::DoLayout: ResizeView, size = (2490, 3300)
[2ad86b0] nsPopupSetFrame::DoLayout: DONE
[2ad86b0] nsPopupSetFrame::DoLayout: SetViewVisibility(show)
[29b0e08] nsViewManager::SetViewVisibility: SetVisibility(1)
[39f2e30] nsView::SetViewVisibility: SetVisibility(1)
[de05a4] nsWindow::Show: state = 1
[39f2e30] nsView::SetViewVisibility: DONE
[29b0e08] nsViewManager::SetViewVisibility: DONE
[2ad86b0] nsPopupSetFrame::DoLayout: DONE
[29b0e08] nsViewManager::FlushPendingInvalidates - BEGIN (ProcessPendingUpdates)
[de05a4] nsWindow::Move to (873, 746), resize to (166, 220), repaint = 1
[29b0e08] nsViewManager::FlushPendingInvalidates - END

Problems occur not because nsPopupSetFrame::DoLayout resizes the view, then
makes it visisble (correct order to do things), but because the resize is queued
up for later (refresh disabled, presumably because we're right in the middle of
a reflow?) and the set visibility isn't.

Thus, the two key events (resize, show) occur backwards.
Comment 54 Boris Zbarsky [:bz] (still a bit busy) 2005-09-23 20:47:00 PDT
So maybe nsMenuPopupFrame should not show its widget until after it's gotten its
initial reflow?
Comment 55 James Ross 2005-09-23 21:07:49 PDT
Well, it's nsPopupSetFrame that's actually showing it, but yes, the showing
definately needs to be delayed until the resize is done (this applies generally
to all popups - except top-level menus - as far as I can see, so it wouldn't
work as a change to the menu popup code, AFAICT).

I've had a bit of a poke at making nsView::SetVisibility get buffered like
resizing the view is. It works, but better ideas welcome.

http://twpol.dyndns.org/temp/firefox_popup_shadow.png
Comment 56 James Ross 2005-09-24 10:58:37 PDT
Created attachment 197275 [details] [diff] [review]
Buffer view visibility changes like resizes

This patch is what I tried yesterday, and fixes the context menu and tooltip
shadows. I've not found anything it breaks, either, but I don't know enough
about layout & views to know if it is the Right Way or if it is really
bad/evil. :)
Comment 57 Todd A. Fisher 2005-09-24 12:25:30 PDT
Built xulrunner with the patch from attachment 197275 [details] [diff] [review] applied, looks good - all
the xul popups have dropshadows again.
Comment 58 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-26 19:28:07 PDT
Comment on attachment 197275 [details] [diff] [review]
Buffer view visibility changes like resizes

Looks okay, but it needs some trunk testing.
Comment 59 Mike Connor [:mconnor] 2005-09-27 09:15:40 PDT
Silver: if you're not where you can land this, let me know and I'll get it in
today on trunk.
Comment 60 James Ross 2005-09-28 02:01:01 PDT
Comment on attachment 197275 [details] [diff] [review]
Buffer view visibility changes like resizes

Checked in to trunk.
Comment 61 Vidar Haarr (not reading bugmail) 2005-09-28 08:11:29 PDT
This might be some local change I have that needs to be fixed because of some
recent checkin, but seeing as this seems like the most related change in the
last day to what I'm seeing, I'll comment here before testing further.

1. Open a tab with www.mozilla.org
2. Open a new tab with www.mozillazine.org
3. Switch between them 2 times.

Results: Both tabs are rendered only as a flat color (gray on my system, might
be white on others).
Comment 62 Vidar Haarr (not reading bugmail) 2005-09-29 00:57:22 PDT
Yes, attachment 197275 [details] [diff] [review] did indeed cause what I described in comment 61.
Seamonkey Linux GTK2 build.
Comment 63 James Ross 2005-09-29 02:47:07 PDT
Can you actually describe what you see better? Or better yet, provide a 
picture? What about a tinderbox (not sure the nightlies have built yet) build?

I can't reproduce any rendering problems (both following your steps and with 
general usage) with tabs - or anything else, for that matter - with both a 
tinderbox build and my own Firefox build, on Windows.
Comment 64 Vidar Haarr (not reading bugmail) 2005-09-29 03:22:59 PDT
Created attachment 197829 [details]
gray content area

1. Open two tabs.
2. Switch to the second tab (renders OK) and back again (gray).
3. Switch to the second tab (gray).
Comment 65 David Baron :dbaron: ⌚️UTC-10 2005-09-29 17:43:05 PDT
I'm also seeing comment 61.
Comment 66 Asa Dotzler [:asa] 2005-09-29 18:30:33 PDT
*** Bug 282059 has been marked as a duplicate of this bug. ***
Comment 67 Boris Zbarsky [:bz] (still a bit busy) 2005-09-29 20:16:48 PDT
I'll bet the problem is that nsIWidget::IsVisible is nowhere guaranteed to match
the value you last passed to Show().  And the behavior is not consistent across
platforms, which is why everything is happy on Windows but not on Linux.  See
bug 268090 comment 18 and bug 268090 comment 22.

Note that I ran into exactly this sort of problem when I tried pretty much this
patch in bug 264231.

Chances are this can work if we add another api on nsIWidget for what we really
want to know here (or change IsVisible and check all its callers to make sure
they can cope with the new behavior...).
Comment 68 James Ross 2005-09-30 05:55:28 PDT
So am I right in thinking the problem is that views/widgets are claiming to be
visible while a hide has been buffered (so they say "I'm visisble" but as soon
as they get unbuffered they vanish), and visa versa?

I can't do any testing on Linux, so unless someone else can work up a patch to
fix it, the only option is to back it out.
Comment 69 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-30 10:51:57 PDT
A disgusting branch-only hack would be to enable this patch for Windows only.
Comment 70 Boris Zbarsky [:bz] (still a bit busy) 2005-09-30 11:00:15 PDT
James, would you be willing to implement nsIWidget::IsShown (on all platforms)
which returns the last value passed to Show()?  Then use it here.  That should
fix the Linux problems...
Comment 71 James Ross 2005-09-30 11:31:09 PDT
That sounds possible. I'll give it a bash. I don't know what you mean by "use it
here", though - where in the current patch would I use it?
Comment 72 James Ross 2005-09-30 11:36:21 PDT
Oh, I see. You mean the mWindow->IsVisisble call is returning bogus answers, and
thus it is not updating the visibility when it should be?
Comment 73 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-09-30 11:42:06 PDT
right
Comment 74 David Baron :dbaron: ⌚️UTC-10 2005-09-30 12:27:03 PDT
I've backed out the original patch until the regressions can be fixed; having
tabbed browsing be unusable on a Tier 1 platform shouldn't stay in the tree for
days.
Comment 75 James Ross 2005-09-30 12:55:31 PDT
Thank you and good-bye.
Comment 76 David Baron :dbaron: ⌚️UTC-10 2005-10-01 01:23:28 PDT
I'm not saying we don't want the patch, just that we don't want it until we also
have the regression fix.  I was still hoping you'd work on comment 70 / comment
72 -- although maybe comment 75 doesn't mean what I thought it meant.
Comment 77 David Baron :dbaron: ⌚️UTC-10 2005-10-01 15:29:02 PDT
Look, I'm sorry if I offended you by backing the patch out without giving you
notice first or asking you to do it.  (Given your UK time zone, I also wasn't
sure if you'd be able to respond before the next day.)  Getting backed out isn't
something you should take personally (and, frankly, it's something we should be
doing more often).  It doesn't mean we don't want your patch.  It just means
that the regressions are serious enough that they prevent other people from
getting work done (testing, development, etc.).  I hope you still work on the
regression fix, and once that's reviewed you can check the whole thing back in.
Comment 78 Ere Maijala (slow) 2005-10-02 00:22:50 PDT
CC'ing James back (please read the last comments).
Comment 79 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-10-02 13:49:31 PDT
I've had a little e-mail conversation with James, and he basically said he won't
be contributing to this bug anymore.
So I guess someone else has to adjust the patch to circumvent the regression.
Comment 80 Blake Kaplan (:mrbkap) 2005-10-02 16:12:38 PDT
I volunteered to take a crack at this once my other blockers are squared away,
so I may get to this on Monday.
Comment 81 Scott MacGregor 2005-10-03 14:54:41 PDT
Created attachment 198360 [details] [diff] [review]
fall back patch for the branch....

In case we don't come up with a real patch by the deadline this evening, here's
the patch ifdef'ed for XP_WIN32 only, thus dodging the regression on linux. If
this patch goes in, we should leave it off of the trunk and continue pursuing
the current discussion.
Comment 82 David Baron :dbaron: ⌚️UTC-10 2005-10-03 15:04:06 PDT
FWIW, another thing I don't like about this code (although it's just being
moved) is that view visibility being hidden generally applies to descendant
views as well, but we seem to be being inconsistent about that here -- calling
IsViewVisible (a static function in nsViewManager.cpp) may be preferable.
Comment 83 Blake Kaplan (:mrbkap) 2005-10-03 18:20:24 PDT
Created attachment 198391 [details] [diff] [review]
possible patch

This patch attempts to follow the advice given in the above comments. It does
not appear to regress tabbed browsing on GTK 2.
Comment 84 Blake Kaplan (:mrbkap) 2005-10-03 21:35:25 PDT
Created attachment 198408 [details] [diff] [review]
Compiling on Windows

This is like attachment 198391 [details] [diff] [review], except that it actually compiles on Windows.
jst says that it worked for him. This needs testing on all platforms possible.
I've (indirectly tested gtk2 and Mac so far). Looking for r=.
Comment 85 Stuart Parmenter 2005-10-03 22:12:10 PDT
Comment on attachment 198408 [details] [diff] [review]
Compiling on Windows

So this patch sucks.  It basically adds an API that should in theory do the
same thing as IsVisible() on nsIWidget but it doesn't quite.

If we want a fix for the branch, this one isn't _awful_, but it certainly isn't
a long term fix.  If we're going to take it, we should probably only do so on
the branch and take a right fix on the trunk.  Widget is a big enough mess
already.. don't need it to get any worse.
Comment 86 Stuart Parmenter 2005-10-03 22:14:28 PDT
I guess I should be a little more specific.  I'd like to see IsVisible() fixed
in all the widget implementations to do the right thing so that this patch can
work without needing IsShown() at all.  I don't think that that fix is very hard
or big, but requires more testing than could be done tonight for the branch.
Comment 87 Darin Fisher 2005-10-03 22:24:57 PDT
Comment on attachment 198408 [details] [diff] [review]
Compiling on Windows

>@@ -425,20 +425,25 @@ class nsIWidget : public nsISupports {
>      */
>     NS_IMETHOD SetModal(PRBool aModal) = 0;
> 
>     /**
>      * Returns whether the window is visible
>      *
>      */
>     NS_IMETHOD IsVisible(PRBool & aState) = 0;
> 
>     /**
>+     * Returns whether or not this widget is actually shown.
>+     */
>+    virtual PRBool IsShown() = 0;
>+

nsIWidget is an XPCOM interface (despite not being written in
XPIDL).  You must change NS_IWIDGET_IID whenever you change its
vtable layout.
Comment 88 Brendan Eich [:brendan] 2005-10-03 22:25:50 PDT
Comment on attachment 198408 [details] [diff] [review]
Compiling on Windows

Branch-only -- no one wants this on the trunk.	Question is, who will fix
IsVisible there?

/be
Comment 89 Scott MacGregor 2005-10-03 22:27:14 PDT
Comment on attachment 198408 [details] [diff] [review]
Compiling on Windows

Blake, please bump the IID before you check this in. I'll let Brendan's a=
stand but that needs to get changed before this lands. Thanks!
Comment 90 Brendan Eich [:brendan] 2005-10-03 22:33:44 PDT
(In reply to comment #89)
> (From update of attachment 198408 [details] [diff] [review] [edit])
> Blake, please bump the IID before you check this in. I'll let Brendan's a=
> stand but that needs to get changed before this lands. Thanks!

Argh!  I should have said that, and said to "add at the end".  I now owe Darin
two pounds of flesh.

/be
Comment 91 Blake Kaplan (:mrbkap) 2005-10-03 22:39:15 PDT
I checked a fix in with all of the comments addressed (new iid, new function at
the end). Leaving this bug open for a better branch fix. Somebody should feel
very (very!) free to steal this from me.
Comment 92 Boris Zbarsky [:bz] (still a bit busy) 2005-10-04 18:43:48 PDT
pav, the problem is that iirc some consumers really do want to know whether the
widget is actually visible.  So for those consumers what IsVisible() does now on
GTK1/2 is preferable.  Other consumers want what this IsShown() thing returns
(here, and some IsVisible() callers we already have).
Comment 93 David Baron :dbaron: ⌚️UTC-10 2005-10-09 14:16:18 PDT
It sounds like this may have caused bug 311146.
Comment 94 David Baron :dbaron: ⌚️UTC-10 2005-10-19 09:47:21 PDT
I think this should be backed out.  See bug 311146 comment 26.
Comment 95 David Baron :dbaron: ⌚️UTC-10 2005-10-19 11:12:21 PDT
(Note that this is checked in only on MOZILLA_1_8_BRANCH and bug 311146 is only
on the branch.)
Comment 96 Stuart Parmenter 2005-10-19 15:49:39 PDT
I've got a branch build going.  I'll start looking at it, but I don't know the
view code at all.  Let me know what ideas I can try Boris.
Comment 97 Boris Zbarsky [:bz] (still a bit busy) 2005-10-19 18:35:19 PDT
So I talked to Blake, and it sounds like the toplevel menus do show the
dropshadows (which would make sense since we fixed bug 282359, I think...).  So
the remaining issue is context menus and suchlike.
Comment 98 Boris Zbarsky [:bz] (still a bit busy) 2005-10-19 18:35:26 PDT
Created attachment 200158 [details] [diff] [review]
One possible patch

This is one possible approach.	This makes sure to update the widget size
before showing it.  This would be in place of the patch mrbkap already checked
in on the branch, not in addition to it...
Comment 99 Boris Zbarsky [:bz] (still a bit busy) 2005-10-19 18:36:57 PDT
Created attachment 200159 [details] [diff] [review]
Same as diff -w
Comment 100 Boris Zbarsky [:bz] (still a bit busy) 2005-10-19 19:01:50 PDT
Created attachment 200163 [details] [diff] [review]
backout of mrbkap's patch
Comment 101 Boris Zbarsky [:bz] (still a bit busy) 2005-10-19 19:02:49 PDT
Created attachment 200164 [details] [diff] [review]
Another approach

Again, this would replace mrbkap's patch...
Comment 102 Stuart Parmenter 2005-10-19 22:14:02 PDT
Comment on attachment 200158 [details] [diff] [review]
One possible patch

this patch works for me after backing out blake's patch
Comment 103 Bob Clary [:bc:] 2005-10-20 03:22:38 PDT
The first patch (One possible patch) attachment 200158 [details] [diff] [review] worked for me on trunk
(drop shadow on menu items and context menus) and 1.8 branch (drop shadow on
context menus, but I forgot to check menu items) after mrbkap's stuff was backed
out.

The second patch (Another approach) attachment 200164 [details] [diff] [review] did not work on the
branch. Menu items had the drop shadow but context menus did not.
Comment 104 Boris Zbarsky [:bz] (still a bit busy) 2005-10-20 07:35:39 PDT
Created attachment 200205 [details] [diff] [review]
Fixed "another approach" patch

I missed the place that actually mattered, somehow... this should work, I
think.	Can someone test?
Comment 105 Stuart Parmenter 2005-10-20 14:09:06 PDT
Comment on attachment 200205 [details] [diff] [review]
Fixed "another approach" patch

this patch also works.
Comment 106 Scott MacGregor 2005-10-20 14:39:44 PDT
bz & others, any objections to me backing out the original patch so we can close
out the regression this caused? It seems clear we are on the right track for a
different fix now and we'll be backing this out anyway. I'd like to do that
today so speak up if you object. 
Comment 107 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-10-20 15:25:14 PDT
Comment on attachment 200205 [details] [diff] [review]
Fixed "another approach" patch

I slightly prefer the first one, but this one is probably lower risk. What do
you think, Boris?
Comment 108 Boris Zbarsky [:bz] (still a bit busy) 2005-10-20 15:59:37 PDT
roc, that was basically my assessment as well -- the first patch is far better
on general principle, the second one is slightly lower risk since it only
touches context menus.

I think I would prefer to go with the first patch; I think the difference in
risk is pretty minimal and the first patch might help out other places that we
haven't caught too...
Comment 109 Boris Zbarsky [:bz] (still a bit busy) 2005-10-20 16:11:54 PDT
Scott, I think backing the widget change out is the way to go for 1.8, yeah...
Comment 110 Scott MacGregor 2005-10-20 16:37:04 PDT
I've backed out the original patch from the branch to get the ball rolling. So
now we can evaulate bz's new fix that doesn't cause the regression for 1.8.
Comment 111 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-10-20 19:40:31 PDT
checked in on trunk and branch.
Comment 112 Simon Fraser 2005-10-21 23:57:52 PDT
The backed-out patch caused issues with Cocoa widget (bug 312563). I'll test
with the one that roc checked in.
Comment 113 kokoko3k 2016-10-20 02:14:51 PDT
Now it happens under linux with kde/kwin.
shadows are missing from context menus and awesome bar popup menu.
Comment 114 kokoko3k 2016-10-20 02:15:52 PDT
(In reply to kokoko3k from comment #113)
> Now it happens under linux with kde/kwin.
> shadows are missing from context menus and awesome bar popup menu.

compositing is active
Comment 115 David Baron :dbaron: ⌚️UTC-10 2016-10-20 14:46:08 PDT
Please move that problem to a new bug, rather than commenting in a bug that was fixed 11 years ago.

Note You need to log in before you can comment on or make changes to this bug.