690191 - Switch to Tab should indicate URL

Status: NEW

(Firefox :: Address Bar, enhancement, P5)

Description

[A Capesius]

User Agent: Mozilla/5.0 (Windows NT 5.2; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0
Build ID: 20110916091512

Steps to reproduce:

With multiple tabs open to the same domain, switch-to-tab only shows the page title when a domain name is entered (example.com). When a URL with a path or querystring is present, the URL is displayed.

With four tabs open to:
www.example.com
www2.example.com
test.example.com
example.com


Actual results:

If a domain name is entered into the location bar and multiple tabs are open they all appear, sorted by left to right appearance order,. All four appear the same unless highlighted.



Expected results:

As it functions now, domain names are being treated as if they only appear as a component of a larger host name.

Each of the four names could refer to a different host.

Switch-to-tab should show the URL in all cases. Results should be sorted by exact match first, then tab order. Domain name example.com should be first in this case. 

For domain names, if the fourth entry isn't open in a tab, a recent URL exact match should appear above the switch-to-tab options.

Comment 1

[Felix Fung (:felix)]

Attachment: Switch to Tab should indicate URL

Append URL to the description string for awesome-bar entry.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 563603 [details] [diff] [review]
Switch to Tab should indicate URL

>+            let desc = "]]>&action.switchToTab.label;<![CDATA[" + ": " + url;

The ": " needs to be localizable somehow. Also please get a ui-review on this.

Comment 3

[Felix Fung (:felix)]

Attachment: Switch to Tab should indicate URL

Changes:
- Made switch to tab strings localizable
- Made sure showing the URL wasn't redundant

Yes, this still needs ui-review.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 563986 [details] [diff] [review]
Switch to Tab should indicate URL

actions.properties appers to be missung?

Comment 5

[Dão Gottwald [:dao]]

*appears ;)

Comment 6

[Felix Fung (:felix)]

Attachment: Switch to Tab should indicate URL

My bad, forgot to hg add the new file.

Comment 7

[:Margaret Leibovic]

Felix, you could upload a screenshot for ui-review. Stephen is probably the best person to advise on the appearance of the urlbar autocomplete entries.

Comment 8

[Felix Fung (:felix)]

Attachment: Screenshot of Change

Note the third entry of the autocomplete dropdown.

Comment 9

[Marco Bonardo [:mak]]

does this also fix bug 555326? at first glance looks like it should show the matching text, the screenshot is no resolutive though (searching for "mozi" should do it).
If so please add dependency.

Comment 10

[Felix Fung (:felix)]

Attachment: Switch to Tab should indicate URL

Additional Change:
- Highlight matching text in the URL. Putting that change here felt more 'feature-complete' of a diff than splitting up into bug 555326

Comment 11

[Felix Fung (:felix)]

Attachment: Highlighting matching URL

Comment 12

[Stephen Horlander [:shorlander]]

Comment on attachment 564645 [details]
Highlighting matching URL

The blue of the "Switch to tab" and the URL runs together a bit.

Changing the "Switch to tab" text to grey descriptive text would break it up a little.

I will attach a mockup. 

Looks good! :)

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: Grey text mockup

Comment 14

[Felix Fung (:felix)]

Attachment: Switch to Tab should indicate URL

Additional Changes:
- Switch to Tab text is now grey/inactive
- When entry is selected on autocomplete, uses HighlightText colour instead

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 565175 [details] [diff] [review]
Switch to Tab should indicate URL

>+            actionBundle = document.getAnonymousElementByAttribute(
>+              this, "role", "string-bundle");

This should probably cached, getting the bundle element for every _setupDescription call doesn't seem ideal.

>+            if (!aText) {
>+              let span = aDescriptionElement.appendChild(
>+                document.createElementNS("http://www.w3.org/1999/xhtml", "span"));
>+              span.textContent = actionBundle.getString("switchToTabLabel");
>+              span.className = "switchtotab-text";
>+              return;
>+            }

Why would aText be null or empty here?

>+            let label = actionBundle.getString("switchToTabURLLabel");
>+            let replacepos = label.indexOf("%S");
>+            let prelabel = label.substr(0, replacepos);
>+            let postlabel = label.substr(replacepos + 2);
>+
>+            let prespan = aDescriptionElement.appendChild(
>+              document.createElementNS("http://www.w3.org/1999/xhtml", "span"));
>+            prespan.textContent = prelabel;
>+            prespan.className = "switchtotab-text";
>+
>+            aDescriptionElement.appendChild(result);
>+
>+            let postspan = aDescriptionElement.appendChild(
>+              document.createElementNS("http://www.w3.org/1999/xhtml", "span"));
>+            postspan.textContent = postlabel;
>+            postspan.className = "switchtotab-text";

Does this display URIs correctly in RTL mode?
postspan should be empty for all locales, so it doesn't look like you need %S at all.

>+html|span.switchtotab-text {
>+  color: GrayText;
>+}
>+
>+.autocomplete-richlistitem[selected="true"] html|span.switchtotab-text {
>+  color: HighlightText;
>+}

This looks like it could be one rule:

.autocomplete-richlistitem:not([selected]) html|span.switchtotab-text {
  color: HighlightText;
}

