391421 - OS/2 submenu highlight offset of pointer

Status: RESOLVED FIXED

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

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a8pre) Gecko/2007080820 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a8pre) Gecko/2007080820 Minefield/3.0a8pre

As reported in the newsgroup the mouse pointer and submenu highlighting are out of sync. This regards the main browser window as well as the bookmarks manager FF and SM (maybe also TB, but no report yet)
I figured out that that's a fallout of bug 388359. Reverting the changes made in that bug brings the mouse pointer back in sync. It seems that the coordinates are changed as when you move the windows the highlighting of a menu entry seems to be fixed, i. e. when you move the window up, you get in sync, when you move the window down the offset grows. Probably a fix of widget/src/os2/nsWindow.cpp would help?

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Karl Tomlinson (:karlt)]

From an inspection of WidgetToScreen and ScreenToWidget in os2/nsWindow,
it looks like increasing screen y coordinates moves up, which is different to our widget coordinates and (at least some) other platforms.  I wonder if this is causing trouble.

Comment 2

[Karl Tomlinson (:karlt)]

Actually it looks like WinMapWindowPoints is invoked with our widget coordinates.  That seems inconsistent.  Is it our widget coordinates with increasing y moving up on OS/2?

Comment 3

[Stefan Schmohl]

Yes, WinMapWindowPoints is working in coordinates refering to the left bottom of the screen. 
But in "WidgetToScreen( )" it seems to be correctly encapsulated by "NS2PM( )" and "WinQuerySysValue( HWND_DESKTOP, SV_CYSCREEN )" to yield the proper screen values measured from left top. ( Also checked that by piping stdout of "printf( )" to a display and watching the values while using seamonkey. ) "ScreenToWidget( )" on the other hand apparently never gets called.

And so far I can see it, in "layout/base/nsLayoutUtils.cpp" - "TranslateWidgetToView( )", there is always a common root of 
1. the transfered Widget "aWidget" and
2. the "GetNearestWidget( )" of the transfered View "aView".
Also, the subtrahend in the return line is always zero. 
So the shift in the coordinates is solely calculated from the offsets to the common root ( by means of "GetWidgetOffset( )" ).
And here something is going wrong ...

Hope this can help to understand what is going on.
 

Comment 4

[Stefan Schmohl]

Attachment: Could be a temporary workaround

A common root widget ( fromRoot == toRoot ) seems to be always found. But, if the pointer is over a context/sub menu, fromOffset is relative to screen and toOffset relative to canvas. So the method fails.
On the other hand, deducing the shift by "WidgetToScreen( )" - "ScreenToWidget( )" is apparently reliable.

Comment 5

[Stefan Schmohl]

Attachment: nsLayoutUtils_cpp.diff - v2

"#ifdef"ed to be platform specific

Comment 6

[Walter Meinl]

