Closed Bug 353011 Opened 18 years ago Closed 16 years ago

bookmark tooltips appear behind bookmarks menu

Categories

(Core Graveyard :: Widget: OS/2, defect)

x86
OS/2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: stevew, Assigned: mz)

Details

(Keywords: fixed1.8.1.18)

Attachments

(1 file, 1 obsolete file)

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.
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.
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.
But the problem with the menu and tooltip offsets that is probably connected is still there.
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...
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.
It's really erratic, today it also shows again for me in the main bookmarks menu. Weird.
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.
(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?
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? :-)
(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 ... 

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

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.
(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 ? 
Would suggest to forward intermediate discussion to
news:news.mozilla.org/mozilla.dev.ports.os2 ( see thread "bug 353011" ).
Principle similar as under windows. Some necessary corrections to PlaceBehind() ( as proposed in #9 ) and inactivation of SetZIndex().
Attachment #309689 - Flags: review?(mozilla)
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?
Assignee: nobody → mz
Component: Bookmarks → Widget: OS/2
Product: Mozilla Application Suite → Core
Hardware: Other → PC
Target Milestone: --- → mozilla1.9beta5
Version: 1.8 Branch → Trunk
Can this get in for 1.8.1.13 also?
(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.
(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.
Attached patch improved styleSplinter Review
Peter, OK like this ? Still not sure about "} else" ...
Attachment #309689 - Attachment is obsolete: true
Attachment #309689 - Flags: review?(mozilla)
Attachment #309742 - Flags: review?(mozilla)
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.
Attachment #309742 - Flags: review?(mozilla) → review+
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.
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
(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.
(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...
Works well in the PmW build, so I'd say check it in.
OK, done.
Keywords: fixed1.8.1.18
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.