Instead of the descendant selector, you should use the child selector if possible.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #15)
> This looks like it could be one rule:
> 
> .autocomplete-richlistitem:not([selected]) html|span.switchtotab-text {
>   color: HighlightText;
> }

s/HighlightText/GrayText/

Comment 17

[Felix Fung (:felix)]

Attachment: Switch to Tab should indicate URL

Additional Changes:
- Used child selectors
- Reduced it to one CSS styling
- Cached the actionBundle

Comments:
- We use pre/postlabels precisely for RTL. In an RTL language, there wouldn't be a prelabel, and there'd only be a postlabel.
- In pages with no title, we default the title to be the url. In those cases, we don't need to show the url again beside 'switch to tab:'

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 565358 [details] [diff] [review]
Switch to Tab should indicate URL

>+      <xul:stringbundle src="chrome://global/locale/actions.properties" role="string-bundle" />

nit: remove space before />

>+            this._actionBundle = document.getAnonymousElementByAttribute(
>+              this, "role", "string-bundle");

How come you're calling this attribute "role"? We usually call this anonid.

>--- a/toolkit/themes/gnomestripe/global/autocomplete.css
>+++ b/toolkit/themes/gnomestripe/global/autocomplete.css

>+html|span.switchtotab-text {
>+  color: GrayText;
>+}

You probably meant to remove this.

>+.autocomplete-richlistitem:not([selected]) > .ac-url-box > .ac-url >
>+.ac-action-text > html|span.switchtotab-text {

nit: keep this on one line

Comment 19

[Felix Fung (:felix)]

Attachment: Switch to Tab should indicate URL

- Addressed above comments

Comment 20

[Felix Fung (:felix)]

Ping

Comment 21

[Jennifer Morrow [:Boriss] (UX)]

I'm removing uiwanted, since this has ui and only needs a patch.  Felix, if you need any ui help, please feel free to add uiwanted back or ping Shorlander or myself.

Comment 22

[Matthew N. [:MattN]]

Attachment: Rebased Felix's patch

I rebased Felix's patch from 9 months ago (only the MPL header change caused a problem).

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 636957 [details] [diff] [review]
Rebased Felix's patch

Review of attachment 636957 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the one thing below, this looks good to me.  Dão, any chance you can review this shortly?  It would be good to land in the same release as bug 587909.

::: toolkit/locales/en-US/chrome/global/actions.dtd
@@ +1,3 @@
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

This file can be deleted now from what I can tell.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 636957 [details] [diff] [review]
Rebased Felix's patch

>--- a/toolkit/locales/en-US/chrome/global/actions.dtd
>+++ b/toolkit/locales/en-US/chrome/global/actions.dtd
>@@ -1,5 +1,4 @@
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> 
>-<!ENTITY action.switchToTab.label "Switch to tab">

This file should be removed.

>--- /dev/null
>+++ b/toolkit/locales/en-US/chrome/global/actions.properties
>@@ -0,0 +1,2 @@
>+switchToTabLabel=Switch to tab
>+switchToTabURLLabel=Switch to tab: %S

I'm not sure why this file is called actions.properties. How about autocomplete.properties?

Comment 25

[Dão Gottwald [:dao]]

> >--- /dev/null
> >+++ b/toolkit/locales/en-US/chrome/global/actions.properties
> >@@ -0,0 +1,2 @@
> >+switchToTabLabel=Switch to tab
> >+switchToTabURLLabel=Switch to tab: %S
> 
> I'm not sure why this file is called actions.properties. How about
> autocomplete.properties?

Also, add the license header.

Comment 26

[Axel Hecht [:Pike]]

Comment on attachment 636957 [details] [diff] [review]
Rebased Felix's patch

Review of attachment 636957 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/locales/en-US/chrome/global/actions.properties
@@ +1,2 @@
> +switchToTabLabel=Switch to tab
> +switchToTabURLLabel=Switch to tab: %S

It'd be great to get a localization note here to explain that %S is a url. Could be a number or title or so, too.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 636957 [details] [diff] [review]
Rebased Felix's patch

Review of attachment 636957 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/autocomplete.xml
@@ +1193,5 @@
>    </binding>
>  
>    <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>      <content>
> +      <xul:stringbundle src="chrome://global/locale/actions.properties" anonid="string-bundle"/>

In bug 480350, we decided to stay away from using a string bundle, to avoid the overhead of accessing it for (potentially) every item whenever the list was refreshed (see the end of bug 480350 comment 100). If a string bundle is really wanted here, I think it should be used in the parent richlistbox, with the strings memorized.

Comment 29

[Graham Perrin]

With Firefox 55.0.3 (64-bit) on FreeBSD-CURRENT: 

1. where the 'Switch to Tab' icon appears, without that phrase

2. refrain from a normal click

3. present the contextual menu, e.g. right-click

4. horizontally adjacent to the click, observe either (a) the phrase 'Switch to Tab'; or (b) the Firefox Sync name of the device that is associated with the tab – AND

5. in the location field, observe the URL of the tab. 



Exception
=========

If auto-completion caused a different URL to appear before steps (4) and (5), then: 

3. again, right-click