Closed Bug 135079 Opened 23 years ago Closed 21 years ago

On multiple/dual/secondary monitor setup, highlighting/hover on items broken

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.7final

People

(Reporter: kvo--bugzilla, Assigned: jsfbbz)

References

Details

(Keywords: fixed1.7, Whiteboard: patch ready to land. mail sent to drivers asking for landing help)

Attachments

(4 files, 1 obsolete file)

fix mouse position handling under Windows
494 bytes, patch
emaijala+moz
: review-
dbaron
: superreview+
Details | Diff | Splinter Review
fix mouse position handling under Windows
5.27 KB, patch
Details | Diff | Splinter Review
Update macro definitions as per comment 69
5.54 KB, patch
brendan
: review+
dbaron
: superreview+
brendan
: approval1.7+
Details | Diff | Splinter Review
the patch for the 1.7 branch
6.36 KB, patch
Details | Diff | Splinter Review
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9+) Gecko/20020402 BuildID: 2002040203 On a dual monitor setup, when the secondary monitor is to the left of the primary screen (setup [2] [1]), there is a problem with the highlighting of the menus when the browser is on the secondary monitor. Reproducible: Always Steps to Reproduce: 1. Move browser to screen [2] which is on the left of [1] 2. Hover the mouse over a menu-item Actual Results: The menu item is highlighted for a short time and then deactivated again, until the mouse moves again. Expected Results: The menu item should remain highlighted as long as the pointer is over the item. This problem also affects links in the browser window. Normally hovering over a link, shows the URL in the status bar. In this case the 'Document: Done' is shown again after a short time until the mouse is moved again. This might be related to Bug #127575
Same thing, Windows 2000 or XP, dual monitors, build 2002031104, classic or modern skins. This may be related to script/CSS bug where the background properties flicker off after showing briefly. Bug# 134547
*** Bug 134548 has been marked as a duplicate of this bug. ***
-> XP APPS and new based on the dupe
Assignee: Matti → sgehani
Status: UNCONFIRMED → NEW
Component: Browser-General → XP Apps
Ever confirmed: true
QA Contact: imajes-qa → paw
-> Menus for a look
Assignee: sgehani → hyatt
Component: XP Apps → XP Toolkit/Widgets: Menus
QA Contact: paw → shrir
I believe this bug is a dup of Bug 130527. But I think both are related to the more general problem seen in Bug 134547. -Ellis
Additional information. If you navigate the menus on the extended monitor with the keyboard instead of the mouse the flicker does not occur and you get normal expected behavior. -Ellis
*** Bug 127956 has been marked as a duplicate of this bug. ***
This problem affects Mozilla Firebird as well. Tested with latest nightly (2003-06-11).
I get the flickering hover behavior on web pages themselves using :hover in CSS. Again, only when the browser is on the 2nd monitor. Mozilla 1.4rc1, Windows XP SP1 here. nVidia GeForce 440 Go.
*** Bug 219640 has been marked as a duplicate of this bug. ***
*** Bug 201091 has been marked as a duplicate of this bug. ***
*** Bug 210869 has been marked as a duplicate of this bug. ***
*** Bug 197091 has been marked as a duplicate of this bug. ***
*** Bug 205072 has been marked as a duplicate of this bug. ***
Confirmed on : XP Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 Seems that it only happen on the second monitor. Try to set them both to the same resolution, but still doesn't work on the second.
It also affects the Mozilla Mail.
I think all these bugs are the same: bug 135079, bug 141599, bug 154163, bug 159277, bug 222817. I'm putting this in all the bugs in the hopes one of the 5 different owners of these 5 different bugs will fix it and resolve the rest as dupes; I don't know if I have permission to resolve/dupe myself and I don't know which one is most likely to get attention first :)
Summary: On multiple monitor setup, highlighting on items broken → On multiple/dual/secondary monitor setup, highlighting/hover on items broken
*** Bug 222682 has been marked as a duplicate of this bug. ***
*** Bug 222817 has been marked as a duplicate of this bug. ***
*** Bug 229428 has been marked as a duplicate of this bug. ***
Don't know if this will help.. From the looks of it this bug only occurs when the application is placed in at negative desktop coordinates (not sure if the coordinates actually is negative though). The bug will occur whenever the application is placed on the secondary monitor, -if- the secondary monitor is placed within the 180 degrees of 225 - 45 (top-right, top, top-left, left, bottom-left) of the primary monitor. Can place half the application in the negative coordinates, and the other half in the positive coordinates, and the hover effects on the positive half will work as intendended, and the other half will "flicker", ie. it won't work as intended. Versions used. Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7 Mozilla Thunderbird 0.3 (20031013)
*** Bug 222283 has been marked as a duplicate of this bug. ***
*** Bug 224010 has been marked as a duplicate of this bug. ***
*** Bug 228222 has been marked as a duplicate of this bug. ***
*** Bug 230512 has been marked as a duplicate of this bug. ***
*** Bug 154163 has been marked as a duplicate of this bug. ***
Sorry, but this is not only a dualhead bug... It often occours without running dualhead.. And as i wrote in a similair bug as this one (dup?): This is a *critical* bug, since it will make sites with javascripts depending on onmouseover/onmouseout triggers not work. Wich is most of javascript menus, making navigation of many sites not working with this.. For instance on my site i have added a disclaimer for Mozilla users about this, i will NOT make a work around for something in Javascripts that is obviously a browser bug, a simple sollution is to switch browser or restart it until the bug isnt shown anymore.. (will work for a while).
*** Bug 222947 has been marked as a duplicate of this bug. ***
Severity: normal → major
*** Bug 233861 has been marked as a duplicate of this bug. ***
I can confirm this. It only happens for me when the primary display is to the right of the secondary display. When I switch them in the Display->Settings menu, the problem goes away. It may be a bug with determining the mouse location.
*** Bug 235221 has been marked as a duplicate of this bug. ***
*** Bug 148676 has been marked as a duplicate of this bug. ***
I can confirm this bug on third monitor as well ... again only when it is placet to the left from the primary one.
*** Bug 235546 has been marked as a duplicate of this bug. ***
Blocks: multimon-win
*** Bug 159277 has been marked as a duplicate of this bug. ***
*** Bug 236745 has been marked as a duplicate of this bug. ***
OS: Windows 2000 → All
Hardware: PC → All
I am also getting this bug, and I can confirm that it occurs only when the secondary display is on the left of the primary. Firefox 0.8, WinXP.
I can't reproduce this on Linux. Should this really be marked All/All?
(In reply to comment #39) > I can't reproduce this on Linux. Should this really be marked All/All? Does Linux/X have negative Coordinates with multimonitor setups? I didn't try everything, but as far as I remember it moved everything right/down resulting in everything-positive coordinates... [not sure on this]
IE 6 has a similar problem. Perhaps it's a windows API issue?
IE6 has no such problem on XP (just checked), only Mozilla-based apps (from my experience).
Re: comment 41, I've never seen this behaviour in IE of any version, but the prod did make me think of one circumstance that will generate the problem. I downloaded the firefox 0.8 sources and had a prod around, and found two relevant occurances of the GetMessagePos function, in mozilla/widget/src/windows/nsToolkit.cpp and mozilla/widget/src/windows/nsWindow.cpp. nsWindow.cpp says (line 985): // get the message position in client coordinates and in twips DWORD pos = ::GetMessagePos(); POINT cpos; cpos.x = (short)LOWORD(pos); cpos.y = (short)HIWORD(pos); if (mWnd != NULL) { ::ScreenToClient(mWnd, &cpos); event.point.x = cpos.x; event.point.y = cpos.y; This is correct (though I'd prefer to see the MSDN Library formulation using MAKEPOINTS instead of casting the return of {LO,HI}WORD. GetMessagePos() and GetCursorPos() return screen coordinates, 16 bits per axis, which can be negative. The (eventual) cast (which is a reinterpret_cast here) to short ensures values with the top bit set are treated as such even though the intermediate value is an unsigned 32 bit combination. However, nsToolkit.cpp says (line 1022): POINT mp; DWORD pos = ::GetMessagePos(); mp.x = LOWORD(pos); mp.y = HIWORD(pos); if (::WindowFromPoint(mp) != mHoldMouse->GetWindowHandle()) { Here, LOWORD returns a WORD, which is (unsigned short). The resulting values in mp (a struct of LONGs which are (signed long)) will be huge and positive for negative screen coordinates. The resulting WindowFromPoint will then return NULL causing us to think the mouse is not over any part of our content. (GetCursorPos(), WM_MOUSEMOVE, WM_NCMOUSEMOVE, WM_{L,M,R}BUTTON{DOWN,UP,DBLCLK} handling should probably be checked too. I didn't see anything obviously wrong in a cursory search, but the call tree for event handling is way longer than the effort I just put in. Really only GetCursorPos() and WM_NCMOUSEMOVE are likely to cause similar problems since they return screen coordinates, and they aren't really used AFAICT. The others return client coordinates which will always be >=0 unless the values are munged by bits of mozilla I didn't look at.)
Correction to comment 41: I've seen similar behavior in java applets within both IE and Mozilla, where the rollover is pushed to the primary monitor (i.e. 0 x-position) as well as the symptoms are described above for mozilla/firebird. Since the summary doesn't mention it, I'll confirm that this occurd when my secondary monitor is positioned LEFT of my primary monitor.
*** Bug 238163 has been marked as a duplicate of this bug. ***
*** Bug 98830 has been marked as a duplicate of this bug. ***
*** Bug 239407 has been marked as a duplicate of this bug. ***
Comment on attachment 145388 [details] [diff] [review] fix mouse position handling under Windows According to http://msdn.microsoft.com/library/en-us/winui/WinUI/WindowsUserInterface/Window ing/MessagesandMessageQueues/MessagesandMessageQueuesReference/MessagesandMessa geQueuesFunctions/GetMessagePos.asp you could also use GET_X_LPARAM and GET_Y_LPARAM (just replace LOWORD and HIWORD and save the extra variable). But sr=dbaron either way.
Attachment #145388 - Flags: superreview+
Attachment #145388 - Flags: review?(ere)
Great to see a patch -- what was this bug languishing on hyatt's list for? The GET_X_LPARAM and GET_Y_LPARAM macros do seem cleaner, but whatever ere says goes. I'll approve for 1.7 with ere's r=. /be
Flags: blocking1.7+
The problem with GET_X_LPARAM and GET_Y_LPARAM is that although they do *exactly* what is needed here and in several other places in the moz codebase, they're defined in windowsx.h which also defines a lot of function macros with the same names as many MFC/ATL wrapper class functions (which are generally names any HWND wrapper class might want to use). Basically the header was designed for use with old straight C Windows code, and plays *very* badly with modern C++ code, generating lots of compile errors. Perhaps it wouldn't clash so badly with the moz codebase, but, bleurgh. You can of course just extract the two macro definitions and copy them into any file which uses them, or into a project level include - I've done this on several occasions - but stylistically that is a bad thing to do.
Comment on attachment 145388 [details] [diff] [review] fix mouse position handling under Windows I prefer GET_X_LPARAM and GET_Y_LPARAM. Could you fix nsWindow.cpp too while at it?
Attachment #145388 - Flags: review?(ere) → review-
Oops, I didn't notice your comment about the header. In that case, feel free to use MAKEPOINTS, but at least for nsWindow you could define a macro that converts GetMessagePos() to POINT.
How about POINTSTOPOINT(mp, MAKEPOINTS(pos))?
Here are two more patches. Both use GET_X_LPARAM/GET_Y_LPARAM, and I've changed all occurances in nsToolkit.cpp and nsWindows.cpp (and checked the rest of the windows/ directory) of LOWORD/HIWORD that operate on screen coordinates to use these instead. Messages that pass client coordinates continue to use LOWORD/HIWORD as recommended by MSDN - client coordinates are always positive. The first #includes windowsx.h. If this causes a problem, or if you take a look at windowsx.h and decide to avoid it, the second copies the two macros directly. Both via nsWindowAPI.h. Although I don't like duplicating stuff from system includes into private files, I'll point out that the latest versions of Microsoft's own ATL header files copy these macros instead of #including the source, as well as attempt to protect themselves by #undeffing other windowsx.h stuff in case the application has already pulled it in, which is a pretty big admission from them that windowsx.h is a crock! I've also fixed another bit of nsWindows I hadn't noticed before, which was trying to compare a client-coordinates mouse position against screen-coordinates window bounds. (To answer the previous comment, I don't much like the MAKEPOINTS or POINTSTOPOINT macros myself either - nasty horrible pointer casting kludges that require an lvalue to operate on - so it's much the same whatever way. A better option might be using inline functions to do the conversion but you get the macros for free with the SDK so it's more obvious to any Windows programmer what you're doing.)
Pulls in GET_X_LPARAM/GET_Y_LPARAM from windowsx.h
Copies GET_X_LPARAM/GET_Y_LPARAM into nsWindowAPI.h to avoid pulling in windowsx.h
(In reply to comment #58) > Created an attachment (id=145429) > fix mouse position handling under Windows > > Copies GET_X_LPARAM/GET_Y_LPARAM into nsWindowAPI.h to avoid pulling in > windowsx.h Why you have created two attachments and not included all patches into one? I don't know if we can set r/sr in that way.
Sorry for the spam but forget my last comment! I havn'r read comment 56 close enough. I hope this will be fixed soon...
==> patch author
Assignee: hyatt → jsfbbz
Comment on attachment 145428 [details] [diff] [review] fix mouse position handling under Windows Which of these two do you prefer?
Attachment #145428 - Flags: review?(ere)
I'd say attachment 145429 [details] [diff] [review] is far less likely to break anything. Both attachment 145429 [details] [diff] [review] and attachment 145428 [details] [diff] [review] address the problem though.
*** Bug 240415 has been marked as a duplicate of this bug. ***
Comment on attachment 145428 [details] [diff] [review] fix mouse position handling under Windows Despite being a little bit higher risk, I like 145428 more. With a quick glance I didn't seeanything destructive in windowsx.h and it's not that huge anyway.
Attachment #145428 - Flags: review?(ere) → review+
What's destructive is that it defines a whole bunch of macros with names that look like normal functions (ie, they're not all uppercased like most Platform SDK macros) with names that a Windows app might want to use. SubclassWindow is a fairly common example. Because they're macros, and not functions, it's impossible to override them safely, in a C++ class for example. Attempting to do so will most likely cause a compilation error as the macro turns your function prototype into mush. Slightly worse is the possibility that it won't cause a compilation error directly, but that because of name changes your symbol table and vtables will get messed up, resulting in harder to diagnose link or runtime errors. In fact, I tested attachment 145429 [details] [diff] [review] but not the other one. I have done so now since you're favouring it. It gets two compile errors due to the use of SubclassWindow (member of class nsWindow at nsWindow.h line 442) and GetNextSibling (member of class nsIAccessible at dist/include/accessibility/nsIAccessible.h line 60). To get attachment 145428 [details] [diff] [review] working you need to place two extra lines after #include <windowsx.h>: #undef SubclassWindow #undef GetNextSibling Then it compiles and works, however you run the risk of the same problem occuring should anyone ever decide that nsIAccessible needs a GetLastSibling method, or nsWindow's WindowStyle() would be better called GetWindowStyle(), or anything similar in any file which can see nsWindowAPI.h.
Ok, I thought it didn't cause trouble with the current code. In that case, let's use the other one. I wish they were defined somewhere else..
Comment on attachment 145429 [details] [diff] [review] fix mouse position handling under Windows Let's use this one then..
Attachment #145429 - Flags: review+
Attachment #145428 - Flags: review+
Comment on attachment 145429 [details] [diff] [review] fix mouse position handling under Windows My one problem with this is that I don't think you can copy-and-paste from windowsx.h and claim you have the right to license it under the MPL. So I'd prefer a rewritten version of the macros. Perhaps (using C++ construction-style casts): #define GET_X_LPARAM(pt) short(LOWORD(pt)) #define GET_Y_LPARAM(pt) short(HIWORD(pt)) (Or is there a reason I'm missing that the int cast is desirable?) Also, nsWindowsAPI.h has to do with the use of Unicode windows APIs, and I'm not sure whether it's the best place for this. Maybe I'd prefer nsToolkit.h, but I guess it really doesn't matter since nothing is particularly good.
My reason for chosing nsWindowsAPI.h was that that was the file that #included windows.h, so it seems a logical place to either #include windowsx.h or put the definitions from there in. As far as the rewrite: no, I can't see why the final (int) cast is necessary - possibly it silences a *very* picky compiler warning level, but really any normal compiler will automatically promote the value as needed. I can only see unnecessary truncation errors coming from keeping the cast in. Although as I pointed out, MS copy the definition where they need it rather than include it from their own file, they have the right to do so and I can well see why you wouldn't want it in the moz tree. Your reformulation seems quite sane to me (though I'd stick an extra pair of brackets round the lot), but I would keep the #ifdef guards around it. (You never know when MS might break with 15 years of tradition and put those defines somewhere sensible.) There comes a point when you have to consider that a certain amount of a given construct is dictated by the language and hopefully not legislatable against as being copyrightable as an original work - it's the only sensible way that could have been expressed in that language. But I just want to see this bug fixed in a shipped release and don't live in the States so choose the best variation you can think of. (The rest of either patch will just work as long as GET_*_LPARAM does the right thing, and the patch does fix the bug.)
*** Bug 240806 has been marked as a duplicate of this bug. ***
Blocks: majorbugs
*** Bug 240776 has been marked as a duplicate of this bug. ***
Will this make it into the next 1.7 release?
*** Bug 220383 has been marked as a duplicate of this bug. ***
Comment on attachment 145429 [details] [diff] [review] fix mouse position handling under Windows >- RECT r; >- VERIFY(::GetWindowRect(hWnd, &r)); >+ RECT r; >+ VERIFY(::GetClientRect(hWnd, &r)); I didn't notice this before: why the change in function called? This looks like a substantive change -- does it fix a known bug?
The existing code: 1) Retrieved the latest mouse position in screen coordinates. 2) Found the desktop window containing that point 3) Converted the mouse position into client coordinates relative to that window. (Client coordinates are relative to the top-left corner of the drawable area of a window.) 4) Compared this client-coordinate position to the Window rectangle which is returned in screen coordinates. Whatever this is supposed to achieve, it is wrong - you can't directly compare client coordinates to screen coordinates. The fix makes sure that the comparison is at least using the same coordinate system. I don't know what it's then supposed to achieve since the window was retrieved by asking which window contains that mouse position - going on to then ask whether the window contains that mouse position is tautologous. (There is a slight semantic change in that GetWindowRect returns the whole frame rectangle, whereas GetClientRect returns only the drawable area of the window. This is a stricter test than the original WindowFromPoint applies. The alternative is to GetWindowRect and compare directly to the screen-coordinate mouse position. The following comment implies we're not interested in non-client hits so I think this is right, and is the only way I can see the bounds check being at all useful.) As to whether it fixes a known bug - it is clearly a mistake. I know that the patch as a whole fixes this bug at least, and probably some related ones, but I didn't bother to find out which particular section of the patch is the critical change. I just fixed any coordinate handling issues I noticed on my way through.
*** Bug 242858 has been marked as a duplicate of this bug. ***
Do we have a final patch for 1.7 here or not?
What's going on with this patch?
Somebody who can build and test on Windows should produce a new patch per comment 69.
I hope this version is acceptable?
Attachment #145429 - Attachment is obsolete: true
*** Bug 243518 has been marked as a duplicate of this bug. ***
Fix checked in to trunk, 2004-05-13 16:13 -0700. Please test tomorrow's trunk builds.
Comment on attachment 148409 [details] [diff] [review] Update macro definitions as per comment 69 ere r+'d already, I am approving for 1.7. Someone please check in tonight, so the fix is in RC2. /be
Attachment #148409 - Flags: review+
Attachment #148409 - Flags: approval1.7+
Ok, dbaron has convinced me that this patch (attachment 148409 [details] [diff] [review]) should wait for some trunk baking. That means we'd take it after RC2, possibly without another RC, for 1.7 final. Asa, beware. /be
*** Bug 243578 has been marked as a duplicate of this bug. ***
*** Bug 240452 has been marked as a duplicate of this bug. ***
Brendan, it's been about 10 days. Can we get this in for RC3?
Hey, I was all for it up front -- dbaron was the one who wanted some trunk bake time, and I was ok with that. Sounds like it's done. /be
Does everyone agree this is working with no regressions? If so, let's get it in.
can someone land this for dbaron?
Seems to work well here.
Whiteboard: patch ready to land
Whiteboard: patch ready to land → patch ready to land. mail sent to drivers asking for landing help
after apply it, I did a little smoke testing. I've landed the fix on the 1.7 branch. thanks to john sullivan for the fix.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.7final
Keywords: fixed1.7
This seems to be occurring on yesterday's Firefox branch build. Does this need to be applied to FF branch or does it need a separate bug or ...?
That's what happens when you branch too soon.
Added Firefox bug of it: Bug 245256
fixed on aviary branch
Not all of us can wait around for always-weeks-late Mozilla releases to ship - some of us have actual product deadlines to meet :-P We're well aware of the list of bugs that need to be brought across from 1.7, we did not branch too soon, it's a manageable situation.
*** Bug 242647 has been marked as a duplicate of this bug. ***
*** Bug 239418 has been marked as a duplicate of this bug. ***
*** Bug 239811 has been marked as a duplicate of this bug. ***
*** Bug 242797 has been marked as a duplicate of this bug. ***
*** Bug 251863 has been marked as a duplicate of this bug. ***
No longer blocks: majorbugs
*** Bug 135074 has been marked as a duplicate of this bug. ***
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: shrir → xptoolkit.widgets
Hy, SeaMonkey 1.1.11 Problem with links. The html code "on mouse over" doesn't seem to be correctly interpreted. It doesn't keep the highlighted and only a very tiny zone on the very top of the links can be clicked to move on. This code is in my page : <div id="navd">Pages : <a href="gall2_css.htm">2</a> &nbsp;&nbsp;&nbsp;</div> This one in a css file : #navd {color:#023a39; position:absolute; border-collapse: collapse; border-left: 1px solid green; border-bottom: 1px solid green; left:/*42%;*/400px; top:92px; width:200px; font-family:"Microsoft Sans Serif", sans serif; font-size: 10pt; } #navd A:link {color:#023a39; text-decoration: none;} #navd A:hover {color: #b8fa4e;text-decoration: none; font-weight: bold;} It works correctly under IE 6.0.29.5512
Hy again I've checked again to be sure. only a very small small zone in the very top of the links(about perhaps 1 pixel) is mouse sensitive to make appear the higlight of the A:hover. If you doesn't find the zone(wich is very difficult) you can NOT click the link.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: