Closed
Bug 596678
Opened 14 years ago
Closed 14 years ago
Link hover in location bar: link appears instantly, without fade, if a link is previously moused over too quickly to show it
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 4.0b7
People
(Reporter: adw, Assigned: adw)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
dao
:
review+
Gavin
:
approval2.0+
|
Details | Diff | Splinter Review |
If you quickly mouse-over and mouse-out a link so that it does not appear in the location bar and then mouse-over that link (or any other link) again, it appears instantly in the location bar. It should instead fade in after some delay as normal.
The problem is that neither the fade-in nor fade-out transitions ever actually starts, despite setting and removing the overlinkstate attribute, which means they never end, which means transitionend isn't fired, which means _overLinkTransitioning is not set to false after having been set to true. So the next time you mouse-over, _overLinkTransitioning is true, and the link is shown instantly.
Assignee | ||
Comment 1•14 years ago
|
||
Hmm, the best way I can think of to fix this is to break the transition in two: fire a timer on mouse-over that implements the initial delay, and when the timer is done set the overlinkstate attribute to start the CSS transitions. The timer can be canceled on mouse-out so that the CSS transitions never start. I was hoping to do everything with CSS though... I wish there were a transitionstart event.
Assignee | ||
Comment 2•14 years ago
|
||
Just talked to David Baron, and my understanding in comment 0 is slightly wrong. transitionend is only fired when a transition has run to completion. So the transitions may (or may not) be starting, but at least they're not ending, which is obvious. (And there're no transitioncanceled or transitionstart events.)
Assignee | ||
Comment 3•14 years ago
|
||
This patch does what comment 1 says: use a timeout for the initial delay. It also:
* increases both the mouseover and mouseout delays to 100ms, which
makes it less likely to trigger the over-link by mistake and gives
you a little more time to mouse from link to link without the
over-link disappearing in between.
* changes the over-link to fade in/out with the default "ease"
transition function, which is snappier. I shouldn't have used a
custom cubic-bezier and linear function here; those were only needed
on the textbox and origin URL label to make that transition look OK.
This makes the over-link and origin URL fade in/out at slightly
different rates, but I think it looks better overall.
Comment 5•14 years ago
|
||
Comment on attachment 475739 [details] [diff] [review]
patch
initialize _overLinkDelayTimer in a <field>
Attachment #475739 -
Flags: review?(dao) → review+
Assignee | ||
Comment 6•14 years ago
|
||
Addresses comment 5. Thanks Dão.
Attachment #475739 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #475907 -
Flags: approval2.0+
Assignee | ||
Comment 7•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
Assignee | ||
Comment 8•14 years ago
|
||
Had to back out because browser_overLinkInLocationBar.js timed out on a Linux debug Mochitest box.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 9•14 years ago
|
||
I don't see much delay here, its still almost instant.
Assignee | ||
Comment 10•14 years ago
|
||
Think I have a fix. The test failure exposed a real problem in my initial patch: _overLinkTransitioning is not reliable. It's not reliable because the transitionend event is not reliable: if you start to transition the opacity to zero and therefore expect to receive transitionend, but the opacity is already zero, naturally transitionend isn't fired. The patch here tickles such a corner case. overlinkstate is set, but before its opacity actually moves off zero, overlinkstate is removed.
What _overLinkTransitioning is really trying to keep track of is whether the over-link's opacity is non-zero. So get rid of it and replace it with a direct test of opacity.
Will push to tryserver to see what happens, attach patch if OK.
Assignee | ||
Comment 11•14 years ago
|
||
Tryserver still coming in, but so far this patch is passing where the old one failed.
This does what comment 10 says. It also makes these other changes from the previous patch:
* If _overLinkDelayTimer is already scheduled when setOverLink is
called, it's canceled immediately (and restarted if needed).
So if you drag your mouse quickly up a long list of links for
example, the over-link won't appear until you stop on one.
* Simplifies the test by calling setOverLink directly rather than
synthesizing a mouse event on a link. I'm not testing the
wiring between mouseover and setOverLink but setOverLink
itself. This also avoids potential focus issues.
Attachment #475907 -
Attachment is obsolete: true
Attachment #476442 -
Flags: review?(dao)
Updated•14 years ago
|
Attachment #476442 -
Flags: review?(dao) → review+
Updated•14 years ago
|
Attachment #476442 -
Flags: approval2.0+
Assignee | ||
Comment 12•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•