Untrim the address bar value when the user starts manipulating it.
This doesn't untrim on focus, because that would break double-click to select
word, and drag-select. In the future we'll evaluate untrim on focus, but we'll
need additional platform support to replicate that functionality.
The behavior is currently controlled by the feature-gate preference
`browser.urlbar.untrimOnUserInteraction.featureGate`.
Original patch by Marc Seibert.
Differential Revision: https://phabricator.services.mozilla.com/D206135
When UrlbarView [handles the help command for a result](https://searchfox.org/mozilla-central/rev/863aef60f9fa1536884252ad90cad1d8ee9d8a41/browser/components/urlbar/UrlbarView.sys.mjs#3471-3476), first it determines the
result's help URL. If `result.payload.helpUrl` is defined it uses that, and
otherwise it falls back to generating a default help URL. Either way, then it
sets `menuitem.dataset.url` to the help URL and calls `input.pickResult()`.
Before bug 1883378, `pickResult()` would similarly first check
`result.payload.helpUrl` and then fall back to `menuitem.dataset.url` when
determining the help URL to load. Bug 1883378 changed this behavior so that it
only checks `result.payload.helpUrl`. That means we break any provider that
implements the help command but doesn't define a help URL on its results, like
Yelp and MDN.
One way to fix this would be to make sure providers that implement the help
command always define a help URL on their results. I don't like that idea
because it's nice for providers to be able to rely on a default help URL that's
defined elsewhere.
Another fix would be to always use `menuitem.dataset.url` in `pickResult()` and
don't check `result.payload.helpUrl` at all. That would work for result menu
items, but it wouldn't work for other elements like buttons that don't go
through the result-menu-command path. I don't know if we currently have any such
paths but I don't want to break something else.
So instead I think we should just go back to the status quo before bug 1883378:
First check `result.payload.helpUrl` if the command is help, and then fall back
to `menuitem.dataset.url`.
Differential Revision: https://phabricator.services.mozilla.com/D208197
Preparatory test to cover some not-well tested selection manipulations in the
Address Bar, as we're about to test different kind of trimming behaviors.
Differential Revision: https://phabricator.services.mozilla.com/D206134
The toolbar height includes the top border now. It's not what we want
anyways:
* In some cases its usage was completely redundant (we can use the
default static position).
* In other cases what we want is the urlbar-container height, which
doesn't include that border. I confirmed this looks good in compact
mode (which was the reason for introducing this in bug 1580248).
So simplify the code a tiny bit and fix the flickering that got the
original version reverted.
Differential Revision: https://phabricator.services.mozilla.com/D206596
Like a lot of the other Suggest tests, this Pocket one seems to have
intermittent timeouts simply due to the test taking too long, despite the fact
that the test isn't very long. I'm not sure why. I have more comments about it
in bug 1888594 and D206263.
While I was here, I also updated this test so it uses `add_tasks_with_rust()` so
it tests the Rust backend too. The comment that I removed refers to errors that
happened when I tried that before, but we must have fixed them because it works
fine now.
Differential Revision: https://phabricator.services.mozilla.com/D206442
There seem to be two problems:
1. The test loads a long URL in the browser test's default tab, which can cause
the test to hang for a few seconds at the end. I think this might be the
cause of intermittent non-verify-mode timeouts. Please see the bug for
details. To fix this, I modified the test to load the long URL in a new tab.
2. Even with the first problem fixed, the test takes forever in chaos mode in
verify mode on Mac. I don't know why, this test isn't really doing much. To
fix this, I added a `requestLongerTimeout()`. I don't know if it's as slow on
other platforms. My hunch is that it has something to do with all the
communication Firefox does with the mock RemoteSettingsServer during the
test. When you watch the test run, you can see the RemoteSettingsServer log
its output very slowly. Also, other Suggest tests seem to have this problem
too because over time we've added similar `requestLongerTimeout()` calls to
a bunch of them.
Differential Revision: https://phabricator.services.mozilla.com/D206263
If a provider onEngagement method causes the address bar panel to close when
a result is picked, we may record consecutive engagement and abandonment events
for the same search session.
Differential Revision: https://phabricator.services.mozilla.com/D206110
Fixed clipboard provider to stop running when address bar is in search mode.
This allows the Bookmarks, Tabs and History suggestions to be displayed
when in seach mode and a clipboard entry saved.
Differential Revision: https://phabricator.services.mozilla.com/D204288
The problem has to do with hidden exposures whose hypothetical rows can't
immediately be made visible either because they would overflow the filled-up
view or because a misplaced result was seen.
The relevant part of `#updateResults()` is [here](https://searchfox.org/mozilla-central/rev/6a2a2a52d7e544a2fd5678d04991a7e78b694f22/browser/components/urlbar/UrlbarView.sys.mjs#1289-1291). When `canBeVisible` is false,
we effectively forget about that hidden-exposure result, which is not right. For
normal results, we also have that `canBeVisible` logic (a few lines down from
the part I linked), but the difference is that we create a hidden row for them
and then unhide the row when the query finishes and stale rows are removed.
We're missing an analog to that logic for hidden-exposure results, so we're
undercounting them.
Fortunately it's not too hard to fix. I added a new kind of exposure for
`TelemetryEvent` to track called "tentative" exposures. Tentative exposures are
entirely analogous to this case where a row would be added but hidden, and then
unhidden when stale rows are removed.
While I was modifying the test, I rephrased the comments for existing tasks so
hopefully they're easier to understand.
Differential Revision: https://phabricator.services.mozilla.com/D205155
The heuristic result is always kept first when receiving new results, that means
any result could be upgraded to be a heuristic result, including the first Top
Site, as in the zero prefix case there's no heuritic.
Thus it's safer to generate new DOM contents when upgrading between heuristic and
non-heuristic.
Also switch-tab results with container info have complex structure and should
generate new DOM contents, unless they are reused by other switch-tab results.
Unfortunately these changes are mostly visual and not easily testable, as it would
be very time consuming.
In general, for the long term, reusing the DOM of results is fragile and causes
nodes, attributes or classes to be ported over to incompatible results. We do it
to avoid flicker, but we must investigate better ways to do it.
Differential Revision: https://phabricator.services.mozilla.com/D204483
This simplifies weather fetching so that multiple fetches are queued up instead
of hitting Merino at the same time. The newest fetch will cause earlier fetches
to stop (when possible). This is similar to how `SuggestBackendRust` handles
ingest.
This also improves `add_tasks_with_rust()` in a couple ways: (1) It adds
`skip_if_rust_enabled` and (2) handles task function args better by cloning them
instead of reusing them in the `_rustEnabled` and `_rustDisabled` tasks.
Currently we're relying on the normal xpcshell `skip_if` predicate to skip
`_rustEnabled` tasks when Rust is enabled while still running their
`_rustDisabled` counterparts when Rust is disabled. Except it doesn't work at
all because the `skip_if` predicate is evaluated at the time `add_task()` is
called, not when its task runs. That means the `_rustDisabled` versions of these
tasks are never run, since Rust is now enabled by default. So we're missing test
coverage in these cases. It's not a huge problem since hopefully we will not go
back to enabling the JS backend by default, but until we remove the JS backend,
we should maintain test coverage. (The `setupAndTeardown()` task in
`test_quicksuggest.js` failed when I fixed this, so that's once case where we
had a failing task and didn't know about it.)
Depends on D204138
Differential Revision: https://phabricator.services.mozilla.com/D204325
I found several problems:
Problem 1:
`getViewUpdate()` is allowed to be async and `UrlbarView` [awaits it](https://searchfox.org/mozilla-central/rev/a0ebb2a2c286c1d98d7ae93d043f65ed9970108b/browser/components/urlbar/UrlbarView.sys.mjs#2017) even though
the `Weather` implementation is not async. That means the summary text content
will be updated asyncly, so `browser_weather.js` needs to wait for it.
This is the cause of the recent frequent failures.
Problem 2:
This has to do with `add_tasks_with_rust()`. When it disables the Rust backend
and forces sync, the JS backend will sync `Weather` with remote settings. Since
keywords are present in remote settings at that point, `Weather` will then start
fetching. Each task that uses `add_tasks_with_rust()` needs to wait for this
fetch to finish. Otherwise `weather.suggestion` may be null. To fix this, I
added a way for tests to register a setup function that will run each time
`add_tasks_with_rust()` calls an original task.
This is the cause of infrequent failures that go back to at least when Rust was
enabled by default. I think this might be the cause of recent failures in
`test_weather.js` and `test_weather_keywords.js` too so I did the same thing
there.
Problem 3:
`browser_weather.js` sometimes times out in verify mode, so I requested a longer
timeout. It looks like the test finishes, it just takes a long time sometimes.
Problem 4:
`test_weather_keywords.js` sometimes times out in verify mode. This test takes
forever to run, especially now that it uses `add_tasks_with_rust()`. I added a
`skip-if` for verify mode. It's fine to skip verify mode for this test. Once we
remove the JS backend, we can probably remove the `skip-if`. If it still times
out, we could use `requesttimeoutfactor` or split it up.
Differential Revision: https://phabricator.services.mozilla.com/D204138
Adding the check in one more place that is failing on Linux.
This should resolve the failing of `browser/components/urlbar/tests/browser/browser_acknowledgeFeedbackAndDismissal.js` test with a11y-checks jobs, thus removing the temporary `fail-if` notation from its test manifest.
Differential Revision: https://phabricator.services.mozilla.com/D202897
This makes sure the row label transfers to the dismissal acknowledgment tip, if
the dismissed row has a label.
With that fixed, there's one other cosmetic problem where the tip's top border
is right up against the row label. It doesn't look good. I added some additional
space between the label and border.
Differential Revision: https://phabricator.services.mozilla.com/D203559
This modifies the Rust backend so it always ingests whenever it's enabled,
including on every startup.
Ideally we would test a schema update, but since the current schema version is
hardcoded Rust, I don't think that's possible.
Differential Revision: https://phabricator.services.mozilla.com/D203855
