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 )

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.

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...

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?

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.

Ok... so who actually draws the dropshadows?

I think Windows is doing it in response to some window style we set via Win32.

OK.. any idea where said window style would be set?

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.

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

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.

In addition to the shadow not appearing the first time a submenu is displayed, the shadow *never* appears for context menus.

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.

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.

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.

> 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...

(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.

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.

Er... could some win32 widget person tell me why commenting out the XBL handler makes the shadow show up?

It's not some weird layout reentrancy bug caused by setting the widths then?

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.

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.

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?

(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?

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).

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

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?

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?

Dunno. Someone would have to test....

The proposed fix for bug 282359 seems to fix this bug, indeed.

Which one? There are two ;-)

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>

(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.

bz: I wasn't suggesting changing the class, just removing and adding the specific style using GetWindowLong/SetWindowLong.

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()

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?

Sure. Email would work, as I'm on a business trip right now.

(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).

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.

> 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.

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?

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?

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.

Could someone test this?

> 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);

(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).

Nominating for blocking status... I think we really want to get this resolved; otherwise our context menus look really weird....

transferring nomination to 1.8b4. If we don't get it there, we don't get it for 1.1

*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.

So maybe nsMenuPopupFrame should not show its widget until after it's gotten its initial reflow?

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

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. :)

Built xulrunner with the patch from attachment 197275 [details] [diff] [review] applied, looks good - all the xul popups have dropshadows again.

Comment on attachment 197275 [details] [diff] [review] Buffer view visibility changes like resizes Looks okay, but it needs some trunk testing.

Silver: if you're not where you can land this, let me know and I'll get it in today on trunk.

Comment on attachment 197275 [details] [diff] [review] Buffer view visibility changes like resizes Checked in to trunk.

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).

Yes, attachment 197275 [details] [diff] [review] did indeed cause what I described in comment 61. Seamonkey Linux GTK2 build.

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.

1. Open two tabs. 2. Switch to the second tab (renders OK) and back again (gray). 3. Switch to the second tab (gray).

I'm also seeing comment 61.

*** Bug 282059 has been marked as a duplicate of this bug. ***

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...).

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.

A disgusting branch-only hack would be to enable this patch for Windows only.

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...

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?

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?

right

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.

Thank you and good-bye.

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.

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.

CC'ing James back (please read the last comments).

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.

I volunteered to take a crack at this once my other blockers are squared away, so I may get to this on Monday.

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.

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.

This patch attempts to follow the advice given in the above comments. It does not appear to regress tabbed browsing on GTK 2.

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 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.

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 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 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 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!

(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

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.

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).

It sounds like this may have caused bug 311146.

I think this should be backed out. See bug 311146 comment 26.

(Note that this is checked in only on MOZILLA_1_8_BRANCH and bug 311146 is only on the branch.)

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.

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.

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...

Again, this would replace mrbkap's patch...

Comment on attachment 200158 [details] [diff] [review] One possible patch this patch works for me after backing out blake's patch

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.

I missed the place that actually mattered, somehow... this should work, I think. Can someone test?

Comment on attachment 200205 [details] [diff] [review] Fixed "another approach" patch this patch also works.

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 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?

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...

Scott, I think backing the widget change out is the way to go for 1.8, yeah...

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.

checked in on trunk and branch.

The backed-out patch caused issues with Cocoa widget (bug 312563). I'll test with the one that roc checked in.

Now it happens under linux with kde/kwin. shadows are missing from context menus and awesome bar popup menu.

(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

Please move that problem to a new bug, rather than commenting in a bug that was fixed 11 years ago.