Last Comment Bug 353011 - bookmark tooltips appear behind bookmarks menu
: bookmark tooltips appear behind bookmarks menu
Status: RESOLVED FIXED
: fixed1.8.1.18
Product: Core Graveyard
Classification: Graveyard
Component: Widget: OS/2 (show other bugs)
: Trunk
: x86 OS/2
: -- normal (vote)
: mozilla1.9beta5
Assigned To: Stefan Schmohl
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-16 16:16 PDT by Steve Wendt
Modified: 2014-12-09 11:27 PST (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Simple approach : place on top when Show()n first time. (4.17 KB, patch)
2008-03-15 13:31 PDT, Stefan Schmohl
no flags Details | Diff | Splinter Review
improved style (4.44 KB, patch)
2008-03-16 04:29 PDT, Stefan Schmohl
mozilla: review+
Details | Diff | Splinter Review

Description Steve Wendt 2006-09-16 16:16:30 PDT
In Seamonkey 1.1a, the bookmark tooltips are appearing behind the bookmarks menu.  On the newsgroup, it was reported that trunk builds show the same problem.
Comment 1 Peter Weilbacher 2007-07-01 02:13:24 PDT
Yes, and the tooltips are also offset from the mouse position, and the offset is related to the position of the main window with respect to the top left corner of the screen.

I suspect that this is related to the problem of the offset of popups of menu items that Ilya reported in the newsgroup. We probably missed some patch to widget/src/os2 that was done to the code of the other platforms.
Comment 2 Andy Willis (abwillis) 2007-07-01 08:15:06 PDT
Trunk builds no longer show this problem, not sure if it was the switch to toolkit or the switch to thebes but it is now working correctly on trunk.
Comment 3 Peter Weilbacher 2007-07-01 09:54:52 PDT
But the problem with the menu and tooltip offsets that is probably connected is still there.
Comment 4 Peter Weilbacher 2007-07-17 14:51:26 PDT
Now everything works fine for me on trunk. At least with my own (debug) build Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a7pre) Gecko/200707152020 SeaMonkey/2.0a1pre.