(In reply to comment #5)
> Created an attachment (id=276135) [details]
> nsLayoutUtils_cpp.diff - v2
Just to confirm, this works here for SM, TB, FF and FF on Top of XULRunner
though it doesn't look very nice. Is it an OS/2 feature that we count differently the point of origin of the screen or was it defined such somewhere in widget/src/os2/nsWindow.cpp ?

Comment 7

[Peter Weilbacher]

That the y coordinates are reversed in os2/nsWindow is due to OS/2 PM, we have to use it when drawing and converting mouse coordinates, etc.

I now stared at the GetWidgetOffset() / TranslateWidgetToView() changes for quite a while and cannot figure out what is going wrong. The offsets seem to be calculated correctly. The only thing I noticed when adding a few debug printfs is that viewOffset is always 0,0 at the end of TranslateWidgetToView().

Comment 8

[Stefan Schmohl]

(In reply to comment #6)
> > nsLayoutUtils_cpp.diff - v2
> Just to confirm, this works here for SM, TB, FF and FF on Top of XULRunner
> though it doesn't look very nice. Is it an OS/2 feature that we count
> differently the point of origin of the screen ...

I'm not sure. However, I think there should be no need for platform-dependent
handling in "TranslateWidgetToView( )". There is something wrong upstream of
nsIWidget::GetBounds( nsIRect bounds ). The submenu "nsIWidget"-instance
reports "bounds" with a ".TopLeft( )" relative to screen - which should
probably be relative to canvas. 
I have a lot to learn ( know nothing about the architecture up to now ), but I
will try to find the reason the next 2 days.

Comment 9

[Peter Weilbacher]

Hmm, it also never happens that fromRoot != toRoot. I wonder if that is because the desktop window (HWND_DESKTOP) is always the topmost parent of all widgets on OS/2 (I remember that we stumbled on that about two years ago in a bug that I don't find right now). But nsWindow::GetParent() should never return the desktop window (and I checked that it doesn't).

(In reply to comment #8)
> There is something wrong upstream of
> nsIWidget::GetBounds( nsIRect bounds ). The submenu "nsIWidget"-instance
> reports "bounds" with a ".TopLeft( )" relative to screen - which should
> probably be relative to canvas. 

It seems to me that mBounds is always computed relative to the parent in nsWindow and in cross-platform coordinates. But yes, please check that.

Comment 10

[Peter Weilbacher]

(In reply to comment #9)
> I wonder if that is because
> the desktop window (HWND_DESKTOP) is always the topmost parent of all widgets
> on OS/2 (I remember that we stumbled on that about two years ago in a bug that
> I don't find right now).

OK, so this was bug 268090 and bug 270981 but that was about finding children so it is probably unrelated.

Comment 11

[Stefan Schmohl]

(In reply to comment #9)
> ... But nsWindow::GetParent() should never return the
> desktop window (and I checked that it doesn't).
Are the "nsIWidget::GetParent( )" and "nsIWidget::GetBounds( )" methods inherited from "nsWindow" ?

By the way - is there any method to efficiently "walk" along the object hierarchies. E.g., when I try to find the definition of "nsIWidget::GetBounds( )" by means of "grep" or "lxr" I have to manually filter out a lot of instances and declarations. Pretty time consuming ...
 
> It seems to me that mBounds is always computed relative to the parent in
> nsWindow and in cross-platform coordinates. But yes, please check that.

This is also what I've expected from it. So in case of a submenu I should be able to observe ( by means of "printf"s ) the offset from the submenu to the canvas while walking up the "GetParent( )" hierarchie in "GetWidgetOffset( )". Or more than one offset adding to that distance.
What I can find are some (0,0)-offsets and one offset matching the distance from the left top of the submenu to the left top of the screen.

"widget/public/nsIWidget.h" says about it : "widget outside coordinates in global coordinates". Makes things not clearer for me ...
  

Comment 12

[Peter Weilbacher]

(In reply to comment #11)
> Are the "nsIWidget::GetParent( )" and "nsIWidget::GetBounds( )" methods
> inherited from "nsWindow" ?

The other way around, nsWindow has an implementation of nsIWidget::GetParent. (nsWindow inherits nsBaseWidget which is a subclass of nsIWidget.)

> By the way - is there any method to efficiently "walk" along the object
> hierarchies.

I haven't really found any that I am more happy with.

Comment 13

[Stefan Schmohl]

Would be nice if someone could answer to the following questions :

Mouse events should have coordinates relative to the "nsView" origin, except for mouse positions over popup ( = sub = context ) menus, which should have coordinates relative to the screen origin, right ?
And "mBounds" of a Widget should be relative to the "GetParent( )" widget, except for popup menu widgets ( eWindowType_popup ), which should be relative to the screen origin, also right ?

Comment 14

[Eli Friedman]

Mouse events should have coordinates relative to the widget they fire against, no exceptions.  GetBounds should return coordinates relative to the parent widget, no exceptions.

Comment 15

[Andy Willis (abwillis)]

I can reduce the number of #ifndef to:
static nsPoint GetWidgetOffset(nsIWidget* aWidget, nsIWidget*& aRootWidget) {
  nsPoint offset(0, 0);
  nsIWidget* parent = aWidget->GetParent();
#ifndef XP_OS2
  while (parent) {
    nsRect bounds;
    aWidget->GetBounds(bounds);
    offset += bounds.TopLeft();
    aWidget = parent;
    parent = aWidget->GetParent();
  }
#endif
  aRootWidget = aWidget;
  return offset;
}

Which makes me wonder, how if offset is apparently good when it starts, how can adding bounds.TopLeft() to it any number of times do anything but throw it off.  
Changing offset += bounds.TopLeft(); to offset = bounds.TopLeft(); made it where it wasn't nearly as far off (one line instead of several inches).
Changing GetBounds() to GetClientBounds() got it closer but not as close as the above line got it (maybe as close up and down but threw off it off to the right).

Comment 16

[Stefan Schmohl]

(In reply to comment #14)
> Mouse events should have coordinates relative to the widget they fire 
> against, no exceptions.
  
Thank you for your reply ! Sounds straight forward. So mouse coordinates seems to be all right on Os2. Unfortunately, mBounds not. 

> GetBounds should return coordinates relative to the parent widget, no 
> exceptions.
On Os2, when a popup is displayed, "nsView::DoResetWidgetBounds( )" is called, this accesses "nsView::CalcWidgetBounds( )" which in turn returns the bounds in screen coordinates. ( Then "nsWindow::Resize()" sets the "mBounds" of the widget, "nsWidget::GetBounds( )" inside of "nsLayoutUtils::GetWidgetOffset( )" gets the values, resulting in a falsely deduced offset in "nsLayoutUtils::TranslateWidgetToView( )". )
Problem is, I don't understand why that works for other platforms. Is there no "GetParent( )", no "parentWidget", is "aType" != "eWindowType_popup" | "mVis" != "nsViewVisibility_kShow", or are other platforms even able to deal with the screen coordinates ?
 
Would be nice if you could take a look at http://lxr.mozilla.org/seamonkey/source/view/src/nsView.cpp#339 .

Thanks in advance

Comment 17

[Eli Friedman]

Here's the implementation of nsWindow::GetBounds on Windows:
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#2282. Note that this has no relationship with the coordinates passed into the widget from the view code.

Yes, this is a confusing/sucky setup... patches welcome, I guess, but I'd suggest just fixing the OS/2 implementation for now.

Comment 18

[Stefan Schmohl]

(In reply to comment #15)
> I can reduce the number of #ifndef to:
> static nsPoint GetWidgetOffset(nsIWidget* aWidget, nsIWidget*& aRootWidget) {
>   nsPoint offset(0, 0);
>   nsIWidget* parent = aWidget->GetParent();
> #ifndef XP_OS2
> ...
> #endif
>   aRootWidget = aWidget;
>   return offset;
> }
> 
> Which makes me wonder, how if offset is apparently good when it starts, how can
> adding bounds.TopLeft() to it any number of times do anything but throw it off. 
> ...
As a result the returned parentWidget isn't the root. And so inside of "TranslateWidgetToView( )", "fromRoot" is nearly never "toRoot" and the offset is calculated by means of "WidgetToScreen( )" - "ScreenToWidget( )" - which is doing it all right ...

Comment 19

[Peter Weilbacher]

Attachment: Revert bug 286555

Huh, reverting the patch from bug 286555 (attachment 200215 [details] [diff] [review]) seems to work. Like this patch...

If that works for the others I guess now we have to build Thunderbird to see if that other bug is back...

Comment 20

[Peter Weilbacher]

OK, TB is still broken when reverting the old patch. So I guess we have to make sure that we also set mBounds to the correct values inside the if (mWnd) branch. Not sure how...

Comment 21

[Stefan Schmohl]

(In reply to comment #20)
> OK, TB is still broken when reverting the old patch. 
Anyway, fine to have reached the platform-specific code.

> OK, TB is still broken when reverting the old patch. So I guess we have to make
> sure that we also set mBounds to the correct values inside the if (mWnd)
> branch. Not sure how...
Fortunately, "mBounds" is protected. So we can try to maintain it in a consistent manner. And handle the exceptions in the few functions accessing "mBounds" ( like RealDoCreate, SetBounds, GetBounds, Resize ).

Comment 22

[Walter Meinl]

(In reply to comment #19)
> Created an attachment (id=277445) [details]
> Revert bug 286555
> 
> Huh, reverting the patch from bug 286555 (attachment 200215 [details] [diff] [review]) seems to work.
Just for curiosity as I'm in the moment trying to find the cause for bug353011 that seems to be related (at least for the offset of the context-menus and tooltips) to this bug here. Does reverting of bug 286555 help in this regard?

Comment 23

[Peter Weilbacher]

> Just for curiosity as I'm in the moment trying to find the cause for bug 353011
> that seems to be related (at least for the offset of the context-menus and
> tooltips) to this bug here. Does reverting of bug 286555 help in this regard?

Well, when I reverted the patch I didn't see that problem any more but that doesn't say much, as I am not seeing this in a consistent manner on trunk anyway.

Comment 24

[Peter Weilbacher]

Attachment: Work around bug and don't destroy TB

This is what I think we could do. The Thunderbird panes are of type eWindowType_child, so setting mBounds upfront for those circumvents the TB problem of bug 286555. And it still seems to correct the problem of this bug.

But I cannot really say that I understand why eWindowType_popup (which I think is the type of the windows/menus that we see the problem for) are treated specially in ::Resize().

Comment 25

[Stefan Schmohl]

Attachment: added reference to this bug to Peter's patch

(In reply to https://bugzilla.mozilla.org/show_bug.cgi?id=391421#c24)

Works here flawlessly for 2007-08-22 5:00 seamonkey trunk. ( Wouldn't have thought this could be fixed with a few lines. )

Would suggest adding a comment referring to this bug ( see attachement ).

I think the special treatment of "eWindowType_popup" is because the platform-independent code uses coordinates relative to screen - and not to the parent - as an exception to all other types. So we can't invoke "WinMapWindowPoints( parentWin, thisWin, &pointToModify, 1 )" to get the needed relative shift "ptl" we can apply for "SetWindowPos( 0, x, y, w, h, SWP_MOVE | SWP_SIZE )". But I'm also not really sure ...

Comment 26

[Peter Weilbacher]

But Eli says (in comment 14) "GetBounds should return coordinates relative to the parent widget, no exceptions." and he knows this stuff. So eWindowType_popup should not be more special than other types just that it has the whole screen as "parent" and not something else. That should be the same for eWindowType_toplevel and probably also eWindowType_dialog...

About the patch, I think you should stick to the (bracketing) style that is used within that function (even if it is ugly), use || instead of |. Is mWindowType always defined, even if we don't have an mWnd? I would try this:

if( !mWnd || ( mWnd && mWindowType == eWindowType_child))

Also mWind -> mWnd... And while one should normally not mix stuff, nsWindow is such a mess and a global cleanup will probably never happen, so we should take every chance to remove rubbish on a small scale... Please also remove the already commented NS_ASSERTIONs at the top.

Comment 27

[Stefan Schmohl]

Attachment: Peter's patch without my mistakes

(In reply to comment #26)

Since my suggestion doesn't change the logic of your patch, just the way it is written, first question : should we cancel it ? Please feel free to mark it as obsolete. 

> But Eli says (in comment 14) "GetBounds should return coordinates relative to
> the parent widget, no exceptions." and he knows this stuff. So
> eWindowType_popup should not be more special than other types just that it has
> the whole screen as "parent" and not something else.

Yes, but ... - please take a look at http://lxr.mozilla.org/seamonkey/source/view/src/nsView.cpp#339 . This is definitely an exception of the rule anchored in the cross-platform code. On the other hand, the cross-platform code expects values without exception from "GetBounds( )". E.g., in http://lxr.mozilla.org/seamonkey/source/layout/base/nsLayoutUtils.cpp#660 .

For Windows, this issue is solved by transferring the values inside of "Resize( )" without exception to "mBounds" ( see http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#2174 ) but handling exceptions inside of "GetBounds( )" ( see http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#2282 ).

Eli called that "confusing/sucky" ( #17 ). I agree in that I'm a little bit confused. ;-)

> That should be the same
> for eWindowType_toplevel and probably also eWindowType_dialog...

OK, the criterion for Windows is "GetParent( mWnd )", we have a different one. I'm far from overlooking the implications. Just know : it works so far ...

> About the patch, ...

Thank you for your hints. I've corrected the issues. "mWindowType" is defined ( in the "nsBaseWidget" constructor ) and without meaning for "!mWnd == TRUE" in this "logical or" condition so I kept it for brevity.

Comment 28

[Peter Weilbacher]

Comment on attachment 277913 [details] [diff] [review]
Peter's patch without my mistakes

OK, I think you could remove the extra brackets on the if line and remove the space before the first one to be consistent with the style.
And you should also only indent by three spaces, at least in this function. ;-) (Sorry, didn't notice this in your first patch.)

But let's get a review from Mike about it first, then I can change that on checkin.

Comment 29

[Stefan Schmohl]

(In reply to comment #28)
> OK, I think you could remove the extra brackets on the if line and remove the
> space before the first one to be consistent with the style.
> And you should also only indent by three spaces, at least in this function. ;-)

Thank your for your patience. I didn't even notice that. And every advice is highly appreciated !

Comment 30

[Mike Kaply [:mkaply]]

Comment on attachment 277913 [details] [diff] [review]
Peter's patch without my mistakes

r=mkaply

does this help the tooltip positioning problem at all?

Comment 31

[Stefan Schmohl]

(In reply to comment #30)
> does this help the tooltip positioning problem at all?

Sorry, I can't see any tooltip positioning problem nor with neither without the patch. Seamonkey build from trunk, comitted by date 2007-08-22 5:00 PDT. 
However, the z-index bug353011 still persists.

Comment 32

[Peter Weilbacher]

Patch checked into trunk. (While doing that I also wrapped the comments to about 80 chars per line.)

Many thanks for the help, Stefan! I hope we'll see more patches from you in the future. :-)

Mike, on my setup it seems that the tooltip position is OK (but I have said that before), but the z-order is not. Let's discuss this more in bug 353011.