685470 - Bookmarks tooltip should not pop up when I click the bookmark.( not only bookmarks but also links in contents )

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/09935ede3c77
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110907 Firefox/9.0a1 ID:20110907030853

Bookmarks tooltip should not pop up when I click the bookmark.


Reproducible: Always

Steps to Reproduce:
1. Open Firefox with clean profile
2. Open Bookmarks Sidebar (Ctrl+b)
3. Expand tree "Bookmarks Menu" and "Mozilla Firefox"
4. Click a Bookmarks quickly before tooltip pops up.
5. And stay mouse pointer(do not move mouse).

Actual Results:
  Bookmark tooltip will pop up.
  
Expected Results:
  Bookmark tooltip should not pop up.



Regression window(cached m-c hourly),
Works:
http://hg.mozilla.org/mozilla-central/rev/7898841a922a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110630 Firefox/7.0a1 ID:20110702004346
Fails:
http://hg.mozilla.org/mozilla-central/rev/7b44ed36faf3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110702 Firefox/7.0a1 ID:20110702022656
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7898841a922a&tochange=7b44ed36faf3



Regression window(cached m-i hourly),
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cdc314af3241
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110630 Firefox/7.0a1 ID:20110630121221
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/11e4980826aa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110630 Firefox/7.0a1 ID:20110630150117
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cdc314af3241&tochange=11e4980826aa


Triggered by:
Bug 664058 - Kill AddEventListenerByIID/RemoveEventListenerByIID from layout

Comment 1

[Alice0775 White]

This happens not only bookmarks but also links in contents 

Steps to Reproduce:
1. Open Firefox with clean profile
2. Open http://kb.mozillazine.org/Knowledge_Base
3. Click Image of half Firefox Logo quickly before tooltip pops up.
5. And stay mouse pointer(do not move mouse).

Actual Results:
  tooltip will pop up.
  
Expected Results:
  tooltip should not pop up.

Comment 2

[Alice0775 White]

Right click also triggers.
This is worse because the tooltip covers contexi menu item.

Comment 3

[Alice0775 White]

Right-click Back/Forward button, the tooltip overlaps navigation history drop down.
And Left button down on Back/Forward button, the tooltip appears and then navigation history pops up.
This also regression of Bug 664058 .

Comment 4

[LegNeato]

Jonas, can you help me reason about this? Why did it happen? What can we back out to get back to old behavior?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The patch is purely cleanup, so there's not really any cost of backing it out, other than the normal having to respin and the like.

Backing out on trunk though is harder since other cleanup has landed on top of it.

OTOH, it doesn't sound like a terrible regression, but I might have misunderstood it.

Comment 6

[LegNeato]

Ok, we're not going to back this out for 7. What's the plan for dealing with this on trunk?

Comment 8

[Alice0775 White]

This problem lets irritate.
http://www.youtube.com/watch?v=JibVmuF8MYk

Comment 9

[LegNeato]

Jonas, poke. If Alice is irritated by this we should probably fix it :-)

Comment 10

[Alice0775 White]

Attachment: testcase

Mouse over text and right click immediately.

A tooltip overlaps with context menu.

Comment 11

[Alice0775 White]

Screen capture http://youtu.be/cTMINVmuvOQ

After click forward button, un-wanted tooltip pops up.
So, the tooltip does not hide even if the other apprication is focused.

Comment 12

[James]

Attachment: Part 1 - add mousedown/mouseup listeners before tooltip is shown

Addresses the bug as shown in comment #8 and comment #10. This adds the mousedown/mouseup event listeners to tooltip nodes so clicks will kill the timer. It also sets the mTooltipShownOnce flag so moving the mouse within the target after clicking (and without leaving the node) doesn't show the tooltip.

Comment 13

[James]

Attachment: Part 2 - Use TrackMouseEvent to listen for mouseout on Windows

This addresses the bug as shown in comment #11 which I assume is only a Windows bug. The WM_MOUSELEAVE message isn't received sometimes if the mouse is moved too fast out of the browser after briefly hovering over the forward button (you don't even have to click).

Comment 14

[James]

Attachment: Test case

Test for the patch in Part 1. I wasn't able to reliably test for the second part of the bug.

Comment 15

[Jim Mathies [:jimm]]

If we add this, can't we remove the mMousePresent logic?

Comment 16

[James]

Possibly, I'll need to do a little testing to make sure the WM_MOUSELEAVE message is still generated for non-client areas with just TrackMouseEvent. I also need to fix the original patch. I noticed I forgot to initialize mMouseTracking.

Comment 17

[James]

Attachment: Part 2 (v2) - Use TrackMouseEvent to listen for mouseout on Windows

