Last Comment Bug 646038 - URL destination flickers when the mouse pointer is over a link positioned at the bottom corner
: URL destination flickers when the mouse pointer is over a link positioned at ...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: 4.0 Branch
: All All
: -- normal with 1 vote (vote)
: Firefox 12
Assigned To: Dão Gottwald [:dao]
:
:
Mentors:
data:text/html,<a href="http://exampl...
: 634433 713808 (view as bug list)
Depends on: 727793 733284
Blocks: 717752 629925
  Show dependency treegraph
 
Reported: 2011-03-29 06:27 PDT by dror3go
Modified: 2013-12-27 14:27 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
patch (5.81 KB, patch)
2011-05-13 06:48 PDT, Dão Gottwald [:dao]
gavin.sharp: review-
Details | Diff | Splinter Review
patch v2 (6.07 KB, patch)
2011-05-17 01:29 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v3 (7.39 KB, patch)
2011-05-18 02:47 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v3 (7.41 KB, patch)
2011-05-22 11:50 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v3 (7.56 KB, patch)
2011-09-27 05:54 PDT, Dão Gottwald [:dao]
gavin.sharp: review+
Details | Diff | Splinter Review

Description dror3go 2011-03-29 06:27:43 PDT
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: 

When moving the mouse over a link positioned at the bottom left (bottom right in RTL builds of Firefox), the cursor flickers once. At that specific moment, the link is not clickable (clicking will not go to the destination URL).

Testcase:
data:text/html,<a href="http://example.net/" style="position:absolute; bottom: 0.5em;left:0">HOVER ME</a>

* RTL users should change left:0 to right:0

