Closed Bug 1256074 Opened 8 years ago Closed 8 years ago

Always present a 'search' option when the location bar search is "fixed" to an URL

Categories

(Firefox :: Address Bar, defect, P2)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: KWierso, Assigned: evanxd)

References

(
URL
)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file, 2 obsolete files)

Bug 1256074 - Always present a 'search' option when the location bar search is "fixed" to an URL.
58 bytes, text/x-review-board-request
mak
: review+
Details 
If I type "http.listenandserve" into the location bar, the only action shown is to "Visit http://http.listenandserve/" because it was detected as a URL to visit, but I wanted to search for "http.listenandserve" to find out how to use it.  

It'd be nice if there was still an option listed to search for "http.listenandserve" with my default engine, at least if the string after the '.' isn't detected as a common tld.
This would really be helpful to better match other browsers behavior. AFAICT Firefox is the only one that doesn't provide URL search in the Awesomebar. It would also remove one big disadvantage disabling the search bar (only way to search an URL currently).
I agree, we should evaluate this.

Stephen, what do you think? When we offer to visit something that looks like a domain, should we also offer to search for it ina separate entry? Would that take up too much space (in the end when typing a domain it's less likely for the user to search for something deeper, indeed we do "to the next slash" autofill)
Did you have ideas about alternative approaches to solve the same problem?
Flags: needinfo?(shorlander)
Priority: -- → P2
Whiteboard: [fxsearch]
Thank you for taking this into consideration! I was the guy asking the question on reddit and Wes was kind enough to create a bug for it. 
The main problem for me is not that Firefox doesn't allow to search for URLs, but that it recognizes everything with a dot in it as an URL. The "http.ListenAndServe" example is a function from golang's http library. A visit option should probably not even be there. Maybe an easy solution would be to test against the list of valid TLDs and show search or visit options based on that? Searching the 1300 valid TLDs should be very fast, a lot quicker than the bookmark search that is already happening.
Blocks: 1262507
I think the last time _I_ reported this, it was pretty much considered and rejected by Marco
in bug 1080682.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(shorlander)
Resolution: --- → DUPLICATE
not exactly a dupe, I still think we should keep this as a fall back, since I'm not sure bug 1080682 can easily be done.
I don't think I rejected the idea, I think I only expressed some concerns. Otherwise, maybe I was just wrong.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to Marco Bonardo [::mak] (Away 6-20 Aug) from comment #5)
> not exactly a dupe, I still think we should keep this as a fall back, since
> I'm not sure bug 1080682 can easily be done.
I never said that this is duplicate of bug 1080682.
I know that _this_ is duplicate my _open_ bug 1198832 filed long time ago with explanation.
(In reply to arni2033 from comment #6)
> I never said that this is duplicate of bug 1080682.
> I know that _this_ is duplicate my _open_ bug 1198832 filed long time ago
> with explanation.

We don't duplicate bugs based on filed date, it's mostly based on usefulness of the contained comments. The original duplication there made sense cause the only solution was to use the PSL. This bug suggests a different solution, so honestly the duplication was correct. But just in case I updated all the dependencies.
Hi Marco,

Look like we need to add two `richlistitem` elements(moz-action:visiturl and moz-action:searchengine ones)[1] on the `#PopupAutoCompleteRichResult` panel when the `moz-action` is `visiturl` to fix the issue.

I would like to give it a shot.

[1]: https://github.com/mozilla/gecko-dev/blob/master/toolkit/content/widgets/autocomplete.xml#L1220
Assignee: nobody → evan
Status: REOPENED → ASSIGNED
But look like we also need UX input here to ensure(always present a `search` option) the behavior is what we want.

Hi Stephen,

How do you think of Comment 2 Marco mentioned?

> Stephen, what do you think? When we offer to visit something that looks like
> a domain, should we also offer to search for it ina separate entry? Would
> that take up too much space (in the end when typing a domain it's less
> likely for the user to search for something deeper, indeed we do "to the
> next slash" autofill)
> Did you have ideas about alternative approaches to solve the same problem?
Flags: needinfo?(shorlander)
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #10)
> Look like we need to add two `richlistitem` elements(moz-action:visiturl and
> moz-action:searchengine ones)[1] on the `#PopupAutoCompleteRichResult` panel
> when the `moz-action` is `visiturl` to fix the issue.

I think it will be enough to do some changes to UnifiedComplete.js, we don't want a UI hack like showing/hiding boxes for this.
Attached patch bug-1256074.patch (obsolete) — — Splinter Review 
Hi Stephen,

Could you review the UI change[1]?

In the video[1], when an user inputs `node.js` keyword, the URL bar will give search `node.js` option not only `visiturl` option. It enhances UX.

And if you're using Mac OS, you can download the build[2] and use it.

What do you think of the UI change?

[1]: https://youtu.be/6q8ZKEufvUI
[2]: https://drive.google.com/open?id=0B3MxK4_nj3bpUEw3eElHeFBjWHM
Attachment #8784759 - Flags: ui-review?(shorlander)
Comment on attachment 8784759 [details] [diff] [review]
bug-1256074.patch

Hi Marco,

Thanks for the suggestions in Comment 12. It's so helpful to write the patch.

Could you give feedback for it?

Thanks.
Attachment #8784759 - Flags: feedback?(mak77)
Comment on attachment 8784759 [details] [diff] [review]
bug-1256074.patch

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

::: toolkit/components/places/UnifiedComplete.js
@@ +1328,5 @@
>      }
>  
>      this._addMatch(match);
> +    // Always add a `searchengine` match.
> +    yield this._matchCurrentSearchEngine();

I'd probably do this at the calling point, to avoid mixing up the code, so where we do:

 let matched = yield this._matchUnknownUrl();
      if (matched) {
======> here
        return;

Also, I think this deserves a larger comment briefly explaining why we also want to add a searchengine match that boils down to something like: "Since we can't tell if this is a real url and whether the user wanted to visit or search for it, we always provide an alternative searchengine match."

Finally, I don't think we should provide a search option if we didn't "fix" what was typed. Basically, if the user typed a valid URI (scheme//domain/path) it's very unlikely he needs to search for it. The cases where search is needed is when something LOOKS LIKE a uri and we "fix" it to be an uri.
That could be done by checking if new URL(this._originalSearchString) throws. it's a little bit more expensive, but it's only done when _matchUnknownUrl returns true...
Attachment #8784759 - Flags: feedback?(mak77)
Summary: Always present a 'search' option, even location bar's contents are detected as URLs → Always present a 'search' option when the location bar search is "fixed" to an URL
Attached patch bug-1256074.patch (obsolete) — — Splinter Review 
Updated the patch for feedbacks in Comment 15.
Comment on attachment 8788726 [details] [diff] [review]
bug-1256074.patch

Hi Marco,

Updated the patch for your comments.
Thanks for the review and clear comments.

Please help review it.
Thanks.


The try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfa7b19965ff
Attachment #8788726 - Flags: review?(mak77)
Attachment #8784759 - Attachment is obsolete: true
Attachment #8784759 - Flags: ui-review?(shorlander)
Comment on attachment 8788726 [details] [diff] [review]
bug-1256074.patch

Hi Stephen,

Could you review the UI change[1]?

In the video[1], when an user inputs `node.js` keyword, the URL bar will give search `node.js` option not only `visiturl` option. It enhances UX.

You can download the below builds to try the UI changes.
Mac OS: https://archive.mozilla.org/pub/firefox/try-builds/itoyxd@gmail.com-bfa7b19965ffc3f5f29a3aa69fdba9fcb807de84/try-macosx64/
Linux: https://archive.mozilla.org/pub/firefox/try-builds/itoyxd@gmail.com-bfa7b19965ffc3f5f29a3aa69fdba9fcb807de84/try-linux64/
Windows: https://archive.mozilla.org/pub/firefox/try-builds/itoyxd@gmail.com-bfa7b19965ffc3f5f29a3aa69fdba9fcb807de84/try-win64/

What do you think of the UI change?

[1]: https://youtu.be/6q8ZKEufvUI
[2]: https://drive.google.com/open?id=0B3MxK4_nj3bpUEw3eElHeFBjWHM
Attachment #8788726 - Flags: ui-review?(shorlander)
Hi Philipp,

Let me answer the question here.

> To confirm: this would NOT happen for known TLDs like .com .net .de and the like right?
> If the item would also show up for TLDs like those, I would consider that a blocking issue.

Yes, if the search keyword is a valid URI(scheme//domain/path), just like Marco mentioned in Comment 15.
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #19)
> > To confirm: this would NOT happen for known TLDs like .com .net .de and the like right?
> > If the item would also show up for TLDs like those, I would consider that a blocking issue.
> 
> Yes, if the search keyword is a valid URI(scheme//domain/path) [...]

I don't think people type in the scheme most of the time. If it's considered undesirable to show the search option for known valid hosts, excluding only complete URIs will not accomplish that. Rather, we should see if the host is in the browsing history.

I was under the impression that that's how this was going to work, but now I'm no longer sure.

To illustrate, assume I've never visited Reddit. Upon typing in "reddit.com" for the first time, the default action would be "Visit", and the secondary action would be "Search". On subsequent occasions, "Search" would no longer be displayed, as "www.reddit.com" is in the history. Correct? No?

And note I said "host". If http://kb.mozillazine.org/Browser.urlbar.match.url is found in my history, it doesn't mean I'd like to visit http://urlbar.match.url when I type in "urlbar.match.url" again.
Note we are still waiting for UX to evaluate this, so let's not go too far into discussions for now :)

The fact is Nightly already has a way to do this, through one-off buttons, you can just type whatever and use the default engine one-off search button. So this workaround would only be needed until we get one-off buttons in Release. That makes all of this very much depending on the fate and timing of that feature.
Wait, this feature is going to be a temporary workaround? But there are already at least two perfectly fine workarounds:

* Prefix a single or double quote -- example: 'node.js
* Use the shortcut to your preferred search engine -- example: g node.js
those are not discoverable. Btw, I'm just saying there's no point in going too deep into discussing this until we get ux feedback.
The one-off search buttons would fix this by providing a persistent way to always search. But given that this is a pretty specific use-case elevating the search option into the list, even though it creates a slight redundancy, seems worth it.
Flags: needinfo?(shorlander)
(In reply to Stephen Horlander [:shorlander] from comment #24)
> The one-off search buttons would fix this by providing a persistent way to
> always search. But given that this is a pretty specific use-case elevating
> the search option into the list, even though it creates a slight redundancy,
> seems worth it.

Hi Stephen,

Looks like you agree with that (Comment 19) show search option when the search keyword is a valid URI(scheme//domain/path), right? If so, could you set the "ui-review"(Comment 18) flag as "ui-review+" for the patch? Or you could continue to review it with the video[1] or using the builds[2].

Thanks.

[1]: https://youtu.be/6q8ZKEufvUI
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfa7b19965ff
Flags: needinfo?(shorlander)
Comment on attachment 8788726 [details] [diff] [review]
bug-1256074.patch

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

Looks good to me. Thanks!
Attachment #8788726 - Flags: ui-review?(shorlander) → ui-review+
Flags: needinfo?(shorlander)
Comment on attachment 8788726 [details] [diff] [review]
bug-1256074.patch

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

The patch looks good to me, but we really want some kind of automated test.
The test should live in toolkit/components/places/tests/unifiedcomplete, and I actually think you could just modify test_visiturl.js
Indeed, I suspect that test may already be failing with this change, cause it's expecting one result and getting 2 now.

You should run the tests in that subfolder with ./mach test toolkit/components/places/tests/unifiedcomplete to be sure
we clearly need something that fails without the patch and passes with it, if modifying existing tests is enough, that be it, otherwise you'll have to add a further subtest.

Finally, I'm actually thinking some things could not really want a search option, for example strings containing a "/" or valid ips are unlikely to be searched. I guess in addition to the url check we may want a regexp like !/(.*\..*){3,}|[\[\/]/.test(_originalSearchString) (no more than 2 dots, no / and no [)
Attachment #8788726 - Flags: review?(mak77) → feedback+
It's useful to sometimes use the awesomebar as a calculator. In those cases I might type 23.5/3.324 or (567*987)/6 expecting the search engine to get back with a result. Can't we be more lenient with this kind of input?
(In reply to Panos Astithas [:past] from comment #28)
> It's useful to sometimes use the awesomebar as a calculator. In those cases
> I might type 23.5/3.324 or (567*987)/6 expecting the search engine to get
> back with a result. Can't we be more lenient with this kind of input?

those are not valid urls, indeed if you type them in the awesomebar you get a search entry, not a visit entry.
The option here is added only for strings that URI Fixup *thinks* are urls (so we show a visit entry for them).
Excellent, carry on then!
Marco, thanks for the comments. Pushed a new try for fixing the tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f302c052744b06b90d68cb5ca124d06b4d905a86
Comment on attachment 8798290 [details]
Bug 1256074 - Always present a 'search' option when the location bar search is "fixed" to an URL.

https://reviewboard.mozilla.org/r/83810/#review82834

It looks good! I'd say we are almost there, just a couple details.

::: toolkit/components/places/UnifiedComplete.js:1007
(Diff revision 1)
> +        try {
> +          new URL(this._originalSearchString);
> +        } catch (ex) {
> +          // Show search option if keyword.enabled is true
> +          // and the string doesn't contain more than 2 dots, [, or /.
> +          if (Prefs.keywordEnabled && !/(.*\..*){3,}|[\[\/]/.test(this._originalSearchString)) {

please move the regexp to a const at the top like we did with REGEXP_SPACES or REGEXP_USER_CONTEXT_ID. So we don't have to rebuild it every time.

::: toolkit/components/places/tests/unifiedcomplete/test_searchSuggestions.js:482
(Diff revision 1)
>    yield check_autocomplete({
>      search: "localhost",
>      searchParam: "enable-actions",
>      matches: [
>        makeVisitMatch("localhost", "http://localhost/", { heuristic: true }),
> +      makeSearchMatch("localhost", { engineName: ENGINE_NAME, heuristic: true })

ok, this is a problem, this is not exactly an heuristic result based on how we consider it.

What you should do is: this._addingHeuristicFirstMatch = false;
yield this._matchCurrentSearchEngine();
this._addingHeuristicFirstMatch = true;

and update the tests showing it as heuristic

::: toolkit/components/places/tests/unifiedcomplete/test_visiturl.js:60
(Diff revision 1)
>    yield check_autocomplete({
>      search: "about:config",
>      searchParam: "enable-actions",
> -    matches: [ { uri: makeActionURI("visiturl", {url: "about:config", input: "about:config"}), title: "about:config", style: [ "action", "visiturl", "heuristic" ] } ]
> +    matches: [
> +      { uri: makeActionURI("visiturl", {url: "about:config", input: "about:config"}), title: "about:config", style: [ "action", "visiturl", "heuristic" ] },
> +      { uri: makeActionURI("searchengine", {engineName: "MozSearch", input: "about:config", searchQuery: "about:config"}), title: "MozSearch", style: ["action", "searchengine", "heuristic"] }

HM, right, about:something...
I think for now we may want to also add ":" to the list of blacklisted chars, for the protocol:something case

And again, remember to update tests accordingly.
Attachment #8798290 - Flags: review?(mak77)
Hi Marco,

I updated the patch and tests(all passed in local).
Please help review it, thanks.

The try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1238e5538337adcb9324f57ce62b23b0a7641289
Attachment #8788726 - Attachment is obsolete: true
Comment on attachment 8798290 [details]
Bug 1256074 - Always present a 'search' option when the location bar search is "fixed" to an URL.

https://reviewboard.mozilla.org/r/83810/#review85084

I'm sorry for that, but you likely need to unbitrot the tests, cause I added some new testcases to them in a different bug.
I think this will be the last roundtrip, then the patch should be landable.

::: toolkit/components/places/UnifiedComplete.js:85
(Diff revision 2)
>  
>  // Regex used to match one or more whitespace.
>  const REGEXP_SPACES = /\s+/;
>  
> +// Regex used to match URL.
> +const REGEXP_URL = /(.*\..*){3,}|[\[\/\:]/;

this doesn't exactly match a url, rather something that may look like an url-
I'd name it REGEXP_LOOKSLIKEURL. But actually...
...I'd love if we could reuse looksLikeUrl function for this, so maybe we could modify it like this:

function looksLikeUrl(str, ignoreAlphanumericHosts = false) {
  // Single word not including special chars.
  return !REGEXP_SPACES.test(str) &&
         ["/", "@", ":", "["].some(c => str.includes(c)) &&
         (ignoreAlphanumericHosts ? /(.*\..*){3,}/.test(str) : str.includes("."));
}
(note I replaced . with [ in the includes check)

and then our new code could use !looksLikeUrl(this._originalSearchString, true);

::: toolkit/components/places/UnifiedComplete.js:1004
(Diff revision 2)
>        // but isn't in the whitelist.
>        let matched = yield this._matchUnknownUrl();
>        if (matched) {
> +        // Since we can't tell if this is a real URL and
> +        // whether the user wants to visit or search for it,
> +        // we always provide an alternative searchengine match.

s/always/may/
Attachment #8798290 - Flags: review?(mak77)
or the argument may be named "ignoreSingleDottedHosts", I couldn't come up with a better name, but it's not critical.
I think `looksLikeUrl` is OK.
Hi Marco,

I've updated the patch for your comments. Please review it again.

The try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=162b0a732c1f4c32a38710d682252a9c495b233f

Thanks.
Comment on attachment 8798290 [details]
Bug 1256074 - Always present a 'search' option when the location bar search is "fixed" to an URL.

https://reviewboard.mozilla.org/r/83810/#review85616

it looks good! thank you
Attachment #8798290 - Flags: review?(mak77) → review+
jsut one small note, please update the commit message to the current bug title since it's clearer
I've updated the commit message.

Wait for that the try[1] is good, and then let's do `checkin-needed`.

Thanks for review, Marco.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=162b0a732c1f4c32a38710d682252a9c495b233f
The try is good. Let's checkin-needed.
Keywords: checkin-needed
Please mark the pending issues in MozReview as resolved so that this can autoland.
Keywords: checkin-needed
Hi Ryan,

I've resolved the issues, but I don't have LV3. Still need checkin-needed here.

Thanks.
Keywords: checkin-needed
in theory you should be able to autoland if you got review from someone having LV3, like me.
That said, I just pressed the button for you.
Keywords: checkin-needed
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/bb8d2847640f
Always present a 'search' option when the location bar search is "fixed" to an URL. r=mak
Thanks, Mark.

I don't why my "Land Commits" button is still disabled after you r+ the patch.
https://hg.mozilla.org/mozilla-central/rev/bb8d2847640f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I have reproduced this bug on firefox nightly according to(2016-03-12)

Fixing bug is verified on Latest Nightly -- Build ID:( 20161022030204 ),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0

Tested OS-- Windows10 32bit
QA Whiteboard: [bugday-20161019]
QA Whiteboard: [bugday-20161019] → [testday-20161021]
No longer depends on: 1326235
I have reproduced this bug with Nightly 48.0a1 on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Developer Edition (Aurora)!

Build   ID  :  20170119004006
User Agent  :  Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0

[testday-20170120]
Thanks!
Status: RESOLVED → VERIFIED
[bugday-20170201]
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: