1060642 - Improve display of entries without a title in the Awesomebar

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: The problem

Sometimes Firefox doesn't have a page title saved for an awesomebar result. In this case, it just displays the full URL twice.

In some cases, this really clutters up the UI (see attachment) and even if it doesn't the duplicate URL isn't very helpful.

My proposal would be to replace the title with just the eTLD in those cases (e.g. »email.mail.codeacademy.com« in the third row of the attachment).

Comment 1

[Marco Bonardo [:mak]]

(In reply to Philipp Sackl [:phlsa] from comment #0)
> My proposal would be to replace the title with just the eTLD in those cases
> (e.g. »email.mail.codeacademy.com« in the third row of the attachment).

what happens if the subpage has nothing to do with the host? like an hosting solution where the host doesn't change, or internal enterprise domains where you might have mycompany/support, mycompany/documentation...

I think visually it might confuse the user to read the same exact value for all of the pages and then have to further parse the second line to distinguish them.

Maybe we should just strip or de-emphasize the query string to reduce clutter a little bit?

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #1)
> (In reply to Philipp Sackl [:phlsa] from comment #0)
> > My proposal would be to replace the title with just the eTLD in those cases
> > (e.g. »email.mail.codeacademy.com« in the third row of the attachment).
> 
> what happens if the subpage has nothing to do with the host? like an hosting
> solution where the host doesn't change, or internal enterprise domains where
> you might have mycompany/support, mycompany/documentation...
> 
> I think visually it might confuse the user to read the same exact value for
> all of the pages and then have to further parse the second line to
> distinguish them.
> 
> Maybe we should just strip or de-emphasize the query string to reduce
> clutter a little bit?

Well, you still have the entire URL below. Having a shorter and more readable version above would make it easier too scan in blocks (»I'm looking for something on Google, so I'll only look at those entries«).

But really, we should probably find out what exactly triggers this behavior and in which use cases it would come up. The links in my screenshot actually look mostly like tracking URLs that don't have a lot of value anyway.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Philipp Sackl [:phlsa] from comment #2)
> But really, we should probably find out what exactly triggers this behavior
> and in which use cases it would come up. The links in my screenshot actually
> look mostly like tracking URLs that don't have a lot of value anyway.

These will be redirects - we have a few issues around this:
* bug 933726
* bug 922514
* bug 389642
* bug 329983
etc

Comment 4

[Marco Bonardo [:mak]]

redirects have messed up frecency too (bug 737836)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

Not quite done yet - test doesn't work yet. Only working on this when I'm waiting on something for bug 1067903.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: MozReview Request: bz://1060642/Unfocused

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

/r/325 - Bug 1060642 - Improve display of entries without a title in the Awesomebar.

Pull down this commit:

hg pull review -r ef76068bda078a575407c2a11b69f28502388f46

Comment 8

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/325/#review235

::: browser/base/content/test/general/browser_autocomplete_no_title.js
(Diff revision 1)
> +  yield new Promise(resolve => addVisits([{uri: uri, title: ""}], resolve));

you could just copy promiseAddVisits from head_common.js
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#897
into head.js

One day we plan to provide a PlacesTestUtils module with these helpers, so we can stop having tens differens versions of the same functions around.

::: browser/base/content/test/general/head.js
(Diff revision 1)
> -    if (!places[i].title) {
> +    if (typeof places[i].title != "string") {

heh, the problem here is that now you should update all AddVisits and promiseAddVisits implementations in the tree to do this, or in future we could overwrite this with an up-to-date version from another folder, and we'd break your test :(

As I said, a testing module would have made this less painful...

::: toolkit/content/widgets/autocomplete.xml
(Diff revision 1)
> -          // Show the url as the title if we don't have a title
> +          // Show the domain as the title if we don't have a title.
> +          if (title == "") {
> +            try {
> +              let uri = Services.io.newURI(url, null, null);
> +              title = uri.host;
> +            } catch (e) {
> +              // Not all valid URLs have a domain, so fall back to the full URL.
> +            }
> -          if (title == "")
> +            if (title == "")
> -            title = url;
> +              title = displayUrl;

I'd compact this a little bit

// Try to show the domain as the title, if we don't have a title,
// or fallback to the url.
if (title == "") {
  title = displayUrl;
  try {
    let host = Services.io.newURI(url, null, null).host;
    if (host) // Not all valid URLs have a host.
      title == host;
  } catch (e) {}
}

::: toolkit/content/widgets/autocomplete.xml
(Diff revision 1)
> +            item.input = this.mInput;

IIRC the popup has an input accessor, thus from the item you should be able to do .parentNode.parentNode.input (I didn't try) without having to add an explicit new input field and relation.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8519537 [details]
MozReview Request: bz://1060642/Unfocused

waiting for next version

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://reviewboard.mozilla.org/r/325/#review301

> you could just copy promiseAddVisits from head_common.js
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#897
> into head.js
> 
> One day we plan to provide a PlacesTestUtils module with these helpers, so we can stop having tens differens versions of the same functions around.

Now using PlacesTestUtils.jsm from bug 1067903.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Bug 1097213 is forcing me to go old-school for reviews here. This is dependent on the patches in bug 1067903, as it uses PlacesTestUtils.jsm introduced there (see also bug 1100082).

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8523489 [details] [diff] [review]
Patch v2

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

::: browser/base/content/test/general/browser_autocomplete_no_title.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +let {PlacesTestUtils} = Cu.import("resource://testing-common/PlacesTestUtils.jsm", {});

I think it would be nicer to do this in head.js with a defineLazyModuleGetter. I'm sure other tests will very shortly start using this.

::: toolkit/content/widgets/autocomplete.xml
@@ +1295,5 @@
>              this._adjustAcItem();
>            ]]>
>        </constructor>
>  
> +      <field name="input">null</field>

this change should not be needed anymore, right?

Comment 13

[Marco Bonardo [:mak]]

I'm not sure if I should "close discarded" or "close submitted" the open review requests in RB for this bug...

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #13)
> I'm not sure if I should "close discarded" or "close submitted" the open
> review requests in RB for this bug...

Oh, sorry, I meant to do that.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #12)
> > +      <field name="input">null</field>
> 
> this change should not be needed anymore, right?

Oops, indeed.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/45e442b7d671

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45e442b7d671