Closed
Bug 281709
Opened 20 years ago
Closed 19 years ago
Under at least Windows XP, shadows are not drawn under menus on first appearance
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
People
(Reporter: Athropos, Assigned: bzbarsky)
References
Details
(Keywords: fixed1.8, regression)
Attachments
(11 files, 9 obsolete files)
34.06 KB,
image/png
|
Details | |
5.98 KB,
text/plain
|
Details | |
4.04 KB,
patch
|
Details | Diff | Splinter Review | |
1.55 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
86.10 KB,
image/png
|
Details | |
1.17 KB,
patch
|
Details | Diff | Splinter Review | |
26.26 KB,
patch
|
pavlov
:
review+
darin.moz
:
superreview-
brendan
:
superreview+
brendan
:
approval1.8b5+
|
Details | Diff | Splinter Review |
6.75 KB,
patch
|
roc
:
review+
roc
:
superreview+
asa
:
approval1.8rc1+
|
Details | Diff | Splinter Review |
3.67 KB,
patch
|
Details | Diff | Splinter Review | |
16.14 KB,
patch
|
Details | Diff | Splinter Review | |
12.02 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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•20 years ago
|
||
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...
Assignee | ||
Comment 3•20 years ago
|
||
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•20 years ago
|
||
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.
Assignee | ||
Comment 5•20 years ago
|
||
Ok... so who actually draws the dropshadows?
I think Windows is doing it in response to some window style we set via Win32.
Assignee | ||
Comment 7•20 years ago
|
||
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
Assignee | ||
Comment 10•20 years ago
|
||
Assignee | ||
Comment 11•20 years ago
|
||
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•20 years ago
|
||
In addition to the shadow not appearing the first time a submenu is displayed, the shadow *never* appears for context menus.
Comment 13•20 years ago
|
||
Comment 14•20 years ago
|
||
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.
Assignee | ||
Comment 15•20 years ago
|
||
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•20 years ago
|
||
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.
Assignee | ||
Comment 17•19 years ago
|
||
> 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...
Blocks: 244366
Comment 18•19 years ago
|
||
(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•19 years ago
|
||
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.
Assignee | ||
Comment 20•19 years ago
|
||
Er... could some win32 widget person tell me why commenting out the XBL handler makes the shadow show up?
Comment 21•19 years ago
|
||
It's not some weird layout reentrancy bug caused by setting the widths then?
Comment 22•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #174098 -
Attachment is obsolete: true
Comment 23•19 years ago
|
||
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.
Assignee | ||
Comment 24•19 years ago
|
||
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?
Depends on: 282359
Comment 25•19 years ago
|
||
(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?
Assignee | ||
Comment 26•19 years ago
|
||
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•19 years ago
|
||
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
Assignee | ||
Comment 28•19 years ago
|
||
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•19 years ago
|
||
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?
Assignee | ||
Comment 30•19 years ago
|
||
Dunno. Someone would have to test....
Comment 31•19 years ago
|
||
The proposed fix for bug 282359 seems to fix this bug, indeed.
Comment 32•19 years ago
|
||
Which one? There are two ;-)
Assignee | ||
Comment 33•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
bz: I wasn't suggesting changing the class, just removing and adding the specific style using GetWindowLong/SetWindowLong.
Comment 36•19 years ago
|
||
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()
Assignee | ||
Comment 37•19 years ago
|
||
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•19 years ago
|
||
Sure. Email would work, as I'm on a business trip right now.
Comment 39•19 years ago
|
||
(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).
Assignee | ||
Comment 40•19 years ago
|
||
Attachment #174013 -
Attachment is obsolete: true
Attachment #174014 -
Attachment is obsolete: true
Assignee | ||
Comment 41•19 years ago
|
||
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.
Assignee | ||
Comment 42•19 years ago
|
||
Attachment #174913 -
Attachment is obsolete: true
Assignee | ||
Comment 43•19 years ago
|
||
Attachment #174915 -
Attachment is obsolete: true
Comment 44•19 years ago
|
||
> 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.
Assignee | ||
Comment 45•19 years ago
|
||
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•19 years ago
|
||
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?
Assignee | ||
Comment 47•19 years ago
|
||
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.
Assignee | ||
Comment 48•19 years ago
|
||
Could someone test this?
Attachment #174940 -
Attachment is obsolete: true
Attachment #174941 -
Attachment is obsolete: true
Comment 49•19 years ago
|
||
> 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•19 years ago
|
||
(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).
Assignee | ||
Updated•19 years ago
|
Assignee: firefox → win32
Component: Menus → Widget: Win32
Product: Firefox → Core
QA Contact: bugzilla → ian
Version: unspecified → Trunk
Assignee | ||
Comment 51•19 years ago
|
||
Nominating for blocking status... I think we really want to get this resolved; otherwise our context menus look really weird....
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
Updated•19 years ago
|
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-
Updated•19 years ago
|
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking1.8b2-
Comment 52•19 years ago
|
||
transferring nomination to 1.8b4. If we don't get it there, we don't get it for 1.1
Flags: blocking-aviary1.1? → blocking1.8b4?
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4-
Comment 53•19 years ago
|
||
*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.
Assignee | ||
Comment 54•19 years ago
|
||
So maybe nsMenuPopupFrame should not show its widget until after it's gotten its initial reflow?
Comment 55•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
Built xulrunner with the patch from attachment 197275 [details] [diff] [review] applied, looks good - all the xul popups have dropshadows again.
Updated•19 years ago
|
Attachment #197275 -
Flags: review?(roc)
Comment on attachment 197275 [details] [diff] [review] Buffer view visibility changes like resizes Looks okay, but it needs some trunk testing.
Attachment #197275 -
Flags: superreview+
Attachment #197275 -
Flags: review?(roc)
Attachment #197275 -
Flags: review+
Comment 59•19 years ago
|
||
Silver: if you're not where you can land this, let me know and I'll get it in today on trunk.
Comment 60•19 years ago
|
||
Comment on attachment 197275 [details] [diff] [review] Buffer view visibility changes like resizes Checked in to trunk.
Comment 61•19 years ago
|
||
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).
Updated•19 years ago
|
Flags: blocking1.8b5- → blocking1.8b5+
Comment 62•19 years ago
|
||
Yes, attachment 197275 [details] [diff] [review] did indeed cause what I described in comment 61. Seamonkey Linux GTK2 build.
Comment 63•19 years ago
|
||
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•19 years ago
|
||
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.
Comment 66•19 years ago
|
||
*** Bug 282059 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 67•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 70•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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.
Comment 75•19 years ago
|
||
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.
Comment 78•19 years ago
|
||
CC'ing James back (please read the last comments).
Comment 79•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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.
Comment 83•19 years ago
|
||
This patch attempts to follow the advice given in the above comments. It does not appear to regress tabbed browsing on GTK 2.
Assignee: win32 → mrbkap
Status: NEW → ASSIGNED
Comment 84•19 years ago
|
||
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=.
Attachment #198391 -
Attachment is obsolete: true
Attachment #198408 -
Flags: review?(pavlov)
Comment 85•19 years ago
|
||
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.
Attachment #198408 -
Flags: review?(pavlov) → review+
Comment 86•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #198408 -
Flags: superreview-
Comment 88•19 years ago
|
||
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
Attachment #198408 -
Flags: superreview+
Attachment #198408 -
Flags: approval1.8b5+
Comment 89•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
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.
Keywords: fixed1.8
Assignee | ||
Comment 92•19 years ago
|
||
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).
Updated•19 years ago
|
Keywords: regression
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.)
Comment 96•19 years ago
|
||
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.
Assignee | ||
Comment 97•19 years ago
|
||
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.
Assignee | ||
Comment 98•19 years ago
|
||
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...
Assignee | ||
Comment 99•19 years ago
|
||
Assignee | ||
Comment 100•19 years ago
|
||
Assignee | ||
Comment 101•19 years ago
|
||
Again, this would replace mrbkap's patch...
Comment 102•19 years ago
|
||
Comment on attachment 200158 [details] [diff] [review] One possible patch this patch works for me after backing out blake's patch
Comment 103•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #200158 -
Flags: review?(roc)
Assignee | ||
Comment 104•19 years ago
|
||
I missed the place that actually mattered, somehow... this should work, I think. Can someone test?
Attachment #200164 -
Attachment is obsolete: true
Attachment #200158 -
Flags: superreview+
Attachment #200158 -
Flags: review?(roc)
Attachment #200158 -
Flags: review+
Updated•19 years ago
|
Flags: blocking1.8rc1+
Comment 105•19 years ago
|
||
Comment on attachment 200205 [details] [diff] [review] Fixed "another approach" patch this patch also works.
Attachment #200205 -
Flags: review?(roc)
Updated•19 years ago
|
Attachment #200158 -
Flags: approval1.8rc1?
Comment 106•19 years ago
|
||
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?
Attachment #200205 -
Flags: superreview+
Attachment #200205 -
Flags: review?(roc)
Attachment #200205 -
Flags: review+
Assignee | ||
Comment 108•19 years ago
|
||
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...
Assignee | ||
Comment 109•19 years ago
|
||
Scott, I think backing the widget change out is the way to go for 1.8, yeah...
Comment 110•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #200158 -
Flags: approval1.8rc1? → approval1.8rc1+
checked in on trunk and branch.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 112•19 years ago
|
||
The backed-out patch caused issues with Cocoa widget (bug 312563). I'll test with the one that roc checked in.
Updated•19 years ago
|
Assignee | ||
Updated•19 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•19 years ago
|
Assignee: mrbkap → bzbarsky
Status: REOPENED → NEW
Assignee | ||
Updated•19 years ago
|
Status: NEW → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Comment 113•8 years ago
|
||
Now it happens under linux with kde/kwin. shadows are missing from context menus and awesome bar popup menu.
Comment 114•8 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•