Still no clue about branch. Testing with which nightly it was actually fixed on trunk might help...
Comment 5 Andy Willis (abwillis) 2007-07-18 19:05:20 PDT
My trunk build from the 15th still shows the problem.  If you hover over the main Bookmark dropdown it is fine but if you hover over something in a folder it is underneath.
Comment 6 Peter Weilbacher 2007-07-19 12:05:46 PDT
It's really erratic, today it also shows again for me in the main bookmarks menu. Weird.
Comment 7 Walter Meinl 2007-08-23 11:54:01 PDT
I investigated a little bit, what's going on. It seems to me that we're facing two different problems, the z-Order of the tooltips comment #0 and the offset of the tooltips, respectively also of the context menus.
The trouble making tooltips are only active in Seamonkey, the context menus are in FF and SM. Tooltips were activated on Seamonkey in bug237592 checked in 2006-05-05 and context menus in bug50504 2006-08-06 in the 1_8_Branch. I dowloaded the nearest nightly branch builds (20060508 and 20060809) and can confirm that the tooltips had from the first build the wrong z-Order and the offset problem, the offset problem was also immediately present with the context menus. Therefore, the checkins for seamonkey are unfortunately not the culprit just indicators.
(In reply to comment #1)
> Yes, and the tooltips are also offset from the mouse position, and the offset
> is related to the position of the main window with respect to the top left
> corner of the screen.
> 

Assuming that the offset problem for the context menus and the tooltips comes from the same source I switched to firefox for digging deeper. In the 1.0.x series context menus were activated but worked properly until the last release. However, the first 1.5 build I could get hold of deerpark alpha 1 (20050601) showed already the offset problem for context menus. I finally landed at bug265841, that corrected the offset position of the (main) menus. I checked out the sources before it landed and could see the offset problem with the menus, they are moved to the right and downwards. I applied the patch from this bug and the menus were properly positioned, but the context menus are then shifted to the left (only when the main window is on the right side of the screen).
I played a little bit around with this line of the patch
+         ptl.y = WinQuerySysValue(HWND_DESKTOP, SV_CYSCREEN) - GetHeight( h) - 1 - aY; (for eWindowType_popup) but couldn't find a proper solution, but I think that's should be the place to work on.

Interstingly, the context menus showed newer a wrong z-order problem unlike the tooltips and that's still true for the branch builds (SM-1.1.x and FF-2.0.x).
For the trunk builds the situation changed again with the recent checkin for bug279703. Without any changes to widget/src/os2/nsWindow.cpp the offset problem of the context menus is gone in firefox and seamonkey builds. But now we face the problem that the context menus have also the wrong z-order like the tooltips.

The z-order problem seemed also to be gone at some time during cairo-os2 build break as we can see in the latest builds before the checkin for bug279703 (2007-07-04 checkin date), but now came back with it.
Sorry for the long post, but I hope it helps a bit to unravel it a bit.
Comment 8 Peter Weilbacher 2007-08-23 17:11:16 PDT
(In reply to comment #7)
> I finally landed at
> bug265841, that corrected the offset position of the (main) menus. I checked
> out the sources before it landed and could see the offset problem with the
> menus, they are moved to the right and downwards. I applied the patch from this
> bug and the menus were properly positioned, but the context menus are then
> shifted to the left (only when the main window is on the right side of the
> screen).

So before the patch from bug 265841 the context menus are OK but the normal menus are not, and afterwards it's reversed? Context menus and normal menus share the same mWindowType, both are eWindowType_popup. So there is no (easy) way to handle them differently.

> But now we face the problem that the context menus have also the wrong
> z-order like the tooltips.

That z-order problem I only see with tooltips, not with context menus, at least with trunk.

I just added 
   printf("mWnd=%#08x, mWindowType=%d\n", mWnd, mWindowType);
at the top of nsWindow::Resize. This tells me that tooltips are reused a lot. I see the same mWnd ID for all the tooltips that come up for bookmarks and the personal toolbar for each main SeaMonkey window. They are already created when the main window is created and destroyed with it. But in between they are only moved around and maybe resized to fit the next text. I don't think there is code to get them moved to the top in z-order whenever they are resized. This is different to menus which all get their own unique HWND / mWnd ID.

Z-order changes could be forced with by adding
   // try to force tooltips and other popups to the top after moving them
   if( mWnd && mWindowType == eWindowType_popup)
   {
      WinSetWindowPos( mWnd, HWND_TOP, 0, 0, 0, 0, SWP_ZORDER);
   }
to the bottom of ::Resize, but menus (like the main bookmark menu) are resized again afterwards, and so the effect is not what we want. So, is there some test we could do to see if this mWnd is a tooltip?
Comment 9 Peter Weilbacher 2007-08-26 11:09:42 PDT
More random thoughts that I haven't had a chance to look into in detail:

- I noticed that in 
     nsWindow::PlaceBehind(nsTopLevelWidgetZPlacement aPlacement,
                            nsIWidget *aWidget, PRBool aActivate)
  aPlacement is actually unused on OS/2. Windows has this
     HWND behind = HWND_TOP;
     if (aPlacement == eZPlacementBottom)
       behind = HWND_BOTTOM;
     else if (aPlacement == eZPlacementBelow && aWidget)
       behind = (HWND)aWidget->GetNativeData(NS_NATIVE_WINDOW);
  Do we perhaps need something similar? But then I don't see PlaceBehind
  called from code that is involved with tooltips (or menus)...

- nsXULTooltipListener::ShowTooltip() at 
http://mxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsXULTooltipListener.cpp#391
  seems to be involved in showing the tooltips. That in turn calls 
  nsXULTooltipListener::LaunchTooltip()... Perhaps there is a function that
  one can add there (temporarily) to change the z-order?

Stefan, is this a problem that you would be interested to debug? :-)
Comment 10 Stefan Schmohl 2007-08-26 11:44:51 PDT
(In reply to comment #9)
> Stefan, is this a problem that you would be interested to debug? :-)

Yes, willingly ;-). Will notify you in two or three days whether I get ahead ... 

