We currently restyle URL heuristic results when a one-off is selected/search
mode is previewed, but afaict we should restyle all types. In the case of this
bug, the heuristic is a search result. I ended up rewriting most of
browser_oneOffs_heuristicRestyle.js to check various result types (URL, search,
and keyword) and both Alt-arrowing and non-Alt-arrowing down to the one-offs in
case that makes a difference.
This fixes part of bug 1678765, the part where we do the wrong thing when a
restyled keyword result is picked. It doesn't fix the visual part where the
action text is flush against the title text.
This also fixes a small error in `UrlbarView._on_SelectedOneOffButtonChanged`
where we were using `result.payload.title` when restyling heuristics, but
`result.title` is the correct property.
Depends on D97376
Differential Revision: https://phabricator.services.mozilla.com/D97843
We were restoring search mode after tab switch even if the destination tab had a valid pageproxystate. This was causing issues in the case where the user switched away from a tab that was still loading and had not yet called setURI. If the user switched back to that tab after it was done loading, we would re-enter search mode, using the page's URL as the search string.
Making this change also requires storing pageproxystate differently. If we only made the change to setURI, we would sometimes not restore search mode when we should. If the user entered search mode with an empty string, userTypedValue would never be updated and it would still be the page URL. After the user switched from, then to, that tab, we'd call setURI. setURI would restore userTypedValue, which would be the page URL. Since that page now had a valid pageproxystate, we would not restore search mode. Now we update userTypedValue when we enter search mode, so we restore search mode properly even when there is no search string.
Differential Revision: https://phabricator.services.mozilla.com/D97798
* Add prefs for each local search shortcut
* Remove the `update2.localOneOffs` pref since it's not necessary with the three
new prefs
* Modify preferences UI by adding a new row in the engines tree for each local
shortcut
* Add `UrlbarUtils.LOCAL_SEARCH_MODES` so we have a single place where local
search modes and their properties are defined
* Add a new test file for the preferences UI
* Modify browser_oneOffs.js to test the three new preferences
Differential Revision: https://phabricator.services.mozilla.com/D97376
The previous approach was autofilling partial subdomains from search engines.
Having a search engine pointing to "developer.mozilla.org, when "moz" was typed
we ended up autofilling "moz[illa.org]" but pointing to "developer.mozilla.org".
Unfortunately that made impossible to actually go to "mozilla.org" by typing it.
The new approach instead allows tab-to-search results to appear even if there's
no autofill, because the provider checks autonomously the autofill threshold.
There's a downside though, with the old approach we could return more partial
matches, and the Muxer would pick one, depending on the autofilled host. For
example we could return "rover.ebay.com" and match it to "eb[ay.it]".
With the new approach we don't have an autofilled host, and fetching it would
be expensive, we can only compare the search string with the engine's host.
For this reason the patch also tries to partially match against the searchForm
host, that often points to a page the user visits more often.
Differential Revision: https://phabricator.services.mozilla.com/D96942
This method only is async in order to allow callers to wait for a process switch
triggered by the call to `loadURI` to be finished before resolving. With
DocumentChannel, we should never trigger a process switch eagerly like this
again, so we don't need any of the async behaviour here anymore.
This part is largely mechanical changes to tests, removing the `await` calls on
`loadURI`, and a follow-up part will remove the actual async logic from
`BrowserTestUtils.loadURI`.
Differential Revision: https://phabricator.services.mozilla.com/D94641
This default-true pref has no effect when true and overrides update2 when false. This will allow us to set it to true for our Control branch so they will just fall back to the default-true update2 pref. We can set it to false for the Treatment branch, who will not get update2 features regardless of the value of urlbar.update2.
We want to uplift this patch to 83. That way, we can set browser.urlbar.experiment.update2 in Release 82, but it will have no effect either way until 83 merges to Release.
Differential Revision: https://phabricator.services.mozilla.com/D95806
This method only is async in order to allow callers to wait for a process switch
triggered by the call to `loadURI` to be finished before resolving. With
DocumentChannel, we should never trigger a process switch eagerly like this
again, so we don't need any of the async behaviour here anymore.
This part is largely mechanical changes to tests, removing the `await` calls on
`loadURI`, and a follow-up part will remove the actual async logic from
`BrowserTestUtils.loadURI`.
Differential Revision: https://phabricator.services.mozilla.com/D94641
Allow autofill to fill partial search providers domains so that, for example,
en.wikipedia.org can be filled by typing "wiki".
Differential Revision: https://phabricator.services.mozilla.com/D95277
This converts FX_URLBAR_SELECTED_RESULT_TYPE_2, FX_URLBAR_SELECTED_RESULT_INDEX
and the correlation between them FX_URLBAR_SELECTED_RESULT_INDEX_BY_TYPE_2 to
keyed scalars, that are nicer to use and analyze.
They are converted to one keyed scalar per result type, tracking the number
of times that type was picked per urlbar index.
The sums (count per type or per index) can still be derived from this structure.
Differential Revision: https://phabricator.services.mozilla.com/D94498
The heavily nested structure of UrlbarInput.setValueFromResult made it difficult to handle a result that was both a keywordoffer and autofilled. I took this opportunity to refactor setValueFromResult to be flatter. I think it's a bit easier to read now.
Differential Revision: https://phabricator.services.mozilla.com/D94903
This patch replaces the heuristic's icon, title, and action text when cycling through the one-offs. One problem is that the underlying result is still of RESULT_TYPE.URL, so picking it would visit its underlying URL. One way to get around this would be to create a new search UrlbarResult and swap it with the heuristic result, or change all the properties of the UrlbarResult so it becomes a search result. These approaches were discussed in review and rejected because they break the design of the QuantumBar. Having the URL result navigate to a SERP when picked would also require a lot of flags and conditionals throughout our navigation logic, which isn't ideal. I settled on promoting search mode when one of these restyled results is picked. I admit that this is a bit of a strange interaction, given that something that looks like a search heuristic result behaves like a keywordoffer, but I think its preferable to us navigating to a URL the user cannot see in the UI. As discussed in review, this result can only be picked by clicking on it. The chance of a user clicking the heuristic result while arrowing through the one-offs is exceedingly slim, so it seems like a decent compromise between UX and engineering effort. It also means selecting the restyled result with click and Enter does the same thing, so it's not completely out of nowhere.
I checked a11y with this patch applied and things are mostly unchanged from a screen reader perspective. Currently, typing the string "moz", alt+arrowing to the first one-off, then selecting it will create announcements like: "m", "o", "z", "Selection replaced. Google (@google), button", "Selection replaced. moz, Search with Google, edit text". The same annoucements are made with this patch applied. Pressing down arrow (not alt) will announce the second result. Arrowing back up to the heuristic will announce the original autofill result, because the original autofill result has been swapped back in. I'll ping Jamie in the bug once this patch is mature.
Differential Revision: https://phabricator.services.mozilla.com/D94338
This patch fixes two issues:
1. When a history result was autofilled, cycling through the engine one-offs would not change its icon (as expected) and cycling through the local one-offs would change its icon, but only to the history icon. This was because UrlbarView._iconForSearchResult was only checking for the result source and wasn't checking that the result was of search type as well. For consistency, we no longer change the icon when local one-offs are selected (we will change it once we figure out a way to change it for engine one-offs as well). I also renamed this method to _iconForResult to prep for Part 2 of this patch.
2. Typing a string, alt+arrowing through the one-offs, then pressing Enter flickers the heuristic result. This is because the one-off button is deselected, so we restore the original heuristic result. However, the search mode query also fires at the same time and quickly replaces the heuristic result. I added a check to not restore the original heuristic result in this case, eliminating the flicker.
This patch also refactors UrlbarView._on_SelectedOneOffButtonChanged. Previously, the main loop bailed early if a local one-off was selected, thus treating local one-offs as if they were very different from engine one-offs. This new flow handles both local and engine one-offs over the course of the same loop. I think it's a bit easier to read and understand.
Differential Revision: https://phabricator.services.mozilla.com/D94637
This test times out on macOS 10.14 opt and debug. Logs show that the test
finishes properly but it exceeds the timeout threshold, so it appears that it
just takes too long. Increasing the timeout threshold fixes the problem on opt,
but it reveals another problem on debug where the watchdog process kills the
main process because its shutdown lasts too long. This happens during step 3 of
test-verify, which is "Run each test 10 times in one browser, in chaos mode."
Around the time that the crash happens, logs show that there are "2047 DOMWINDOW
created and 2037 destroyed log strings." The test opens, closes, and reopens
many windows -- with chaos mode on top of that -- so it's possible that debug
window instrumentation or just the debug nature of the build itself, plus slow
machines, causes all the window opening and closing to perform very poorly and
take a long time.
This patch increases the timeout, which fixes opt, and disables the test
entirely on debug -- and to reiterate, it's Mac only. Maybe the test could open
and close tabs instead of windows in order to test session store, but I'm not
sure it's worth rewriting.
Here's a successful try run with an earlier version of this patch that did not
disable the test on debug, but it shows that opt is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ab33a9225b9edf0cb8a95d556f9b76c7fc69470
And here's a run without the patch that shows opt is always orange:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e02bfc26ecb67f503cb24a180536e8c220226d5
Differential Revision: https://phabricator.services.mozilla.com/D94069
There are a couple of issues that still need to be resolved, but I think this is ready for review:
- This patch does not use the icon in the UX spec. I'm waiting on that icon and just using the engine icon in the meantime.
- We also don't have a spec for how the result should wrap at narrow window widths, so I put some placeholder behaviour in.
- It appears dynamic results are incapable of overriding the CSS for normal Urlbar classes unless !important is used, regardless of specificity. Thus a few rules in dynamic-tabToSearch.css use !important, which isn't ideal. It appears this is because dynamic result stylesheets are [loaded](https://searchfox.org/mozilla-central/rev/919607a3610222099fbfb0113c98b77888ebcbfb/browser/components/urlbar/UrlbarView.jsm#1783,1785,1802) as `AGENT_SHEET`s. Changing that value to AUTHOR_SHEET allows the dynamic result CSS to override the normal Urlbar classes without !important. This strikes me as a tough issue. dynamic-tabToSearch.css is an agent sheet since it's provided by the browser. I do wonder if its defensible to load it as an author sheet since it will usually be loaded by an extension/external developer (this might be a stretch :). I'm not well-versed enough in the technicalities here to really make a call, so I'm eager to hear if @dao or @adw have any thoughts on how to resolve this issue.
Differential Revision: https://phabricator.services.mozilla.com/D93015
There are a couple of issues that still need to be resolved, but I think this is ready for review:
- This patch does not use the icon in the UX spec. I'm waiting on that icon and just using the engine icon in the meantime.
- We also don't have a spec for how the result should wrap at narrow window widths, so I put some placeholder behaviour in.
- It appears dynamic results are incapable of overriding the CSS for normal Urlbar classes unless !important is used, regardless of specificity. Thus a few rules in dynamic-tabToSearch.css use !important, which isn't ideal. It appears this is because dynamic result stylesheets are [loaded](https://searchfox.org/mozilla-central/rev/919607a3610222099fbfb0113c98b77888ebcbfb/browser/components/urlbar/UrlbarView.jsm#1783,1785,1802) as `AGENT_SHEET`s. Changing that value to AUTHOR_SHEET allows the dynamic result CSS to override the normal Urlbar classes without !important. This strikes me as a tough issue. dynamic-tabToSearch.css is an agent sheet since it's provided by the browser. I do wonder if its defensible to load it as an author sheet since it will usually be loaded by an extension/external developer (this might be a stretch :). I'm not well-versed enough in the technicalities here to really make a call, so I'm eager to hear if @dao or @adw have any thoughts on how to resolve this issue.
Differential Revision: https://phabricator.services.mozilla.com/D93015
This is a pretty simple implementation of the approach discussed in the bug. When a tab-to-search result is shown, we announce it to screen readers. We keep track of which engines are announced to be sure we don't keep announcing the same engine as the user types.
This approach is maybe slightly crude, but I didn't find a working elegant approach. One approach I considered is checking for tab-to-search results in the loop in UrlbarView._updateResults. After the loop is finished, if we found a tab-to-search result, we go back and change the aria-label on the action text of the prior result to something like "Visit, or press Tab to search with { $engine }. This didn't work because we don't read action text for the heuristic result as the user types. This is presumbably to avoid disrupting the user as they type. This is what my patch does regardless, however... Since Jamie was skeptical about whether we should announce this at all, I put this behaviour behind a default-true pref, browser.urlbar.tabToSearch.accessibility.announceResults.
Differential Revision: https://phabricator.services.mozilla.com/D93451
* Always update the `_searchModesByBrowser` map when entering/exiting search
mode, not only for non-selected browsers, so that search mode can be saved and
restored properly per tab.
* Rename `setSearchMode` to `_updateSearchModeUI` and make it only update the UI.
* Add a new `setSearchMode` method that takes a browser and updates the map.
* Add `getSearchMode` so that SessionStore can get the search mode for a given
browser.
* Add a `searchMode` getter and setter for convenience. They call
`getSearchMode` and `setSearchMode` with the selected browser.
Differential Revision: https://phabricator.services.mozilla.com/D91910
This was more difficult to solve than I expected. The main issue is that the
`[open]` attribute on `#urlbar` wasn't accurate when the view was "open" but
there were no results or one-offs, so it was in fact closed. This broke a few
style rules.
This bug can also be reached when the user has no engines and clears the search
string while in search mode. This includes pressing Accel+K when typing a search
string while not in search mode.
The relationship between the UrlbarView and the one-offs is complex and depends
on a lot of listeners and async calls made in synchronous contexts. Furthermore,
most of the UrlbarView open/close code is synchronous, but checking the number
of engines (to determine if the one-offs will open) is an async operation. These
factors make it difficult for the view to discern any state about the one-offs
and plan accordingly.
First I tried adding a [oneoff] attribute to .urlbarView, set asynchronously when
the one-offs are built. Then I changed CSS rules to check
```
.urlbarView:not([noresults]),
.urlbarView[oneoffs]
```
instead of just `#urlbar[open]`. This approach would've required changing
a bunch of CSS from the simple `#urlbar[open]` to the more complicated CSS
above, which was not good for code readability. Also it would keep `[open]` in
a weird useless state where it couldn't really be trusted. This would've caused
other styling problems.
I settled on adding a `.then` call around the affected UrlbarView opening. The
view opens only if we are sure that we will have either results or one-offs,
so we can once again trust the `[open]` parameter. This approach has its
drawbacks. Mainly, it's a more JavaScript-heavy solution so it's possibly
slow. Thankfully, it's something that can be easily cached.
Differential Revision: https://phabricator.services.mozilla.com/D92526
In many cases with wpt, most of the tests in the file pass, but it is rather time consuming to annotate
.ini files case by case.
Differential Revision: https://phabricator.services.mozilla.com/D91977
This adds tab-to-search telemetry, both for the new tabtosearch search mode entry point and for tabtosearch results in our usual Urlbar result-selection scalars. I also added a subtest in browser_urlbar_event_telemetry, but realized as I was writing it that it was not useful. We don't consider entering search more as the end of an engagement, so tab-to-search results will not appear in event telemetry. We already considered this in bug 1654680 and resolved it by adding detailed urlbar.searchmode.* scalars, so I don't consider it a blocker. I left the new subtest in since it was mostly done anyways and it can't hurt.
Differential Revision: https://phabricator.services.mozilla.com/D91469
This patch adds tab-to-search results, partially reusing the existing code in `UrlbarProviderAutofill._matchSearchEngineDomain`. Tests are included. Telemetry is included in a child revision.
This patch doesn't add the "Search <engine name> directly from the address bar." action text. Our current action text code l10n is in a .properties file and I thought adding branching translation logic in UrlbarView would be a mess. I think Marco might be converting that .properties file to Fluent in bug 1658629. If he is, I'll rebase my patch on his and add the new action text it in this patch. If he isn't, I'll file a follow-up for converting that .properties file and adding the tab-to-search action text, which we can address before preffing `update2.tabToComplete` on. Either way, I think this is ready for a first-round review.
This patch is based on D91076 and D91077. It doesn't have any functional dependency, but there are some conflicts, especially in the added telemetry.
Differential Revision: https://phabricator.services.mozilla.com/D91468
This patch calls setSearchMode directly from search(). This sets up a solution for the problem in the bug and also fixes the issue where a call to search() with a restriction token would flicker the token before it was replaced with the search mode indicator. I added new tabmenu and bookmarkmenu entry points to take advantage of this new functionality.
This also fixes the issues with handoff. Besides the problem of search() recording typed for handoff, `handoff` wasn't even registered as a Telemetry probe! That was my mistake. I added a test for handoff telemetry. It was only practical to test it in PBM since it uses a different implementation than about:home that's easier to test. I wrote a lengthy comment above the subtest about why I think this is okay.
Differential Revision: https://phabricator.services.mozilla.com/D91076
- always show search suggestions first in search mode
- use restyleSearches only in search mode, to get cleaner history results and dedupe against search history
- filter redirects differently depending on restyleSearches
Differential Revision: https://phabricator.services.mozilla.com/D90719
First, `UrlbarView.clear` needs to null the selected element. Second, setting
the selected element to null needs to also null
`UrlbarInput._resultForCurrentValue`. `UrlbarView._selectElement` does call
`input.setValueFromResult`, which sets `_resultForCurrentValue` -- but not if
the `updateInput` arg is false. So I added a new
`UrlbarInput.setResultForCurrentValue` method so `_selectElement` can set it. At
first I tried unconditionally calling `input.setValueFromResult`, but a bunch of
tests failed. I looked into it, but it's complicated, so I gave up and added
this new method.
This also makes some other changes that I'll comment on inline.
Differential Revision: https://phabricator.services.mozilla.com/D90360
This test requires search mode, but it doesn't enable update2, so it fails on
non-Nightly. It only needs to enable update2 like other search-mode tests do.
Differential Revision: https://phabricator.services.mozilla.com/D89850
The "Search in a Private Window" result is included only in Nightly builds, so
this test needs to either enable the corresponding pref, or disable the pref and
remove that result from the expected results. This patch does the latter.
Differential Revision: https://phabricator.services.mozilla.com/D89849
The _shortName variable is basically redundant, and we can transfer the remaining 'identifier' handling across to the telemetry Id as they were virtually the same.
Differential Revision: https://phabricator.services.mozilla.com/D88623
This patch adds support for key modifiers on the refreshed one-offs. They work the same as the old key modifiers, with one exception: Shift+Click and Shift+Enter both execute a search immediately in the current tab, replicating the behaviour of the old one-offs. For empty searches, Shift+Click and Shift+Enter just enter search mode. To support these Shift modifiers, we dropped support for opening a one-off search in a new window. Users can still search in a new window with a non-default engine by typing a search string, selecting a one-off with the keyboard, then Shift+Clicking the heuristic result or the Go button.
Other key modifiers worth pointing out include Accel+Click to search in a new background tab and Alt+Enter to search in a new foreground tab. If these modifiers are used on the local one offs or with an empty query, we open search mode in the new tab.
Differential Revision: https://phabricator.services.mozilla.com/D89504
reInit is unsafe as it completely removes the existing data before reloading. If something interrupts the process that can cause dataloss.
_maybeReloadEngines is safer as it does changes progressively, it also now handles removing engines, which it didn't before.
Depends on D88023
Differential Revision: https://phabricator.services.mozilla.com/D88272
Summary of major changes:
* Bookmarks, history, and tabs restriction chars now enter search mode. I added
a method to UrlbarProviderHeuristicFallback to return a result with a keyword
when one of these is used.
* This fixes other bugs like recognizing aliases that are entered at the
beginning of non-empty search strings, and not quasi-re-entering search mode
when search mode is already entered and you type another alias.
* The heuristic now determines whether we enter search mode, similar to how it
also determines whether we autofill. When the heuristic has a keyword but no
keyword offer, and the keyword is one of the recognized search mode keywords,
then we enter search mode, cancel the current query, and start a new query
with the remainder of the search string after the keyword.
* I slightly changed how we detect an alias, but only when update2 is
enabled. Now, an alias must be followed by a space; otherwise, the alias is
not recognized and instead just remains part of the seach string. Because if
we don't do that, then you end up in a strange situation after typing an alias
but before pressing space: The heuristic says "Search with <engine with the
alias>", but we haven't entered search mode yet because you haven't typed a
space yet. This is true for both @aliaes and non-@aliases.
* A consequence of the previous point is that we can still autofill @aliases
with a trailing space, which IMO is important. Then, once the user types any
char (space or not), we immediately enter search mode with the query being
whatever char they typed. This is less important after bug 1658605 landed, but
it's still good to have.
* Previously, `UrlbarView.onQueryResults` called UrlbarInput in order to
autofill after the first result is received. This is circuitous becaue the
input already has an `onFirstResult` method, which I now use to enter search
mode when appropriate. So I moved the autofill call from UrlbarView to
`UrlbarInput.onFirstResult`.
* As I mentioned, I improved some test framework and simplified some related
product (non-test) code. For example:
* I removed `UrlbarUtils.KEYWORD_OFFER.NONE` in favor of just leaving
`keywordOffer` as `undefined`.
* `tailOffsetIndex` can now be `undefined` if it's not relevant.
* I removed empty-string `icon` properties from payloads in favor of
`undefined`.
* In tests, I ignore `undefined` but present properties in payloads so they
don't count when comparing payloads with `deepEqual`.
* We weren't previously comparing `result.source` and `result.type` in
xpcshell tests, and that's important IMO, so I added checks for those and
updated tests.
* `isSearchHistory` is redundant, so I removed it. For form history, we
should be checking `result.source == HISTORY` and `result.type == SEARCH`.
* A bunch of tests needed to be updated for this new behavior.
Differential Revision: https://phabricator.services.mozilla.com/D87944
Summary of changes:
1. Adds an `entry` property to the searchMode object to tag how search mode was entered.
2. Introduces `urlbar.searchmode.*` scalars. These scalars are suffixed with an entry point into search mode, for example `urlbar.searchmode.oneoff`, or `urlbar.searchmode.topsites_urlbar`. Those entry points tell us how search mode is entered most often. The keys for these scalars are strings describing what kind of search mode was entered. In most cases, this will be the name of a search engine, like "Google", or "DuckDuckGo". It may also be one of "history", "bookmarks", or "tabs". We only collect the names of engines that are bundled with Firefox. If the user enters search mode with an engine they installed themselves, we record "other" as the key.
3. Adds a urlbar-searchmode SAP to SEARCH_COUNTS.
4. Adds a browser.engagement.navigation.urlbar_searchmode probe.
5. Adds a urlbar_searchmode SAP to the navigation.search event.
Differential Revision: https://phabricator.services.mozilla.com/D87510
reInit is unsafe as it completely removes the existing data before reloading. If something interrupts the process that can cause dataloss.
_maybeReloadEngines is safer as it does changes progressively, it also now handles removing engines, which it didn't before.
Depends on D88023
Differential Revision: https://phabricator.services.mozilla.com/D88272
This rewrites the muxer. I'll explain why.
The obvious way to fix this bug is to modify UrlbarProviderSearchSuggestions so
it adds `maxHistoricalSearchSuggestions` form history results first, followed by
as many remote suggestions as there are, followed by any remaining form history
results. And in fact that's what this patch does. But the muxer isn't capable of
handling that very well, with regard to deuping SERPs and form history.
The muxer does a first pass through all results, and it builds a set of form
history suggestions. Then, as it's adding URL results in the second pass, it
excludes SERPs whose search terms are in the set. The problem is that the set
can include search terms from form history results that do not end up in the
final list of results. And that's because UrlbarProviderSearchSuggestions now
returns many form history results -- as many as `context.maxResults + 1` so that
there are enough of them to fill the view when appropriate.
This is a problem with the muxer in general. It collects a bunch of state from
all results in its first pass, even though not all of those results may end up
in the final list. Worst case, we may end up excluding results we should not
exclude. The fundamental problem is that the muxer doesn't know which results
will end up being included until it starts including results.
The key thing about this rewrite is that the muxer builds up state as it goes
along filling its buckets. If a result is excluded, then it doesn't contribute
to the state used to determine whether subsequent results should be included.
There are a couple of exceptions though where it still does build state using
all results. (1) It still determines the heuristic this way, but that's OK since
there's only one heuristic. (2) It still builds `strippedUrlToTopPrefixAndTitle`
this way. I couldn't think of a nice way around that, because AFAICT there's no
guarantee that UnifiedComplete will put the higher-ranking URL result before the
lower one. If the lower one comes first, we'd end up including it too since
`strippedUrlToTopPrefixAndTitle` would not contain the higher-ranking one at
that point.
There's one drawback of building up the state in this new way. It's the flip
side of solving the problem above. If a result depends in some way on a
subsequent result, then the state at that first result won't be accurate and the
muxer will make a bad decision about that result. There's an example of this in
test_search_suggestions.js, near the bottom of the `formHistory` task. In that
test, `matchBuckets` is defined so that general results (e.g., history) appear
before search suggestions. That means the SERP in history can't be deduped in
favor of the form history result that appears later, so both results appear. I
think that's better than the alternative of possibly deduping too aggressively.
One important thing to note is that this patch isn't restricted to search
mode. It will always include more form history results to fill out the final
list if some other higher-ranked result group doesn't fill it out
sooner. Currently `matchBuckets` is `heuristic,1,extension,5,suggestion,4,general,Infinity,suggestion,Infinity,general,Infinity.`
Since `general,Infinity` comes before `suggestion,Infinity`, this means that if
there aren't enough general results to fill out the list, then suggestions will
fill it out as much as possible. Within suggestions, remote suggestions will
fill it out first and then form history, since that's the order that
UrlbarProviderSearchSuggestions adds them in after the initial
`maxHistoricalSearchSuggestions` form history results. I think that's what we
want regardless of search mode.
Finally, this patch also breaks up `sort` into more, smaller methods. The patch
started out as a much larger version that also redesigned `matchBuckets`. That's
the main reason I split it up, but it's nice by itself I think. (I'd like to
come back to that `matchBuckets` redesign, which could now build on top of
this. The original patch is in D87830.)
Differential Revision: https://phabricator.services.mozilla.com/D87838
The last line from the test in the failure log is "INFO - Leaving test bound
tabSwitch", which is the final task in the test. I'm pretty sure the test is just
doing too much rather than timing out waiting for something to happen. So let's
split it up.
Depends on D87172
Differential Revision: https://phabricator.services.mozilla.com/D87384
* Add a check for `queryContext.searchMode` in
`UrlbarProviderHeuristicFallback._matchUnknownUrl`. We should not match a URL
when search mode is active, in addition to the existing check for a
restricting source.
* Move the aforementioned checks to the top of the method so that we avoid when
possible the more expensive string escaping and parsing currently at the top
of that method.
* Add a test task.
* Beef up the existing test tasks.
Differential Revision: https://phabricator.services.mozilla.com/D87189
We need to exit search mode when a page loads in the current tab. We may need to
exit search mode for page loads in other tabs too: If you're in search mode,
click a slow link, switch tabs, and the page loads in the meantime, then search
mode should be not be entered when you switch back. I don't think we handle that
case correctly right now, and this patch doesn't address that at all. That's
worth doing in another bug because I think the fix will be different.
At first I added an `onLocationChange` method to UrlbarInput that was called by
`XULBrowserWindow.onLocationChange` in browser.js [1], just like we have an
`onLocationChange` in UrlbarProviderSearchTips called by
`XULBrowserWindow.onLocationChange`. But we need to potentially exit search mode
any time `input.setURI` is called. `setURI` happens to be called by
`XULBrowserWindow.onLocationChange`, one of the several places that calls it
[2].
`setURI` is also called when switching tabs. Bug 1647899 already took care of
handling search mode for tab switches, but it would be nice to handle all this
in one place. `setURI` is also how `userTypedValue` is restored in the input,
and of course `userTypedValue` is something we need to restore when switching
tabs, just like search mode. For these reasons I moved per-tab search mode
restoration to `setURI` as part of this.
I'm also changing the name of the second parameter in `setURI`. I wasn't sure
whether it's true iff we're switching tabs, so I tracked down why that param was
added. It was added in bug 1478348, and comment 21 confirms it was added to tell
`setURI` and `XULBrowserWindow.onLocationChange` when they're being called due
to a tab switch. To make this clearer, I renamed the param and added some
javadoc for `XULBrowserWindow.onLocationChange`.
[1] https://searchfox.org/mozilla-central/rev/50cb0892948fb4291b9a6b1b30122100ec7d4ef2/browser/base/content/browser.js#5205
[2] https://searchfox.org/mozilla-central/search?q=symbol:%23setURI&redirect=false
Differential Revision: https://phabricator.services.mozilla.com/D87172
I can reproduce this reliably after refreshing my tree and applying the patch to
bug 1657918. It seems to be permanent in fact. The problem is that a previous
test (or the test itself, since this is a failure in verify mode) leaves the
mouse over the heuristic result, which causes it to be highlighted and show its
action text ("Search with Google"). The test is checking that the action text is
hidden.
We just need to synthesize a mousemove away from the view as the test starts. We
do something similar in a few tests already, although sometimes we use
`EventUtils.synthesizeNativeMouseMove` instead of what I use here. I tried that
function first but it didn't work here, so I went with the other one we use,
`EventUtils.synthesizeMouse` with `mousemove`.
Differential Revision: https://phabricator.services.mozilla.com/D87015
This excludes the heuristic for empty searches when in search mode. I haven't
heard back from Verdi yet about excluding it for all searches in search mode. We
can add that in a follow-up if necessary.
Since we're now excluding the heuristic but we want the view to remain open,
it's possible for the view to be empty while it's open. I had to make some
changes to allow that to happen. There are three cases I want to call out:
1. When the search string is empty, the view shows top sites. If you then enter
search mode and the resulting search doesn't return any results, we
previously closed the view. This patch keeps it open. An example of this
scenario is when you don't have any bookmarks and you click the bookmarks
one-off.
2. When the urlbar isn't focused, it's in search mode, the input is empty, and
the search didn't return any results, then focusing the urlbar didn't do
anything previously. This patch auto-opens the view.
3. When the input contains a single char and it's in search mode, deleting the
char previously closed the view if the empty search didn't return any
results. This patch keeps the view open.
When the view is empty, we also need to hide the one-offs' top border that
usually separates them from the results. Otherwise there are two separator lines
right next to each other, the one above the one-offs and the one at the bottom
edge of the input. I don't think there's any CSS selector that will let us
easily do this due to the DOM structure, so I added a new `noresults` attribute
on the view for this.
I had to call `context.searchString.trim()` to tell whether the search string is
empty. Since we do that in a bunch of places, I added
`context.trimmedSearchString`, and a lot of this patch is replacing those `trim`
calls.
Differential Revision: https://phabricator.services.mozilla.com/D86908
We just need to start a new query on backspace, even when the input is empty.
I noticed that unlike backspace, clicking the close button will always start a
new query except when the view is closed. That means the close button was
already doing the right thing. It also means that the close button will not open
the view and show top sites when the view is already closed; however, with this
patch, backspace *will* open the view and show top sites if the view is closed.
That seems right to me despite the inconsistency since inputs in the urlbar
(including backspace) always cause the view to open.
Other changes:
* Make the `click_close` test task more similar to the `backspace` task: Open
the view both with and without a string instead of only with a string
* Some calls in the test didn't have an `await` like they should (maybe the
source of reported intermittent failures?)
Differential Revision: https://phabricator.services.mozilla.com/D86686
This is the simplest possible way to fix this bug in the interest of landing
ASAP. It keeps a hardcoded list of engines that search the web.
Differential Revision: https://phabricator.services.mozilla.com/D86664
This patch changes the paramters to setSearchMode. The original patch to introduce search mode passed engineName to setSearchMode instead of the entire engine object. It was suggested in review that the nsISearchEngine be passed so we could run instanceof to distinguish it from a RESULT_TYPE. This patch reverses this and passes engineName instead through a destructured parameter.
In pickResult, we need to enter search mode synchronously based on the information in a result payload. Result payloads can't/shouldn't pass around complex objects like an nsISearchEngine, so we just pass engineName and the alias as strings. Since pickResult is synchronous, we can't use UrlbarSearchUtils to look up the engine based on the engineName. Besides, setSearchMode only uses engineName, so looking up the engine only to just use its name seems like a waste of resources.
This patch also disables autofill in search mode queries. Autofill was interfering with alias replacement. We were already half doing this (https://searchfox.org/mozilla-central/rev/26b13464c2beb26e0d864d561c30e817a85c348a/browser/components/urlbar/UrlbarController.jsm#391) but adding the searchMode check to UrlbarInput._maybeAutofillOnInput should resolve bug 1655473.
There's still one more bug I'm working through where the placeholder disappears after alias replacement. I though I'd get this out to start review regardless since we want to get the three patches discussed in Thursday's meeting out ASAP.
Differential Revision: https://phabricator.services.mozilla.com/D86389
This patch changes the paramters to setSearchMode. The original patch to introduce search mode passed engineName to setSearchMode instead of the entire engine object. It was suggested in review that the nsISearchEngine be passed so we could run instanceof to distinguish it from a RESULT_TYPE. This patch reverses this and passes engineName instead through a destructured parameter.
In pickResult, we need to enter search mode synchronously based on the information in a result payload. Result payloads can't/shouldn't pass around complex objects like an nsISearchEngine, so we just pass engineName and the alias as strings. Since pickResult is synchronous, we can't use UrlbarSearchUtils to look up the engine based on the engineName. Besides, setSearchMode only uses engineName, so looking up the engine only to just use its name seems like a waste of resources.
This patch also disables autofill in search mode queries. Autofill was interfering with alias replacement. We were already half doing this (https://searchfox.org/mozilla-central/rev/26b13464c2beb26e0d864d561c30e817a85c348a/browser/components/urlbar/UrlbarController.jsm#391) but adding the searchMode check to UrlbarInput._maybeAutofillOnInput should resolve bug 1655473.
There's still one more bug I'm working through where the placeholder disappears after alias replacement. I though I'd get this out to start review regardless since we want to get the three patches discussed in Thursday's meeting out ASAP.
Differential Revision: https://phabricator.services.mozilla.com/D86389
This is very simple, but it gets the job done. It keeps a map in `UrlbarInput`
from browsers to search mode objects. The map is updated in `setSearchMode`, and
it's used to set the search mode in `_afterTabSelectAndFocusChange`.
Differential Revision: https://phabricator.services.mozilla.com/D86296
This is a narrow fix to the bug, which happens because I removed this
conditional in bug 1654439: https://hg.mozilla.org/mozilla-central/rev/f561eb606ad3#l1.251
That conditional kept the selection out of the one-offs when the urlbar hid
them. This patch replaces that conditional with a version that can be overridden
by the urlbar subclass. It adds a `hasView` getter that is equivalent, but note
that `this._view` can't be used because the urlbar subclass sets `this.view` in
its constructor and never nulls it out, unlike the original implementation. So
instead the subclass `hasView` checks whether the one-offs are hidden. This is
effectively the same thing because in the old implementation the urlbar one-offs
were hidden at the same time `this._view` was nulled out.
Differential Revision: https://phabricator.services.mozilla.com/D86293
The original issue this patch addresses is what happens when exiting customize
mode: the reinsertion of the urlbar (customize mode wraps it in a container)
causes fluent to re-run as there's a DOM mutation, which causes the default
placeholder to be re-translated. This caused issues because we were using
.properties to translate the string that included the engine name, and
fluent for the "default" placeholder string.
This patch fixes the issue by always using fluent, and using a hardcoded
'unknown' placeholder to deal with the case where there is no known engine.
Differential Revision: https://phabricator.services.mozilla.com/D85769
This makes us put the copied string as-is into the clipboard, exclusively for the type:
text/plain;charset=utf-8
When pasting strings into Firefox, `\r` might still be converted to `\n`. So disable
part of the test in browser_removeUnsafeProtocolsFromURLBarPaste.js.
Differential Revision: https://phabricator.services.mozilla.com/D82224
A poorly thought fixup SQL condition was stripping "www." from the origin in any
case, but when the search string contained "www.".
This ended up stripping too often, for example when typing "w" and matching
against "www.mozilla.org".
Instead, the query should strip "www." from the origin only if it doesn't start
with the search string.
Differential Revision: https://phabricator.services.mozilla.com/D85006
The old query was picking a random bookmarked status out of the pages in an origin,
depending on the physical position of the origin rows in the table.
The new query uses window functions to partition the data by origin, so it can
query origins just once and sum www and non-www origin's frecency scores before
comparing to the autofill threshold.
Differential Revision: https://phabricator.services.mozilla.com/D84657
