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

RESOLVED FIXED in mozilla20

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Alice0775 White, Assigned: James)

Tracking

({regression})

7 Branch
mozilla20
x86
All
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox7+, firefox8+)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
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.
Summary: Bookmarks tooltip should not pop up when I click the bookmark. → Bookmarks tooltip should not pop up when I click the bookmark.( not only bookmarks but also links in contents )
(Reporter)

Comment 2

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

Updated

7 years ago
Assignee: nobody → jonas
tracking-firefox7: --- → ?
tracking-firefox8: --- → ?
(Reporter)

Comment 3

6 years ago
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 .
OS: Windows 7 → All

Updated

6 years ago
tracking-firefox7: ? → +
tracking-firefox8: ? → +

Comment 4

6 years ago
Jonas, can you help me reason about this? Why did it happen? What can we back out to get back to old behavior?
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

6 years ago
Ok, we're not going to back this out for 7. What's the plan for dealing with this on trunk?
(Reporter)

Updated

6 years ago
Duplicate of this bug: 689553
(Reporter)

Comment 8

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

Comment 9

6 years ago
Jonas, poke. If Alice is irritated by this we should probably fix it :-)
(Reporter)

Comment 10

6 years ago
Created attachment 590638 [details]
testcase

Mouse over text and right click immediately.

A tooltip overlaps with context menu.
(Reporter)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
Created attachment 683106 [details] [diff] [review]
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.
Attachment #683106 - Flags: review?(jonas)
(Assignee)

Comment 13

5 years ago
Created attachment 683107 [details] [diff] [review]
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).
Attachment #683107 - Flags: review?(jmathies)
(Assignee)

Comment 14

5 years ago
Created attachment 683109 [details] [diff] [review]
Test case

Test for the patch in Part 1. I wasn't able to reliably test for the second part of the bug.
Attachment #683109 - Flags: review?(jonas)

Updated

5 years ago
Assignee: jonas → jh.dev0

Comment 15

5 years ago
If we add this, can't we remove the mMousePresent logic?
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 683130 [details] [diff] [review]
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.
Attachment #683107 - Attachment is obsolete: true
Attachment #683107 - Flags: review?(jmathies)
Attachment #683130 - Flags: review?(jmathies)
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?
(Assignee)

Comment 19

5 years ago
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 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!
Attachment #683106 - Flags: review?(jonas) → review+
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
Created attachment 684703 [details] [diff] [review]
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)
Attachment #683106 - Attachment is obsolete: true
Attachment #684703 - Flags: review+
(Assignee)

Comment 24

5 years ago
Created attachment 684706 [details] [diff] [review]
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.
Attachment #683130 - Attachment is obsolete: true
Attachment #683130 - Flags: review?(jmathies)
Attachment #684706 - Flags: review?(jmathies)
(Assignee)

Comment 25

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #684706 - Flags: review?(jmathies)
(Assignee)

Updated

5 years ago
Attachment #684706 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #683109 - Attachment description: Part 3 - test → Test case
(Assignee)

Comment 26

5 years ago
Created attachment 688542 [details] [diff] [review]
Test

Changed commit message (no longer a 3 part patch).
Attachment #683109 - Attachment is obsolete: true
Attachment #688542 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 28

5 years ago
Created attachment 689055 [details] [diff] [review]
mousedown/mouseup listeners only

Removed change to nsXULTooltipListener::HandleEvent to fix test_tooltip.xul failure on tbpl
Attachment #684703 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Created attachment 689056 [details] [diff] [review]
Test

changed test due to nsXULTooltipListener::HandleEvent removal
Attachment #688542 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
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.
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/
(Assignee)

Comment 32

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

Thanks for vouching Ryan!
(Assignee)

Updated

5 years ago
Attachment #689056 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #689055 - Flags: review+
(Assignee)

Comment 33

5 years ago
This passes the tests on try and was a trivial change. Carrying the r+'s back.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dc2daca28d7f
https://hg.mozilla.org/mozilla-central/rev/c2b28290a53c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.