Closed Bug 1085027 Opened 5 years ago Closed 5 years ago

Regression in Tooltip Handling

Categories

(Core :: Widget: Win32, defect)

35 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED WORKSFORME
mozilla36

People

(Reporter: marcausl, Assigned: u518207)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

I have added this to some old bugs but I suspect the reported regression has not been noticed.
I run in pointer focus mode, which you can get in windows by:

Use control panel or search to get to Ease of Access Center
Click 'Make the mouse easier to use'
Check the option 'Activate a window by hovering over it with the mouse' and click OK.

If I have a windows partially overlaying firefox, for example with it's left edge far enough right to expose my menu bar and then move the mouse back and forth across the edge I can pretty reliably get a tool tip placed over the other window. And remember I'm running pointer focus, not click focus.

Also, my menu bar has the bookmark icon to the left of the address bar. Specifically, the edge of the overlaying windows is just right of the bookmark icon, and it's the bookmark tooltip that is usually the offender.

I've attached a screen shot to Bug 236870

In practice, I'm pretty regularly getting tooltips stuck over other windows.  Mousing on the tooltip closes it.

Last good revision: 14665b1de5ee (2014-10-01)
First bad revision: 5d6ec4dddf14 (2014-10-02)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=14665b1de5ee&tochange=5d6ec4dddf14

The pushlog appears to contain several mouse handling changes.
Bug 506815 seems like the most likely culprit in that range.

Can you try mozilla-inbound builds as well and see if you can narrow down
the regression range?
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/?C=M;O=A
(FYI, the revision is listed in the firefox-35.0a1.en-US.win32.txt files)
Keywords: regression
Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c14d1ef31a7f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141001072923
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/165c3fd176ec
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141001112720
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c14d1ef31a7f&tochange=0d9b59a1d80b

Suspect: 
	7c97034feb78	mycoolclub — Bug 506815 - Replace old MouseTrailer code with TrackMouseEvent api. r=jimm
Blocks: 506815
Thanks Alice.
Status: UNCONFIRMED → NEW
Component: General → Widget: Win32
Ever confirmed: true
Product: Firefox → Core
Version: 36 Branch → 35 Branch
mycoolclub@yahoo.com, can you take a look?
Flags: needinfo?(mycoolclub)
Ignorance alert - I'm just a user who doesn't know either windows or firefox code.

It appears to me that there may be a race between putting up the tooltip and the mouse leaving.  If I wait until the tooltip is up and then move away it clears.  But if I sort of rush moving away and the timing is wrong, the tooltip appears after the mouse is gone from the firefox window, overlaying the window it is  now on.  That window was always on top (pointer focus).
Restoring needinfo I trashed - couldn't see a way to comment without clearing it.
The mouse leave event was handled manually, now we use native Windows API method to handle it. As I see from bug 236870 there is similar side effect from described actions (special mouse leave case). I suppose that it is the way that those OS'es (Windows and Mac) handle this action (with shown side effect).
Flags: needinfo?(mycoolclub)
Maybe TrackMouseEvent is buggy in some edge cases and that's what the (now removed)
MouseTrailer was trying to work around?  Do we actually get a WM_MOUSELEAVE event
for the steps to reproduce in comment 0?
Flags: needinfo?(mycoolclub)
The previous method delayed sending WM_MOUSELEAVE message for couple of hundreds of milliseconds. The current method, in contrary, TrackMouseEvent() seems not to make any delay between "mouse leave" and sending actual WM_MOUSELEAVE message.
If the side effect, mentioned earlier, disturbs, maybe we should think to return to the previous implementation of handling "mouse leave" event.
Flags: needinfo?(mycoolclub)
This is consistent with the fact that the problem has always existed but was made much worse by eliminating the delay.
Attached patch 1085027.patch (obsolete) — Splinter Review
Try this fix.
The WM_MOUSELEAVE event was reported only after we returned the cursor to the window and pressed some button, so in some cases the message wasn't always sent.
Now we reinitialize mouse tracking after every NS_MOUSE_MOVE. It seems to solve the problem.
Attachment #8508204 - Flags: review?(jmathies)
I'm guessing this patch is in /pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-64845aa387a6

