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.

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

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.

Changes: - Made switch to tab strings localizable - Made sure showing the URL wasn't redundant Yes, this still needs ui-review.

Comment on attachment 563986 [details] [diff] [review] Switch to Tab should indicate URL actions.properties appers to be missung?

*appears ;)

My bad, forgot to hg add the new file.

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.

Note the third entry of the autocomplete dropdown.

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.

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 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! :)

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

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.

(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/

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 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

- Addressed above comments

Ping

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.

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

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 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?

> >--- /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 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 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.

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