Comment 11 Walter Meinl 2007-08-28 12:46:55 PDT
(In reply to comment #7)
> I finally landed at
> bug265841, that corrected the offset position of the (main) menus. I checked
> out the sources before it landed and could see the offset problem with the
> menus, they are moved to the right and downwards. I applied the patch from this
> bug and the menus were properly positioned, but the context menus are then
> shifted to the left (only when the main window is on the right side of the
> screen).
Ok, I must admit, I was wrong with this analysis, Peter's patch of bug265841 was correct, it restored the status quo ante meaning that the offset problem was older. I made the mistake to only check the date before the OS/2 patch went in, when the main menus were shifted to the right. Maybe by coincidence the context menus were shifted together with the main menus. When checked out the source some days before the main menu shifting bug was checked in the offset problem was there again.
But now I really found the causing checkin
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/content/events/src&command=DIFF_FRAMESET&file=nsDOMEvent.cpp&rev1=1.171&rev2=1.172&root=/cvsroot
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/content/events/src&command=DIFF_FRAMESET&file=nsDOMEvent.h&rev1=1.75&rev2=1.76&root=/cvsroot 
Bug 242833. Make nsDOMEvent::GetClientX/Y handle cases where a subdocument's widget is not an ancestor of the event's widget. Also forward all mouse grabbing to the root view manager of a view manager hierarchy so that subdocuments can grab the mouse when an event occurs in an outer document.
The added/exchanged code in nsDOMEvent{h.cpp} was shifted to nsDOMUIEvent{h.cpp} with Bug 238773 (Separating nsDOMEvent into separate classes) where is still now in the 1_8_branch.
> For the trunk builds the situation changed again with the recent checkin for
> bug279703. Without any changes to widget/src/os2/nsWindow.cpp the offset
> problem of the context menus is gone in firefox and seamonkey builds. 
This was right of course, but additionally the mentioned code changes in nsDOMUIEvent{h.cpp} were removed from the trunk with bug385536 recently.
So, probably it won't come back for the trunk, but the question remains, if it should be fixed for the branch (as this misbehaves now for more than 3 years and probably was uncovered only because we now have widescreens)

Comment 12 Andy Willis (abwillis) 2007-08-28 12:55:00 PDT
This is slightly different in how it behaves but does still occur in Trunk if you have folders.  If you hover over a bookmark entry in the main bookmark list it is fine, if however, you open a folder and hover over an entry in it the tooltip is behind the folder pop-out.
Comment 13 Stefan Schmohl 2007-08-28 13:52:41 PDT
(In rely to comment #11 and comment #12)

This matches to what I can see here. The misbehavior still exists and the origin seems to reside in the cross-platform "nsView" classes ( "nsView::UpdateNativeWidgetZIndexes()" > "nsBaseWidget::SetZIndex()" > "nsWindow::PlaceBehind()" ). 
Does anyone knows whether this bug also applies for other platforms ? 
Comment 14 Stefan Schmohl 2007-08-30 07:14:01 PDT
Would suggest to forward intermediate discussion to
news:news.mozilla.org/mozilla.dev.ports.os2 ( see thread "bug 353011" ).
Comment 15 Stefan Schmohl 2008-03-15 13:31:29 PDT
Created attachment 309689 [details] [diff] [review]
Simple approach : place on top when Show()n first time.

Principle similar as under windows. Some necessary corrections to PlaceBehind() ( as proposed in #9 ) and inactivation of SetZIndex().
Comment 16 Peter Weilbacher 2008-03-15 14:34:50 PDT
This is really cool! In a short test session I could not make tooltips of context menus appear behind something. I will watch it a few days so that we can still get it in in time for FF beta 5 if I don't see any regressions.

Just to note (I'm apparently already known to be picky on such little things): I have been trying to enforce a single style in nsWindow, based on the first line that has been there forever, at least for changes. So I think we should use the chance to get ::Show() to match that style (2 spaces indents, no spaces after opening brackets or before closing brackets, opening curly brackets at line ends). I can do that when checking in.

Btw, ::IsVisible is strange, why is that PRBool not passed in as a pointer but like a class?
Comment 17 Steve Wendt 2008-03-15 16:13:00 PDT
Can this get in for 1.8.1.13 also?
Comment 18 Stefan Schmohl 2008-03-15 18:59:39 PDT
(In reply to comment #16)
Back again ...
> Just to note (I'm apparently already known to be picky on such little 
> things): ...
Really welcome ! I would have done so, if I had known which style I should apply ;-). Will change that soon.
> Btw, ::IsVisible is strange, why is that PRBool not passed in as a pointer 
> but like a class?
Hmm, think just like other style issues : some don't bother. And even if passing as a reference - so far I know a relative late feature of C++ ( working like a pointer but no need to dereference symbol and no danger of accidentally changing the address ) - may have advantages in some cases, here I also can't see any need to diverge from the original style.
Comment 19 Peter Weilbacher 2008-03-15 23:39:10 PDT
(In reply to comment #18)
> Hmm, think just like other style issues : some don't bother. And even if
> passing as a reference - so far I know a relative late feature of C++ ( working
> like a pointer but no need to dereference symbol and no danger of accidentally
> changing the address ) - may have advantages in some cases, here I also can't
> see any need to diverge from the original style.

Well, that was just a remark on the side as you didn't touch that function.

(In reply to comment #17)
> Can this get in for 1.8.1.13 also?
It's a bit late for that already, will maybe get in my builds to get more testing. Then it can go into .14.
Comment 20 Stefan Schmohl 2008-03-16 04:29:24 PDT
Created attachment 309742 [details] [diff] [review]
improved style

Peter, OK like this ? Still not sure about "} else" ...
Comment 21 Peter Weilbacher 2008-03-16 05:27:35 PDT
Comment on attachment 309742 [details] [diff] [review]
improved style

Yes, that's fine now, thanks. The else looks ugly either way, so let's leave it like that.
Comment 22 Peter Weilbacher 2008-03-16 06:35:40 PDT
I also wanted to comment that while the code changes seem OK to me, I yesterday had a problem when trying to drag an URL to the tab-bar of another SeaMonkey window, I got pretty garbled display in that other window. As that only happened once, about 15 minutes after I had started up the build with the patch included but never before, I thought this might be connected (although I don't know how). It hasn't happened since then, but perhaps something to watch out for.
Comment 23 Peter Weilbacher 2008-03-18 02:40:12 PDT
Checked into trunk. Thanks again for your work, Stefan, this is really great!

(Stefan, you'll notice that I made small whitespace changes for the checkin, it occurred to me that that the else with extra brackets would look nicer and I stepped on a rule which said don't compare to PR_TRUE.)

I also just started the nightly build process, so the next set of FF, SM, and TB -- all of which are now available from ftp.mozilla.org -- will have this fix.
Comment 24 Stefan Schmohl 2008-03-18 04:11:48 PDT
(In reply to comment #23)
Fine ! Also looked whether we could change the needless "reference passing" style of "IsVisible" to common "pointer passing" style. Unfortunately, we can't - it's used in nsViewManager.cpp.
Comment 25 Peter Weilbacher 2008-09-13 08:57:32 PDT
(In reply to comment #19)
> (In reply to comment #17)
> > Can this get in for 1.8.1.13 also?
> It's a bit late for that already, will maybe get in my builds to get more
> testing. Then it can go into .14.

I had completely forgotten about this, just found the patch again in my patch directory for my PmW builds. Is this still wanted or has anyone heard complaints because of this? If not, I can at least get it into 1.8.1.18...
Comment 26 Steve Wendt 2008-09-13 11:05:41 PDT
Works well in the PmW build, so I'd say check it in.
Comment 27 Peter Weilbacher 2008-09-13 15:51:13 PDT
OK, done.

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