Closed Bug 597581 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: adw)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
I kind of like this effect.  And what do you mean jankily?
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.
OK, I see your point.  Limi, Alex, UX direction please?
Keywords: uiwanted
OS: Mac OS X → All
Hardware: x86 → All
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: nobody → adw
Status: NEW → ASSIGNED
Keywords: uiwanted
Attached 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-
Attached 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)
Requesting blocking (final) because we shouldn't ship Firefox 4 like this.
blocking2.0: --- → ?
blocking2.0: ? → final+
Attached 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?
(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.
Attached 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+
http://hg.mozilla.org/mozilla-central/rev/a5401aa629ba
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs review dao]
Target Milestone: --- → Firefox 4.0b8
Depends on: 613365
You need to log in before you can comment on or make changes to this bug.