Closed Bug 1060642 Opened 5 years ago Closed 5 years ago

Improve display of entries without a title in the Awesomebar

Categories

(Firefox :: Address Bar, defect)

33 Branch
x86
All
defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.3

People

(Reporter: phlsa, Assigned: Unfocused)

Details

Attachments

(2 files, 2 obsolete files)

Attached image 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).
Flags: firefox-backlog+
Flags: qe-verify?
(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?
Points: --- → 3
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
(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
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Flags: qe-verify? → qe-verify-
Iteration: --- → 36.2
redirects have messed up frecency too (bug 737836)
Attached patch WiP v1 (obsolete) — Splinter Review
Not quite done yet - test doesn't work yet. Only working on this when I'm waiting on something for bug 1067903.
Attachment #8519537 - Flags: review?(mak77)
/r/325 - Bug 1060642 - Improve display of entries without a title in the Awesomebar.

Pull down this commit:

hg pull review -r ef76068bda078a575407c2a11b69f28502388f46
Attachment #8516637 - Attachment is obsolete: true
Iteration: 36.2 → 36.3
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 on attachment 8519537 [details]
MozReview Request: bz://1060642/Unfocused

waiting for next version
Attachment #8519537 - Flags: review?(mak77) → feedback+
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.
Attached patch Patch v2Splinter Review
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).
Attachment #8519537 - Attachment is obsolete: true
Attachment #8523489 - Flags: review?(mak77)
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?
Attachment #8523489 - Flags: review?(mak77) → review+
I'm not sure if I should "close discarded" or "close submitted" the open review requests in RB for this bug...
(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.
https://hg.mozilla.org/mozilla-central/rev/45e442b7d671
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.