596485 - Provide visual indication of Switch to Tab override

Status: VERIFIED FIXED

(Firefox :: Address Bar, enhancement)

Description

[Gordon P. Hemsley [:GPHemsley]]

This was discussed a while back, and I'm not sure what became of the discussion. Beltzner requested I file this bug anew, to ensure it doesn't get forgotten.

The new Switch to Tab feature automatically detects when a particular URL is already open in another tab (when typing the location bar). It then gives you the option to switch to that tab, rather than accidentally duplicating it. However, there are times when that duplication is intended, especially if (for example) the tab it wants to switch to is in another Panorama group than the one you're currently working in.

There is already the option to open that result in a new tab by holding down Alt/Option when selecting it. However, that then removes focus from (and effectively orphans) your already-open "New Tab" tab. Also, the pressing of the key does not change the appearance of the "Switch to Tab" portion of the result (which itself replaces the URL).

There should be a key command (e.g. Shift or Control/Command) that allows you to override the "Switch to Tab" feature while also visually reflecting that keypress onscreen.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Blair - what do you think?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Makes sense and is doable. The visual indication would be the hardest part - especially since we don't do that for any other type of autocomplete entry. 

The actual behaviour is just a matter of detecting the relevant key is held down here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#838

I'm swamped with the EM rewrite still, so it would be awesome if someone else could pick this up.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Visual indication seems easy: when the key is held down, the "Switch to tab" part vanishes.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Easy conceptually, sure, but not easy programmatically given the way those indicators are implemented :)

Comment 5

[Drew Willcoxon :adw]

As comment 0 kind of says, there already is a way to override switch-to-tab: the alt/option key.  What this bug is really about is visually indicating that, right?  I have a proof-of-concept patch that does what comment 3 says.

Comment 6

[Drew Willcoxon :adw]

Should pressing alt change all the switch-to-tab results or only the highlighted one?

Comment 7

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #5)
> As comment 0 kind of says, there already is a way to override switch-to-tab:
> the alt/option key.  What this bug is really about is visually indicating that,
> right?  I have a proof-of-concept patch that does what comment 3 says.

Actually, no. As comment 0 says, alt/option-enter is just a way to open a link in a new tab. Usually, you have already opened a new tab, so doing that on a Switch-to-Tab listing will create _another_ new tab, leaving an orphaned empty tab. There needs to be a way to open the switch-to-tab listing in the _current_ tab.

