Closed
Bug 597581
Opened 14 years ago
Closed 14 years ago
Switching tabs should update location bar immediately, even if a link is hovered
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 4.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: jruderman, Assigned: adw)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
I kind of like this effect. And what do you mean jankily?
Reporter | ||
Comment 2•14 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•14 years ago
|
||
OK, I see your point. Limi, Alex, UX direction please?
Reporter | ||
Comment 4•14 years ago
|
||
It might be best to not show the link destination in the location bar at all when switching tabs.
Comment 5•14 years ago
|
||
(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.
Comment 6•14 years ago
|
||
Yeah I think the transition should be immediate, just as the content area changes immediately.
Comment 7•14 years ago
|
||
(In reply to comment #6)
> Yeah I think the transition should be immediate, just as the content area
> changes immediately.
Sold. ;)
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 8•14 years ago
|
||
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 9•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
Requesting blocking (final) because we shouldn't ship Firefox 4 like this.
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → final+
Assignee | ||
Comment 12•14 years ago
|
||
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)
Updated•14 years ago
|
Whiteboard: [needs review dao]
Comment 13•14 years ago
|
||
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•14 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?
Comment 15•14 years ago
|
||
(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.
Comment 16•14 years ago
|
||
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•14 years ago
|
||
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 18•14 years ago
|
||
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•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs review dao]
Target Milestone: --- → Firefox 4.0b8
You need to log in
before you can comment on or make changes to this bug.
Description
•