I've tried it and it solves the problem for me.  I can still see a late tooltop over another window but as soon as the mouse moves it disappears.

So IMHO the race between leaving and generating the tooltip still exists but this change makes it pretty much a non issue.
I spoke too soon.  If in fact the patch is in the try-build above it's still broken, in the sense that it's possible to wind up with a tooltip over another window while the mouse navigates that window.  The tool-tip closes only if you mouse over it - which was always the case.
Attachment #8508204 - Attachment is obsolete: true
Attachment #8508204 - Flags: review?(jmathies)
Attached patch 1085027-v2.patch (obsolete) — Splinter Review
I could not reproduce the problem, but I see that in some cases WM_NCMOUSELEAVE is not always followed by WM_MOUSELEAVE, as it used to be. Now the message WM_NCMOUSELEAVE triggers WM_MOUSELEAVE, maybe this will help.
Attachment #8508491 - Flags: feedback?(marcausl)
I am not able to build firefox so I need a pointer to a try-built to test and changes.
We need someone in charge, who can do it.
Comment on attachment 8508491 [details] [diff] [review]
1085027-v2.patch

Review of attachment 8508491 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +3997,5 @@
>  
>    // call the event callback
>    if (mWidgetListener) {
>      if (aEventType == NS_MOUSE_MOVE) {
> +      TRACKMOUSEEVENT mTrack;

nit - why mTrack? Was this a member variable at some point?

@@ +4006,5 @@
> +      TrackMouseEvent(&mTrack);
> +      
> +      mTrack.dwFlags = TME_LEAVE;
> +      // Request WM_MOUSELEAVE events for this window.
> +      TrackMouseEvent(&mTrack);

not a huge fan of making two TrackMouseEvent api calls for every mouse move. Is there some way we can avoid this?
Attached patch 1085027-v3.patch (obsolete) — Splinter Review
mTrack is a new name: 'm' like in "mWnd" and "Track" is from TrackMouseEvent().
TrackMouseEvent() is called only for the first mouse move in a window in contrast to calling it every move.
Attachment #8508491 - Attachment is obsolete: true
Attachment #8508491 - Flags: feedback?(marcausl)
Attachment #8508729 - Flags: review?(jmathies)
(In reply to mycoolclub from comment #18)
> Created attachment 8508729 [details] [diff] [review]
> 1085027-v3.patch
> 
> mTrack is a new name: 'm' like in "mWnd" and "Track" is from
> TrackMouseEvent().
> TrackMouseEvent() is called only for the first mouse move in a window in
> contrast to calling it every move.

the 'm' for variables in mozilla's code base denotes "member variable". Please use a local name like 'track' or 'tme'.
Comment on attachment 8508729 [details] [diff] [review]
1085027-v3.patch

Review of attachment 8508729 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the nits, I'd like to see the final patch before it lands.

::: widget/windows/nsWindow.cpp
@@ +3996,5 @@
>    event.mPluginEvent.Copy(pluginEvent);
>  
>    // call the event callback
>    if (mWidgetListener) {
> +    if (aEventType == NS_MOUSE_MOVE) { 

nit - white space here and in other places. Please clean this up.

@@ +4929,5 @@
> +    
> +    case WM_NCMOUSELEAVE:
> +      // If upon mouse leave event, only WM_NCMOUSELEAVE message is sent, sending WM_MOUSELEAVE message
> +      // makes the event being properly handled.
> +        SendMessage(mWnd, WM_MOUSELEAVE, 0, 0);

nit - spacing is off here
Attachment #8508729 - Flags: review?(jmathies) → review-
Assignee: nobody → mycoolclub
Attached patch 1085027-v3.patch (obsolete) — Splinter Review
Spaces removed.
Attachment #8508729 - Attachment is obsolete: true
Attachment #8508822 - Flags: review?(jmathies)
Comment on attachment 8508822 [details] [diff] [review]
1085027-v3.patch

Review of attachment 8508822 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +4851,5 @@
>  
>      case WM_MOUSEMOVE:
>      {
> +      if (!mMousePresent) {
> +        TRACKMOUSEEVENT mTrack;

rename 'mTrack' to 'tme' or 'track' please.

@@ +4858,5 @@
> +        mTrack.hwndTrack = mWnd;
> +        // Request WM_MOUSELEAVE events for this window.
> +        TrackMouseEvent(&mTrack);
> +      }
> +      

nit - white space :)

@@ +4925,5 @@
>        DispatchMouseEvent(NS_MOUSE_EXIT, mouseState, pos, false,
>                           WidgetMouseEvent::eLeftButton, MOUSE_INPUT_SOURCE());
>      }
>      break;
> +    

nit - white space :)
Attachment #8508822 - Flags: review?(jmathies) → review-
Attached patch 1085027-v3.patchSplinter Review
Modifications performed.
Attachment #8508822 - Attachment is obsolete: true
Attachment #8508914 - Flags: review?(jmathies)
Attachment #8508914 - Flags: review?(jmathies) → review+
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=80c0ec8c05d1

Marc, any chance you could take this build for a spin and see if the issue is fixed for you?
Flags: needinfo?(marcausl)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/50f2a49570ce

Thanks for the patch! One request for future patches, please make sure you use a commit message that summarizes what the patch is doing instead of restating the problem being solved. See the link below for more information:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
In a quick sniff I cannot make this fail using the tests that reliably failed before. - looks good but I'll run with is and report later.
Flags: needinfo?(marcausl)
https://hg.mozilla.org/mozilla-central/rev/50f2a49570ce
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Backed out in https://hg.mozilla.org/mozilla-central/rev/ae4d9b4ff2ee on suspicion of being what turned bug 931445 permaorange on Win7 opt.

Looks like that failure was almost exclusively 10.6 until something landed on mozilla-inbound on September 26th or 27th to make it fairly common on Win7 opt, and then this or something else in the same checkin-needed push made it permaorange, opt and PGO, but we didn't bother noticing that and just merged it around. Remains to be seen whether it actually was this, or something even less likely, but this seemed like the first best bet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Phil Ringnalda (:philor) from comment #30)
 Remains to be seen whether it actually was this, or
> something even less likely, but this seemed like the first best bet.

the permaorange went away after the backout in m-c and fx-team of course
dom/tests/mochitest/pointerlock/test_pointerlock-api.html

sounds closely related. mcc can you please test locally to see if you can reproduce the timeout here on win7?
It writes me that all 57 tests passed.
I can't reproduce local either -
Passed: 57
Failed: 0
Todo: 0

Lets push to try again to see if it reproduces there.

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ac53933cc132
remote:   https://tbpl.mozilla.org/?tree=Try&rev=ac53933cc132
(In reply to Jim Mathies [:jimm] from comment #34)
> I can't reproduce local either -
> Passed: 57
> Failed: 0
> Todo: 0
> 
> Lets push to try again to see if it reproduces there.
> 
> remote:  
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ac53933cc132
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=ac53933cc132

Looking pretty green. Lets wait until all the retriggers finish up.
(In reply to Jim Mathies [:jimm] from comment #35)
> (In reply to Jim Mathies [:jimm] from comment #34)
> > I can't reproduce local either -
> > Passed: 57
> > Failed: 0
> > Todo: 0
> > 
> > Lets push to try again to see if it reproduces there.
> > 
> > remote:  
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ac53933cc132
> > remote:   https://tbpl.mozilla.org/?tree=Try&rev=ac53933cc132
> 
> Looking pretty green. Lets wait until all the retriggers finish up.

hmm, win7 opt failures. Odd that this doesn't happen on xp or win8.
(In reply to Jim Mathies [:jimm] from comment #36)
> hmm, win7 opt failures. Odd that this doesn't happen on xp or win8.

Not surprising actually, these tests are disabled on all sorts of platforms, including xp and win8.

http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/pointerlock/test_pointerlock-api.html?force=1#82
Maybe this:
(From  comment 31)
> the permaorange went away after the backout in m-c and fx-team of course
Those tests are not reproducible offline, so I can't say something definite about it.
I meant timeout.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.