(In reply to comment #6)
> Should pressing alt change all the switch-to-tab results or only the
> highlighted one?

I would say it should change all of them.

Comment 8

[Drew Willcoxon :adw]

This just sounds like bug 595046 then.  What does visual indication have to do with it?  I think you are describing at least two bugs/RFEs.

Comment 9

[Drew Willcoxon :adw]

I'm sorry, I'm getting confused about what it means to override switch-to-tab.  I was thinking you meant what alt currently does.  But I guess you mean treating switch-to-tab entries as normal entries.  Both override switch-to-tab.

So does the first case need visual indication too?  And what happens if you press the key for each case simultaneously and hit enter?

Comment 10

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #8)
> This just sounds like bug 595046 then.  What does visual indication have to do
> with it?  I think you are describing at least two bugs/RFEs.

Hmm... bug 595046 refers to bug 573580, which refers as far back as bug 320989.

(In reply to comment #9)
> I'm sorry, I'm getting confused about what it means to override switch-to-tab. 
> I was thinking you meant what alt currently does.  But I guess you mean
> treating switch-to-tab entries as normal entries.  Both override switch-to-tab.
> 
> So does the first case need visual indication too?  And what happens if you
> press the key for each case simultaneously and hit enter?

All of the aforementioned bugs really point to about the same thing, albeit from different directions.

The ability to use alt-enter to open a link in a new window/tab has been around forever. From that comes the desire to not "waste" a tab if alt-enter is used on a brand new, empty tab.

Newly introduced is the ability to switch to an already-open tab, in an attempt to prevent unwanted duplicate tabs. However, there are times when reopening the same page in another tab is the desired behavior. There is not currently a way to do that (without creating an empty tab via alt-enter).

Thus, this bug was suggesting:
(1) add a separate modifier key to reopen an already open tab
(2) revert the "switch to tab" messages to the URL when the modifier key is held.

However, given this new evidence (I wasn't aware of all those other bugs before), the ideal situation would probably be to resolve all these issues at once:
(1) always use the current tab on alt-enter if the current tab is empty
(2) allow alt-enter to override the switch-to-tab behavior, as well
(3) revert the "switch to tab" message to the URL when the Alt key is held down

That would fix all of the aforementioned bugs, including this current one, I think.

Comment 11

[Drew Willcoxon :adw]

I don't know what to do here, so until someone from UX provides guidance I'm going to work on something else.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

FWIW, I'd expect to see all switch-to-tab results turn into normal results, for as long as the key is held down.

Comment 13

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #12)
> FWIW, I'd expect to see all switch-to-tab results turn into normal results, for
> as long as the key is held down.

Yes, I agree. (I'm not sure if I made that clear.)

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #11)
> I don't know what to do here, so until someone from UX provides guidance I'm
> going to work on something else.

ui-design is:

 - when ALT or SHIFT are held, the "Switch to tab" text is removed from awesomebar results
 - when ALT is held down, the URL opens in a new tab; if the current tab is blank, then the current tab should be closed
 - when SHIFT is held down, the URL opens in the current tab

(note: while inconsistent with CTRL+Click which opens links in new tabs, this is consistent with ALT+enter which opens URLs in new tabs and searches in new tabs)

Comment 15

[Drew Willcoxon :adw]

Thanks Mike, that makes sense.  One problem I'm running into though is Shift: I can make it replace "Switch to tab" with the URLs, but that's annoying if you're using Shift to mean "capitalize this letter I'm typing".  A potential solution would be to wait until a small delay elapses and the user hasn't typed any characters, and only then replace the "Switch to tab" text.  Other ideas?

(A simpler solution would be to not use Shift alone, but Shift + another modifier or another key entirely.)

Comment 16

[Gordon P. Hemsley [:GPHemsley]]

(In reply to comment #15)
> Thanks Mike, that makes sense.  One problem I'm running into though is Shift: I
> can make it replace "Switch to tab" with the URLs, but that's annoying if
> you're using Shift to mean "capitalize this letter I'm typing".  A potential
> solution would be to wait until a small delay elapses and the user hasn't typed
> any characters, and only then replace the "Switch to tab" text.  Other ideas?
> 
> (A simpler solution would be to not use Shift alone, but Shift + another
> modifier or another key entirely.)

I would support a brief delay, even for Alt/Option, as those keys also have other possible meanings. (It's been a while since I used Alt on Windows, so I don't remember what can be done with it, but I know Option is used to enter special characters on Mac.)

Chances are, if you're using Shift or Alt for the regular-text meanings, you'll press them a lot quicker than if you're using them as a shortcut meaning, I would think. Might I recommend a 50 ms delay?

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #15)
> any characters, and only then replace the "Switch to tab" text.  Other ideas?

Well, we can go to CTRL. It'll be annoying when entering things like é or other accented characters, but less so than others.

Other possibility is to show it after shift is held down for more than 1s, but that gets twitchy.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

We can make the modifiers only take effect when an entry in the dropdown is selected, right? It's still possible to enter text in those cases, but it's less likely to occur (you're more likely to just be finding an entry to select).

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Gavin is smart. That makes a lot more sense, yes.

Comment 20

[Drew Willcoxon :adw]

It's not quite that simple, since "Switch to tab:" can be present in the URL bar even while the popup is closed, and presumably the modifiers should take effect then, too (and hide the "Switch to tab:" prefix).  So I guess the conditional is: if an entry is selected in the popup or the URL bar contains a switch-to-tab action.

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #20)
> It's not quite that simple, since "Switch to tab:" can be present in the URL
> bar even while the popup is closed

That makes sense - since "Switch to tab:" will already automatically disappear (ie, the URL bar will switch back to the normal state) as soon as any additional text is entered.

Comment 22

[gadjo]

To be honest I don't like the idea of modifiers... Why not just show to options:

URLBAR: [ http://google ]
  LIST: - http://google.com
        - switch to tab: http://google.com
        - http://google.com/analytics
        - ....

Comment 23

[Drew Willcoxon :adw]

Attachment: patch

Makes the changes of comment 14 with the suggestion of comment 18, ignoring my own suggestion in comment 20 because I don't think it's necessary.

Per comment 14, when alt is held and the current tab is blank, that tab is closed before the new one is opened.  However, that's done regardless of whether the current entry is a switch-to-tab.  How you like me now?

Gavin, we talked about my approach awhile ago on IRC.  To recap: rather than modifying autocomplete entries when shift or alt is held, this patch sticks another xul:description in each entry that is only for action text.  When shift or alt is pressed, a "noactions" attribute is set on the autocomplete panel, and CSS is used to toggle between the new action xul:description and the URL xul:description.  Additionally, the actiontype attribute is removed from the URL bar to hide the "Switch to tab:" prefix label.

It was a little tricky working with the overflow ellipses.  Toggling from "Switch to tab" to the URL might overflow the URL's allotted space, which means it needs an overflow ellipsis.  So I hooked that up, but I ran into a problem where the ellipses "pop" in: you toggle to the URL, the full URL appears, and then a split second later its ellipsis appears on top of the last few characters.  Ugly.  So I modified the ellipses slightly so that they always take up space, even when they're not shown (by switching from using the hidden attribute to style.visibility).  That way when the ellipses do appear, they don't pop in over text that's already showing.  Finally I noticed that overflow is currently broken for both URLs and titles: if you start Firefox wide, open the URL bar dropdown by clicking its dropdown arrow, narrow the window so that some titles and URLs in the autocomplete will become truncated, and open the dropdown again, those titles and URLs don't have ellipses.  So this patch fixes that.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 492502 [details] [diff] [review]
patch

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css

>+panel:not([noactions]) > richlistbox > richlistitem[type~="action"] > .ac-url-box > .ac-url > .ac-url-text {
>+  visibility: collapse;
>+}

This appears to duplicate xul.css code.

>--- a/toolkit/content/xul.css
>+++ b/toolkit/content/xul.css

>+.autocomplete-richlistitem[type~="action"] > .ac-url-box > .ac-url > .ac-url-text,
>+.autocomplete-richlistitem:not([type~="action"]) > .ac-url-box > .ac-url > .ac-action-text {
>+  visibility: collapse;
>+}

Seems like these nodes should inherit the action attribute.

Comment 25

[Drew Willcoxon :adw]

Attachment: patch 2

Thanks Dão.  I'm sure Gavin won't mind if you steal this review.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 494083 [details] [diff] [review]
patch 2

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>     <handlers>
>+      <handler event="keydown"><![CDATA[
>+        if ((event.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_ALT ||
>+             event.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_SHIFT) &&

Ci.nsIDOMKeyEvent can be shortened to just "KeyEvent" in window contexts.

>+            this.popup.selectedIndex >= 0) {

Hmm, shouldn't this check be for whether the popup is open, rather than whether an item is selected?

>+          this._numNoActionsKeys = this._numNoActionsKeys || 0;

A field with a default value would remove the need for this.

>+            let action = this._parseActionUrl(this._value);

I think this should use this.value, to avoid relying on our onBeforeValueGet having been called (I think that may be garanteed, but better safe than sorry - there's no downside AFAICT).

>+      <handler event="blur"><![CDATA[
>+        this.popup.removeAttribute("noactions");
>+        this._numNoActionsKeys = 0;

Doesn't this need to potentially restore "actiontype" too?

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>           // Emphasize the matching search terms for the description
>           this._setUpDescription(this._title, title);
>-          if (setupUrl)
>-            this._setUpDescription(this._url, url);
>+          this._setUpDescription(this._url, url);

Hmm, kind of unfortunate that we'll now always do the non-trivial amount of work to set up the URL label (with emphasis and such) even though in the common case (no modifiers pressed) the URL will never be shown. I guess there's no easy way around that though, and it only applies to tab matches (of which there will typically be few).

>diff --git a/toolkit/content/xul.css b/toolkit/content/xul.css

>+.ac-url-text[type~="action"],
>+.ac-action-text:not([type~="action"]) {
>+  visibility: collapse;
>+}

We should really have an autocomplete.css under content/ that contains these rules (and the other autocomplete class rules in xul.css). I filed bug 616258 for that.

Comment 27

[Drew Willcoxon :adw]

Thanks Gavin.

(In reply to comment #26)
> >+            this.popup.selectedIndex >= 0) {
> 
> Hmm, shouldn't this check be for whether the popup is open, rather than whether
> an item is selected?

You suggested it in comment 18! :D  (To recap: you might use the shift key while typing a search, and it's distracting to see the switch-to-tabs toggle every time you do.  So you suggested to wait until there's a selection in the popup, and everyone thought that's a good idea.)

> >+            let action = this._parseActionUrl(this._value);
> 
> I think this should use this.value, to avoid relying on our onBeforeValueGet
> having been called (I think that may be garanteed, but better safe than sorry -
> there's no downside AFAICT).

I tried, and at this point this.value never contains moz-action.  That's because this.value is the value property on the autocomplete binding, which calls the urlbarbinding's onBeforeValueGet, which checks for the actiontype attribute, which is not present since I removed it when shift/ctrl was pressed.  onBeforeValueGet therefore returns null, which causes this.value to return this.inputField.value.

> >+      <handler event="blur"><![CDATA[
> >+        this.popup.removeAttribute("noactions");
> >+        this._numNoActionsKeys = 0;
> 
> Doesn't this need to potentially restore "actiontype" too?

Hmm, probably.  It doesn't look like anything assumes that if _value contains moz-action then the actiontype attribute will be present, but I'll fix it.

> >           // Emphasize the matching search terms for the description
> >           this._setUpDescription(this._title, title);
> >-          if (setupUrl)
> >-            this._setUpDescription(this._url, url);
> >+          this._setUpDescription(this._url, url);
> 
> Hmm, kind of unfortunate that we'll now always do the non-trivial amount of
> work to set up the URL label (with emphasis and such) even though in the common
> case (no modifiers pressed) the URL will never be shown. I guess there's no
> easy way around that though, and it only applies to tab matches (of which there
> will typically be few).

Yeah.  I can look into doing a one-time setup of the popup when shift/ctrl is first pressed.

Comment 28

[Drew Willcoxon :adw]

Attachment: patch 3

This patch addresses comment 26 modulo comment 27 and adds a lazy one-time setup (per popup opening) of URLs in action autocomplete items.  It pushes "noactions" from browser into toolkit autocomplete.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 495101 [details] [diff] [review]
patch 3

(In reply to comment #27)
> You suggested it in comment 18! :D

Gavin from the past has all the best ideas.

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>           this._appendCurrentResult();
>+
>+          this._areActionUrlsSetUp = false;
>+          if (this.noActions)
>+            this._setUpActionUrls();

This won't work reliably, since _appendCurrentResult only adds the first chunk of results (setting a timeout to yield between chunks). There is precedent for autocomplete items reaching up to their panels (panel = this.parentNode.parentNode in the label getter), so I suppose you could add a "noActions" argument to _adjustAcItem(), and have the constructor invocation pass in panel.noActions (the other invocation is in the popup, so it can pass this.noActions directly)... but I think this is getting too complicated. Can you measure the amount of time taken setting up the extra label? It's probably actually negligible, and it'd only affect the action entries, so maybe we should just give up on trying to avoid the extra work and go with your earlier approach.

Comment 30

[Drew Willcoxon :adw]

Attachment: patch 4

Yeah, I agree.  Here's the simpler patch.  I'll get measurements when it's not Friday night and ask for review then.

Comment 31

[Drew Willcoxon :adw]

Ergh, I requested review out of habit.  I'll just leave it. :)

Comment 32

[Drew Willcoxon :adw]

On my debug build, calling

  this._setUpDescription(this._url, url);

takes from 0 to 8 ms.  Out of 104 runs, the average was 2ms.  Long URLs with lots of matches (which have to be highlighted) seem to take longest.  So that's 12 * 8 = 96ms of extra work in the worst case on my debug build, if all entries are switch-to-tab and their URLs are long with lots of matches.  It's 12 * 2 = 24ms in the average case if all entries are switch-to-tab.  I don't think it's worth the complexity of the previous patch I posted.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 495205 [details] [diff] [review]
patch 4

>diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css

>+panel[noactions] > richlistbox > richlistitem[type~="action"] > .ac-url-box > .ac-action-icon {

Looks like we never bothered hiding ac-action-icon before this patch (but rather just toggled whether or not it had an image specified), so I guess this isn't strictly necessary, but I suppose it doesn't hurt.

>diff --git a/toolkit/content/xul.css b/toolkit/content/xul.css

Add a /* FIXME: bug 616258 */ next to the relevant rules here (some of which you're adding)?

Comment 34

[Drew Willcoxon :adw]

http://hg.mozilla.org/mozilla-central/rev/5113288ca83e

Comment 36

[John A. Bilicki III]

The switch-to-tab feature should be removed from Firefox and made available as an extension. I misstyped a URL and was trying to remove it from auto-complete when this feature would not go away. Pressing the down key to select the item I intended to press Shift+Delete to remove the item and it removed everything and kept the tab open. The feature should not have shipped with Firefox incomplete as it was. Is there a bug to vote to have the feature removed altogether?

Comment 37

[u279076]

(In reply to comment #36)
> Is there a bug to vote to have the feature removed altogether?

Not that I am aware.  Feel free to file a new bug and mark it as an "enhancement".

Comment 38

[John A. Bilicki III]

An enhancement is something that extends functionality while a bug fix is something that corrects a bug that prevents functionality, therefore I have filed a bug report to have the feature removed.

For those watching this bug who also find this feature completely unwarranted for implementation beyond as an extension you may find my bug report here...

https://bugzilla.mozilla.org/show_bug.cgi?id=650039

Comment 39

[BikeHelmet]

A way to turn switch-to-tab off would be nice. I don't forget which tabs are open, or where they open, so I don't need that functionality at all. On the other hand, I often open the same bookmarks multiple times. (through the address bar)

Shift detection in Firefox isn't perfect - in particular when pages are loading in the background - so I have accidentally switched to another tab numerous times.

Comment 40

[[not reading bugmail]]

(In reply to comment #39)
> A way to turn switch-to-tab off would be nice. I don't forget which tabs are
> open, or where they open, so I don't need that functionality at all. On the
> other hand, I often open the same bookmarks multiple times. (through the
> address bar)
> 
> Shift detection in Firefox isn't perfect - in particular when pages are loading
> in the background - so I have accidentally switched to another tab numerous
> times.

see bug 530209.