Reproducible: Always
Comment 1 Tomer Cohen :tomer 2011-03-29 06:33:27 PDT
I've added the testcase to the URL field, in order to make it easier to test.
Comment 2 Alex Lakatos[:AlexLakatos] 2011-03-31 09:01:42 PDT
Confirmed.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-03-31 09:09:56 PDT
Presumably because the status thingie is shown and then moved? Can we reverse the order of those?
Comment 4 dror3go 2011-05-08 00:42:07 PDT
Am I the only one who suffers from this on a regular basic since 4.0 (at least 50 times a day, and I'm not kidding)?

Any chance to push this to 4.0.X?
Comment 5 Tomer Cohen :tomer 2011-05-08 01:07:59 PDT
(In reply to comment #4)
> Any chance to push this to 4.0.X?
I afraid we've missed it for Firefox 4.x, but it hope we can get it in for the next version.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-05-09 13:52:16 PDT
I see this.

Dão, could you take a look?
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-09 17:13:02 PDT
(In reply to comment #3)
> Presumably because the status thingie is shown and then moved? Can we
> reverse the order of those?

It's not as simple as that, because the mouseover is what triggers the moving.
Comment 8 Asa Dotzler [:asa] 2011-05-10 14:54:11 PDT
not going to track this for Firefox 5. there are a number of issues with the status implementation that should probably be addressed holistically.
Comment 9 Dão Gottwald [:dao] 2011-05-13 06:48:48 PDT
Created attachment 532196 [details] [diff] [review]
patch
Comment 10 Dão Gottwald [:dao] 2011-05-13 12:48:55 PDT
*** Bug 634433 has been marked as a duplicate of this bug. ***
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-16 12:07:46 PDT
Comment on attachment 532196 [details] [diff] [review]
patch

Seems like MousePosTracker should only register its event listeners when it has listeners to call. Also it doesn't need to store _x/_y on the object, it can just pass them directly to callListener, right? I'm actually not sure that we need the generic MousePosTracker functionality at all - do we have other potential users?

>+    this._x = event.screenX - window.mozInnerScreenX;
>+    this._y = event.screenY - window.mozInnerScreenY;

I think you probably need to consider screenPixelsPerCSSPixel here (mozInnerScreenX is CSS pixels, I think screenX are device pixels). Unless chrome prescontexts are never zoomed?
Comment 12 Dão Gottwald [:dao] 2011-05-16 12:22:28 PDT
(In reply to comment #11)
> Seems like MousePosTracker should only register its event listeners when it
> has listeners to call.

addListener wouldn't be able to directly call the listener then. It also seems pretty clear that the status panel would add the listener soon after startup.

> Also it doesn't need to store _x/_y on the object, it
> can just pass them directly to callListener, right?

Same as above, addListener couldn't call the listener then.

> I'm actually not sure
> that we need the generic MousePosTracker functionality at all - do we have
> other potential users?

I was thinking of using it for full screen mode instead of the 1px tall black bar.

> I think you probably need to consider screenPixelsPerCSSPixel here
> (mozInnerScreenX is CSS pixels, I think screenX are device pixels). Unless
> chrome prescontexts are never zoomed?

Not sure, but it seems strange that mozInnerScreenX would be CSS pixels.
Comment 13 Dão Gottwald [:dao] 2011-05-17 01:29:52 PDT
Created attachment 532897 [details] [diff] [review]
patch v2

Converted event.screenX/Y to CSS pixels. Tested manually by executing this in the error console:

top.opener.getInterface(Components.interfaces.nsIWebNavigation).QueryInterface(Components.interfaces.nsIDocShell).contentViewer.QueryInterface(Components.interfaces.nsIMarkupDocumentViewer).fullZoom = 2
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-17 13:56:15 PDT
I still don't like having a global mousemove handler. Ideally we could poll the mouse position when we open the panel instead, but I suppose there's no easy way to do that...
Comment 15 Dão Gottwald [:dao] 2011-05-17 14:33:39 PDT
Comment on attachment 532897 [details] [diff] [review]
patch v2

bz, do you happen to have an opinion on this? Does this look like something front-end code should avoid doing? Would an API for querying the current mouse pointer position make sense / be more efficient?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-05-17 22:23:42 PDT
Comment on attachment 532897 [details] [diff] [review]
patch v2

I'm going to punt to roc on this; he's thought more about those issues.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-17 22:42:02 PDT
Comment on attachment 532897 [details] [diff] [review]
patch v2

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

::: browser/base/content/browser.js
@@ +8705,5 @@
> +  handleEvent: function (event) {
> +    var screenPixelsPerCSSPixel = this._windowUtils.screenPixelsPerCSSPixel;
> +    this._x = event.screenX / screenPixelsPerCSSPixel - window.mozInnerScreenX;
> +    this._y = event.screenY / screenPixelsPerCSSPixel - window.mozInnerScreenY;
> +

You shouldn't need screenPixelsPerCSSPixel here; screenX/Y should be in CSS pixels already.

@@ +8720,5 @@
> +    let rect = listener.getMouseTargetRect();
> +    let hover = this._x >= rect.left &&
> +                this._x <= rect.right &&
> +                this._y >= rect.top &&
> +                this._y <= rect.bottom;

you probably should be testing < instead of <=
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-17 22:43:48 PDT
So the idea here is to detect events being fired at an element that is transparent to events?

How would an API for querying the current mouse position help?

What issues am I supposed to have thought about, exactly? :-)
Comment 19 Dão Gottwald [:dao] 2011-05-17 23:01:04 PDT
(In reply to comment #18)
> So the idea here is to detect events being fired at an element that is
> transparent to events?

Yes... I want it to respond to mouse movement but not to clicks.

> How would an API for querying the current mouse position help?
> 
> What issues am I supposed to have thought about, exactly? :-)

One problem is that when the mouse is at the lower left, the status panel should appear on the right, rather than appearing on the left and moving to the right when the mouse budges. So in order to tell where the status panel should show up initially, the current patch tracks the mouse movement in the window all the time. Should we be concerned about the overhead of this?
Comment 20 Dão Gottwald [:dao] 2011-05-17 23:03:58 PDT
(In reply to comment #17)
> > +  handleEvent: function (event) {
> > +    var screenPixelsPerCSSPixel = this._windowUtils.screenPixelsPerCSSPixel;
> > +    this._x = event.screenX / screenPixelsPerCSSPixel - window.mozInnerScreenX;
> > +    this._y = event.screenY / screenPixelsPerCSSPixel - window.mozInnerScreenY;
> > +
> 
> You shouldn't need screenPixelsPerCSSPixel here; screenX/Y should be in CSS
> pixels already.

It appeared to deliver bogus numbers with the test from comment 13, though.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 00:08:07 PDT
(In reply to comment #19)
> One problem is that when the mouse is at the lower left, the status panel
> should appear on the right, rather than appearing on the left and moving to
> the right when the mouse budges. So in order to tell where the status panel
> should show up initially, the current patch tracks the mouse movement in the
> window all the time. Should we be concerned about the overhead of this?

I honestly don't know. You may be forcing a reflow flush on every mouse move which we didn't need before, which could be bad on some pages.
Comment 22 Dão Gottwald [:dao] 2011-05-18 00:42:11 PDT
So just listening to the mousemove event wouldn't be a problem, but accessing mozInnerScreenX could be?
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 00:47:16 PDT
No, it's the call to getBoundingClientRect that will cause a layout flush.
Comment 24 Dão Gottwald [:dao] 2011-05-18 00:50:46 PDT
That's good to know as well, but when the status panel isn't visible, that call wouldn't happen -- we'd only listen to the event and store x and y.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 02:22:14 PDT
Ah, hmm. Then this is probably OK I guess.
Comment 26 Dão Gottwald [:dao] 2011-05-18 02:47:58 PDT
Created attachment 533225 [details] [diff] [review]
patch v3

This avoids calling getBoundingClientRect for every mousemove event while the status panel is open, and fixes bug 629925.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2011-05-18 07:28:46 PDT
> What issues am I supposed to have thought about, exactly? :-)

Things like "how do we expose coordinate transformations to script" and "what should hit testing work like" and so forth... ;)
Comment 28 Dão Gottwald [:dao] 2011-05-20 05:07:39 PDT
There's no reason why this should to be tracked for Firefox 6, although I'd certainly like to see it fixed in Firefox 6.
Comment 29 Dão Gottwald [:dao] 2011-05-22 11:50:49 PDT
Created attachment 534313 [details] [diff] [review]
patch v3

updated to tip
Comment 30 Will Elwood (:Will) 2011-08-24 10:35:13 PDT
For what it's worth, my experience of this problem has been that the tooltip doesn't change sides and flickers constantly rather than once, consuming a lot of CPU until the mouse is moved away from the link. I just tried the test case in Firefox 4.0.1 and 7beta1 (win7x64 aero) to confirm the behaviour described.
Comment 31 bugmenot 2011-09-08 18:31:39 PDT
Regarding comment 30, I noticed the same symptoms.  For me they were caused by the Redirect Remover extension.  It seems to prevent the status box from moving away from the cursor.
Comment 32 Dão Gottwald [:dao] 2011-09-27 05:54:40 PDT
Created attachment 562738 [details] [diff] [review]
patch v3

updated to tip
Comment 33 Dão Gottwald [:dao] 2011-12-28 00:06:32 PST
*** Bug 713808 has been marked as a duplicate of this bug. ***
Comment 35 Marco Bonardo [::mak] 2012-01-13 03:00:39 PST
https://hg.mozilla.org/mozilla-central/rev/35ca03af1b5e

Note You need to log in before you can comment on or make changes to this bug.