Original patch with mMouseTracking initialized and mMousePresent logic removed. With TrackMouseEvent, I'm seeing WM_MOUSELEAVE messages before WM_NCMOUSEMOVE messages. So the WM_NCMOUSEMOVE was never falling through to send another mouseleave message anyway.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 683106 [details] [diff] [review]
Part 1 - add mousedown/mouseup listeners before tooltip is shown

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

I don't understand why none of this was needed before the changes in bug 664058 though?

Comment 19

[James]

I built the changeset before bug 664058 landed (7898841a922a) to do some digging and here's what I found.

Mousedown and mouseup events were definitely being received for targets even without adding the event listeners in AddToolTipSupport().

In the Part 1 attachment of bug 661297 (which bug 664058 blocked) the removal of

static const EventDispatchData sMouseEvents[] = {
-  { NS_MOUSE_BUTTON_DOWN,        HANDLER(&nsIDOMMouseListener::MouseDown)     },
-  { NS_MOUSE_BUTTON_UP,          HANDLER(&nsIDOMMouseListener::MouseUp)       },

in content/events/src/nsEventListenerManager.cpp seems to have been the cause.
By commenting those lines out in my changeset 7898841a922a build, I stopped receiving the mousedown/mouseup events and hit this bug.

I'm not too familiar with the event system, but I'm guessing the events in sMouseEvents[] were being applied to everything before that patch.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 683106 [details] [diff] [review]
Part 1 - add mousedown/mouseup listeners before tooltip is shown

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

Ah, yeah, the old code had a lot of magic in it which was very hard to understand. Which is why I nuked it :)

This looks good to me then, though maybe someone else should review the change to nsXULTooltipListener::HandleEvent since that was definitely not in the old code, though it sounds like it's the right thing to do.

Thanks for digging into this by the way!

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Does this also fix bug 787845?

Comment 22

[James]

(In reply to Jonas Sicking (:sicking) from comment #20)
> This looks good to me then, though maybe someone else should review the
> change to nsXULTooltipListener::HandleEvent since that was definitely not in
> the old code, though it sounds like it's the right thing to do.
> 

Yea, I added that to try to match the behavior of other applications. I forgot to add a check for the MOZ_XUL define in that change so I'll need to resubmit a patch. I can remove that completely if you feel that's safer or just add the #ifdef.

(In reply to Jonas Sicking (:sicking) from comment #21)
> Does this also fix bug 787845?

Yep, that looks a dupe of this bug.

Comment 23

[James]

Attachment: Part 1 (v2) - add mousedown/mouseup listeners before tooltip is shown

Added check for MOZ_XUL define.
Carrying r+ over. (hope I did this right)

Comment 24

[James]

Attachment: Part 2 (v3) - Use TrackMouseEvent to listen for mouseout on Windows

Added some logic to check if a drag & drop finished outside the browser before letting WM_MOUSELEAVE fall through. With TrackMouseEvent() the message is received too early, so this should make it behave like it currently does.

Comment 25

[James]

I've been using this for a bit and still encounter the bug in comment #11 even with the TrackMouseEvent patch, albeit less often. There's also a difference in when WM_MOUSELEAVE events are received that I'm not comfortable with when drag & drop is involved. I think the patch in Part 1 is sufficient to fix this bug and maybe a separate bug should be filed for the mouseout issue on Windows. I'm cancelling the review request on that patch Jim and if no one objects I'll request a checkin on just the tooltip listener patch.

Comment 26

[James]

Attachment: Test

Changed commit message (no longer a 3 part patch).

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eed8b79a4803
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1ff68f37d9

Comment 28

[James]

Attachment: mousedown/mouseup listeners only

Removed change to nsXULTooltipListener::HandleEvent to fix test_tooltip.xul failure on tbpl

Comment 29

[James]

Attachment: Test

changed test due to nsXULTooltipListener::HandleEvent removal

Comment 30

[James]

I see this failed on test_tooltip.xul on tbpl. Removing the HandleEvent stuff lets the test run locally. Could someone push these to try? If it passes there I'll request fresh reviews.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Here's the backout changeset. I'm heading off to bed, so I can't push it to Try for you :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad2846f3ece

(If you want to request Level 1 commit access, I'd be happy to vouch for you, though!)
http://www.mozilla.org/hacking/commit-access-policy/

Comment 32

[James]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=cc932d5ba3aa

Thanks for vouching Ryan!

Comment 33

[James]

This passes the tests on try and was a trivial change. Carrying the r+'s back.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2daca28d7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b28290a53c

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dc2daca28d7f
https://hg.mozilla.org/mozilla-central/rev/c2b28290a53c