Switching tabs should update location bar immediately, even if a link is hovered

RESOLVED FIXED in Firefox 4.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jruderman, Assigned: adw)

Tracking

Trunk
Firefox 4.0b8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
Steps to reproduce:
1. Hover over a link.
2. Switch to another tab using a keyboard shortcut.

Result: location bar fades between the two page URLs.  Even worse, when switching back, it additionally adds the link-target to the location bar jankily.

Expected: switch from one state to the other immediately.

This is visually distracting.
(Assignee)

Comment 1

9 years ago
I kind of like this effect.  And what do you mean jankily?
(Reporter)

Comment 2

9 years ago
It's certainly not consistent to use this effect only when one tab has a link and the other doesn't.  I don't like the effect at all; it means there's a period of half a second where there's a mess of two black URLs on top of each other.

It's janky when switching *to* a page where a link is hovered because it transitions between *3* states: old page URL, new page URL shown in full, new page URL shown truncated along with link URL.
(Assignee)

Comment 3

9 years ago
OK, I see your point.  Limi, Alex, UX direction please?
Keywords: uiwanted
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Comment 4

9 years ago
It might be best to not show the link destination in the location bar at all when switching tabs.
(In reply to comment #3)
> OK, I see your point.  Limi, Alex, UX direction please?

Fade the destination URL, replace the current URL immediately, seems to be the best approach. Replacing both immediately also works fine, I'm not partial to any of them.
Yeah I think the transition should be immediate, just as the content area changes immediately.
(In reply to comment #6)
> Yeah I think the transition should be immediate, just as the content area
> changes immediately.

Sold. ;)
(Assignee)

Updated

9 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
Keywords: uiwanted
(Assignee)

Comment 8

9 years ago
Posted patch patch (obsolete) — Splinter Review
Adds overlinkstate="fade-out", whose meaning is what not([overlinkstate]) used to mean, and changes not([overlinkstate]) to mean hide the over-link immediately.

Luckily the over-link is cleared on location change in XULBrowserWindow, which is where setOverLink itself is defined.  So I added an extra param to indicate that the over-link should be hidden immediately and modified urlbarBindings.xml's setOverLink accordingly.

This also hides the over-link immediately when the location bar is focused.  Currently it transitions.

This patch builds on bug 596698 and bug 597338.
Attachment #478955 - Flags: review?(dao)
Comment on attachment 478955 [details] [diff] [review]
patch

setOverLink is part of an XPCOM interface, so just adding the parameter to the implementation isn't right. Not sure the if the argument makes sense at all either, maybe we should rethink the delayed hiding.
Attachment #478955 - Flags: review?(dao) → review-
(Assignee)

Comment 10

9 years ago
Posted patch patch 2 (obsolete) — Splinter Review
(In reply to comment #9)
> Comment on attachment 478955 [details] [diff] [review]
> patch
> 
> setOverLink is part of an XPCOM interface, so just adding the parameter to the
> implementation isn't right.

Fine, but the one caller who uses the extra arg doesn't go through XPCOM, and for the calls that do the extra arg will simply be undefined.  But OK.

> Not sure the if the argument makes sense at all either, maybe
> we should rethink the delayed hiding.

The issue is not the delayed hiding as much as the fade-out transition.  Regardless, there are two distinct and justifiable UX cases here: clicking links should update the location bar immediately, but you should have some leeway to mouse from link to link without the location bar flickering in between.

There are various contexts in which the over-link is shown and hidden that demand various user experiences; I'm struggling with another in bug 596698.  If you have a better way to capture and handle these contexts, I'm all ears.
Attachment #478955 - Attachment is obsolete: true
Attachment #479009 - Flags: review?(dao)
(Assignee)

Comment 11

9 years ago
Requesting blocking (final) because we shouldn't ship Firefox 4 like this.
blocking2.0: --- → ?
blocking2.0: ? → final+
(Assignee)

Comment 12

9 years ago
Posted patch hideOverLinkImmediately patch (obsolete) — Splinter Review
Dão, how's this?  Instead of an extra param to the urlbar's setOverLink, this adds a hideOverLinkImmediately method.  Like the previous patch, it also fixes bug 603883 (but not bug 596698).
Attachment #483333 - Flags: review?(dao)
Whiteboard: [needs review dao]
Comment on attachment 483333 [details] [diff] [review]
hideOverLinkImmediately patch

>-  setOverLink: function (link) {
>+  setOverLink: function (url, anchorElt) {
>     // Encode bidirectional formatting characters.
>     // (RFC 3987 sections 3.2 and 4.1 paragraph 6)
>-    link = link.replace(/[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g,
>-                        encodeURIComponent);
>-    gURLBar.setOverLink(link);
>+    url = url.replace(/[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g,
>+                      encodeURIComponent);
>+    gURLBar.setOverLink(url);
>   },

While you're at it, please add a if (gURLBar) check.

>-    this.setOverLink("", null);
>+    gURLBar.hideOverLinkImmediately();

Thinking of extensions, this seems pretty bad.
How about:

this.hideOverlinkImmediately = true;
this.setOverLink("", null);
this.hideOverlinkImmediately = false;

And let gURLBar.setOverLink access XULBrowserWindow.hideOverlinkImmediately?
(Assignee)

Comment 14

9 years ago
(In reply to comment #13)
> >-    this.setOverLink("", null);
> >+    gURLBar.hideOverLinkImmediately();
> 
> Thinking of extensions, this seems pretty bad.

Could you explain why?  I had actually thought this was good for extensions, since now they can call this new method to hide the over-link immediately if they want.

> How about:
> 
> this.hideOverlinkImmediately = true;
> this.setOverLink("", null);
> this.hideOverlinkImmediately = false;
> 
> And let gURLBar.setOverLink access XULBrowserWindow.hideOverlinkImmediately?

I don't mind doing this if that's what it takes for you to r+, but how is that cleaner than adding a new method?
(In reply to comment #14)
> (In reply to comment #13)
> > >-    this.setOverLink("", null);
> > >+    gURLBar.hideOverLinkImmediately();
> > 
> > Thinking of extensions, this seems pretty bad.
> 
> Could you explain why?  I had actually thought this was good for extensions,
> since now they can call this new method to hide the over-link immediately if
> they want.

I was thinking of extensions implementing setOverLink, not using it.
And thinking one step further, we also don't want extensions using setOverLink call gURLBar.hideOverLinkImmediately, since setOverLink could be implemented differently.
(Assignee)

Comment 17

9 years ago
Posted patch patch v4Splinter Review
I kept a "private" _hideOverLink() convenience function for the urlbar since it needs to hide the over-link immediately itself.  In that case, reaching out and setting hideOverLinkImmediately on XULBrowserWindow seems dumb, so I made another private flag for urlbar called _hideOverLinkImmediately.
Attachment #479009 - Attachment is obsolete: true
Attachment #483333 - Attachment is obsolete: true
Attachment #490297 - Flags: review?(dao)
Attachment #479009 - Flags: review?(dao)
Attachment #483333 - Flags: review?(dao)
Comment on attachment 490297 [details] [diff] [review]
patch v4

>+      <method name="_hideOverLink">
>+        <body><![CDATA[
>+          this._hideOverLinkImmediately = true;
>+          this.setOverLink(null);

this.setOverLink("");

>+            var style = window.getComputedStyle(this._overLinkBox, null);
>+            this._overLinkTransitioning = style.opacity != 1;
>+            this.setAttribute("overlinkstate", aVal);
>+            break;
>+          case "fade-out":
>+            style = window.getComputedStyle(this._overLinkBox, null);

window.getComputedStyle(this._overLinkBox);
Attachment #490297 - Flags: review?(dao) → review+
(Assignee)

Comment 19

9 years ago
http://hg.mozilla.org/mozilla-central/rev/a5401aa629ba
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs review dao]
Target Milestone: --- → Firefox 4.0b8
(Assignee)

Updated

9 years ago
Depends on: 613365
You need to log in before you can comment on or make changes to this bug.