Under at least Windows XP, shadows are not drawn under menus on first appearance

RESOLVED FIXED

Status

()

Core
Widget
RESOLVED FIXED
13 years ago
8 months ago

People

(Reporter: Francois Ingelrest, Assigned: bz)

Tracking

({fixed1.8, regression})

Trunk
fixed1.8, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b3 -
blocking1.8b5 +
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 9 obsolete attachments)

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+
Details | Diff | Splinter Review
86.10 KB, image/png
Details
1.17 KB, patch
Details | Diff | Splinter Review
26.26 KB, patch
Stuart Parmenter
: review+
Darin Fisher
: superreview-
brendan
: superreview+
brendan
: approval1.8b5+
Details | Diff | Splinter Review
6.75 KB, patch
roc
: review+
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+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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

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

Comment 8

13 years ago
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

13 years ago
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
Created attachment 174013 [details] [diff] [review]
Random hack #1 to test
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

13 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

13 years ago
Created attachment 174043 [details]
Image of shadows missing from sub menu and context menu
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.
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.
> 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

Updated

13 years ago
Blocks: 282059
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.
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?

Comment 21

13 years ago
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.

Updated

13 years ago
Attachment #174098 - Attachment is obsolete: true
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?
Depends on: 282359

Comment 25

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

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

13 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?
Dunno.  Someone would have to test....
The proposed fix for bug 282359 seems to fix this bug, indeed. 

Comment 32

13 years ago
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.

Comment 35

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

Comment 36

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

13 years ago
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).
Created attachment 174913 [details] [diff] [review]
Random hack #3 to test
Attachment #174013 - Attachment is obsolete: true
Attachment #174014 - Attachment is obsolete: true
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.
Created attachment 174940 [details] [diff] [review]
Random hack #3 that might compile
Attachment #174913 - Attachment is obsolete: true
Created attachment 174941 [details] [diff] [review]
Random hack #4 that might compile
Attachment #174915 - Attachment is obsolete: true

Comment 44

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

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

13 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?
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.
Created attachment 175341 [details] [diff] [review]
Random hack #5 -- now with Get/SetClassLong

Could someone test this?
Attachment #174940 - Attachment is obsolete: true
Attachment #174941 - Attachment is obsolete: true

Comment 49

13 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);
(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: firefox → win32
Component: Menus → Widget: Win32
Product: Firefox → Core
QA Contact: bugzilla → ian
Version: unspecified → Trunk
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

12 years ago
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-

Updated

12 years ago
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking1.8b2-

Comment 52

12 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

12 years ago
Flags: blocking1.8b4? → blocking1.8b4-

Comment 53

12 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.
So maybe nsMenuPopupFrame should not show its widget until after it's gotten its
initial reflow?

Comment 55

12 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

12 years ago
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

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

Updated

12 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+
Silver: if you're not where you can land this, let me know and I'll get it in
today on trunk.

Comment 60

12 years ago
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).

Updated

12 years ago
Flags: blocking1.8b5- → blocking1.8b5+
Yes, attachment 197275 [details] [diff] [review] did indeed cause what I described in comment 61.
Seamonkey Linux GTK2 build.

Comment 63

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

Updated

12 years ago
Depends on: 310483
I'm also seeing comment 61.

Updated

12 years ago
No longer blocks: 282059

Comment 66

12 years ago
*** 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...).

Comment 68

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

12 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

12 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

12 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

12 years ago
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.

Comment 81

12 years ago
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.
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.
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.
Assignee: win32 → mrbkap
Status: NEW → ASSIGNED
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=.
Attachment #198391 - Attachment is obsolete: true
Attachment #198408 - Flags: review?(pavlov)

Comment 85

12 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

12 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

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

12 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!
(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.
Keywords: fixed1.8
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

12 years ago
Keywords: regression
It sounds like this may have caused bug 311146.
Depends on: 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

12 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.
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.
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...
Created attachment 200159 [details] [diff] [review]
Same as diff -w
Created attachment 200163 [details] [diff] [review]
backout of mrbkap's patch
Created attachment 200164 [details] [diff] [review]
Another approach

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

Comment 102

12 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

12 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

12 years ago
Attachment #200158 - Flags: review?(roc)
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?
Attachment #200164 - Attachment is obsolete: true
Attachment #200158 - Flags: superreview+
Attachment #200158 - Flags: review?(roc)
Attachment #200158 - Flags: review+

Updated

12 years ago
Flags: blocking1.8rc1+

Comment 105

12 years ago
Comment on attachment 200205 [details] [diff] [review]
Fixed "another approach" patch

this patch also works.
Attachment #200205 - Flags: review?(roc)

Updated

12 years ago
Attachment #200158 - Flags: approval1.8rc1?

Comment 106

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

Comment 110

12 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

12 years ago
Attachment #200158 - Flags: approval1.8rc1? → approval1.8rc1+
checked in on trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 112

12 years ago
The backed-out patch caused issues with Cocoa widget (bug 312563). I'll test
with the one that roc checked in.
Component: Widget: Win32 → Widget
Depends on: 312563
OS: Windows XP → All
Hardware: PC → All

Updated

12 years ago
Blocks: 312563
No longer depends on: 312563
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: mrbkap → bzbarsky
Status: REOPENED → NEW
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 113

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

Comment 114

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