commit b11335d4e2546ebe77787965bdf16af3b81a0b8e
parent 975653d57082e79f623516a3ca4376ac27a2dda3
Author: Drew Willcoxon <adw@mozilla.com>
Date: Wed, 26 Nov 2025 08:23:24 +0000
Bug 2002383 - Make online AMP suggestions dismissable by their full keyword, and add support for dismissal keys to all Merino suggestions. r=daisuke
This does two things:
1. Make Firefox check for `suggestion.dismissal_key` for all Merino suggestions.
It already does that for unmanaged Merino suggestions. When defined,
`QuickSuggest.dismissResult()` will use it when dismissing a `UrlbarResult`
created from a Merino suggestion [1].
2. In addition, for Merino AMP suggestions, fall back to the full keyword if the
suggestion does not define `dismissal_key`.
That way Firefox will use full keywords to dismiss Merino AMP suggestions, and
later if Merino starts including `dismissal_key` in AMP suggestions, Firefox
will handle that too.
Please see the bug for details.
[1] https://searchfox.org/firefox-main/rev/af0f713f785417023666ef557e856b3a9d132041/browser/components/urlbar/QuickSuggest.sys.mjs#482-485
Differential Revision: https://phabricator.services.mozilla.com/D274079
Diffstat:
5 files changed, 273 insertions(+), 16 deletions(-)
diff --git a/browser/components/urlbar/UrlbarProviderQuickSuggest.sys.mjs b/browser/components/urlbar/UrlbarProviderQuickSuggest.sys.mjs
@@ -368,9 +368,21 @@ export class UrlbarProviderQuickSuggest extends UrlbarProvider {
result.payload.isSponsored = feature
? feature.isSuggestionSponsored(suggestion)
: !!suggestion.is_sponsored;
- if (suggestion.source == "rust") {
- // `suggestionObject` is passed back into the Rust component on dismissal.
- result.payload.suggestionObject = suggestion;
+
+ switch (suggestion.source) {
+ case "merino":
+ // Set `dismissalKey` unless the feature already did it.
+ if (
+ suggestion.dismissal_key &&
+ !result.payload.hasOwnProperty("dismissalKey")
+ ) {
+ result.payload.dismissalKey = suggestion.dismissal_key;
+ }
+ break;
+ case "rust":
+ // `suggestionObject` is passed back to the Rust component on dismissal.
+ result.payload.suggestionObject = suggestion;
+ break;
}
// Handle icons here so each feature doesn't have to do it, but use `||=` to
@@ -443,7 +455,6 @@ export class UrlbarProviderQuickSuggest extends UrlbarProvider {
let payload = {
url: suggestion.url,
originalUrl: suggestion.original_url,
- dismissalKey: suggestion.dismissal_key,
isBlockable: true,
isManageable: true,
};
diff --git a/browser/components/urlbar/private/AmpSuggestions.sys.mjs b/browser/components/urlbar/private/AmpSuggestions.sys.mjs
@@ -104,17 +104,13 @@ export class AmpSuggestions extends SuggestProvider {
// Normalize the Merino suggestion so it has camelCased properties like
// Rust suggestions.
- suggestion = {
- title: suggestion.title,
- url: suggestion.url,
- fullKeyword: suggestion.full_keyword,
- impressionUrl: suggestion.impression_url,
- clickUrl: suggestion.click_url,
- blockId: suggestion.block_id,
- advertiser: suggestion.advertiser,
- iabCategory: suggestion.iab_category,
- requestId: suggestion.request_id,
- };
+ suggestion.fullKeyword = suggestion.full_keyword;
+ suggestion.impressionUrl = suggestion.impression_url;
+ suggestion.clickUrl = suggestion.click_url;
+ suggestion.blockId = suggestion.block_id;
+ suggestion.iabCategory = suggestion.iab_category;
+ suggestion.requestId = suggestion.request_id;
+ suggestion.dismissalKey = suggestion.dismissal_key;
}
let payload = {
@@ -132,6 +128,15 @@ export class AmpSuggestions extends SuggestProvider {
isManageable: true,
};
+ // AMP suggestions are dismissed by their full keyword, not by URL like
+ // usual. For Rust suggestions, the Rust component handles that for us, no
+ // need to do anything here. For Merino suggestions, Merino should include
+ // `dismissal_key`, but fall back to the full keyword in case it doesn't.
+ if (suggestion.source == "merino") {
+ payload.dismissalKey =
+ suggestion.dismissalKey || suggestion.fullKeyword || originalUrl;
+ }
+
let isTopPick =
lazy.UrlbarPrefs.get("quickSuggestAmpTopPickCharThreshold") &&
lazy.UrlbarPrefs.get("quickSuggestAmpTopPickCharThreshold") <=
diff --git a/browser/components/urlbar/tests/quicksuggest/QuickSuggestTestUtils.sys.mjs b/browser/components/urlbar/tests/quicksuggest/QuickSuggestTestUtils.sys.mjs
@@ -469,6 +469,7 @@ class _QuickSuggestTestUtils {
isSuggestedIndexRelativeToGroup = true,
isBestMatch = false,
requestId = undefined,
+ dismissalKey = undefined,
descriptionL10n = { id: "urlbar-result-action-sponsored" },
categories = [],
} = {}) {
@@ -525,6 +526,7 @@ class _QuickSuggestTestUtils {
});
} else {
result.payload.icon = icon;
+ result.payload.dismissalKey = dismissalKey || fullKeyword || originalUrl;
}
return result;
diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_merino.js b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_merino.js
@@ -392,6 +392,239 @@ add_task(async function dismissals_managed() {
merinoClient().resetSession();
});
+// Tests dismissals of Merino AMP suggestions, which have special handling
+// around their dismissal keys.
+add_task(async function dismissals_amp() {
+ UrlbarPrefs.set("quicksuggest.online.available", true);
+ UrlbarPrefs.set("quicksuggest.online.enabled", true);
+
+ UrlbarPrefs.set("suggest.quicksuggest.all", true);
+ UrlbarPrefs.set("suggest.quicksuggest.sponsored", true);
+ await QuickSuggestTestUtils.forceSync();
+
+ let tests = [
+ {
+ suggestion: {
+ url: "https://example.com/0",
+ },
+ expected: {
+ // dismissal key should be the `url` value
+ dismissalKey: "https://example.com/0",
+ },
+ },
+ {
+ suggestion: {
+ url: `https://example.com/1-${TIMESTAMP_TEMPLATE}`,
+ },
+ expected: {
+ // dismissal key should be the original `url` value with the timestamp
+ // template
+ dismissalKey: `https://example.com/1-${TIMESTAMP_TEMPLATE}`,
+ },
+ },
+ {
+ suggestion: {
+ url: "https://example.com/2",
+ full_keyword: "full keyword 2",
+ },
+ expected: {
+ // dismissal key should be the `full_keyword` value
+ dismissalKey: "full keyword 2",
+ notDismissalKeys: ["https://example.com/2"],
+ },
+ },
+ {
+ suggestion: {
+ url: `https://example.com/3-${TIMESTAMP_TEMPLATE}`,
+ full_keyword: "full keyword 3",
+ },
+ expected: {
+ // dismissal key should be the `full_keyword` value
+ dismissalKey: "full keyword 3",
+ notDismissalKeys: [`https://example.com/3-${TIMESTAMP_TEMPLATE}`],
+ },
+ },
+ {
+ suggestion: {
+ url: "https://example.com/4",
+ dismissal_key: "4-dismissal-key",
+ },
+ expected: {
+ // dismissal key should be the `dismissal_key` value
+ dismissalKey: "4-dismissal-key",
+ notDismissalKeys: ["https://example.com/4"],
+ },
+ },
+ {
+ suggestion: {
+ url: `https://example.com/5-${TIMESTAMP_TEMPLATE}`,
+ dismissal_key: "5-dismissal-key",
+ },
+ expected: {
+ // dismissal key should be the `dismissal_key` value
+ dismissalKey: "5-dismissal-key",
+ notDismissalKeys: [`https://example.com/5-${TIMESTAMP_TEMPLATE}`],
+ },
+ },
+ {
+ suggestion: {
+ url: "https://example.com/6",
+ full_keyword: "full keyword 6",
+ dismissal_key: "6-dismissal-key",
+ },
+ expected: {
+ // dismissal key should be the `dismissal_key` value
+ dismissalKey: "6-dismissal-key",
+ notDismissalKeys: ["full keyword 6", "https://example.com/6"],
+ },
+ },
+ {
+ suggestion: {
+ url: `https://example.com/7-${TIMESTAMP_TEMPLATE}`,
+ full_keyword: "full keyword 7",
+ dismissal_key: "7-dismissal-key",
+ },
+ expected: {
+ // dismissal key should be the `dismissal_key` value
+ dismissalKey: "7-dismissal-key",
+ notDismissalKeys: [
+ "full keyword 7",
+ `https://example.com/7-${TIMESTAMP_TEMPLATE}`,
+ ],
+ },
+ },
+ ];
+
+ for (let test of tests) {
+ info("Doing subtest: " + JSON.stringify(test));
+
+ let { suggestion, expected } = test;
+
+ suggestion = {
+ provider: "adm",
+ title: "title",
+ icon: null,
+ impression_url: "https://example.com/impression",
+ click_url: "https://example.com/click",
+ block_id: 1,
+ advertiser: "advertiser",
+ iab_category: "22 - Shopping",
+ is_sponsored: true,
+ request_id: "request_id",
+ score: 1,
+ ...suggestion,
+ };
+
+ MerinoTestUtils.server.response =
+ MerinoTestUtils.server.makeDefaultResponse();
+ MerinoTestUtils.server.response.body.suggestions = [suggestion];
+
+ let expectedResult = {
+ type: UrlbarUtils.RESULT_TYPE.URL,
+ source: UrlbarUtils.RESULT_SOURCE.SEARCH,
+ heuristic: false,
+ payload: {
+ provider: suggestion.provider,
+ title: suggestion.title,
+ url: suggestion.url,
+ originalUrl: suggestion.original_url || suggestion.url,
+ displayUrl: suggestion.url.replace(/^https:\/\//, ""),
+ dismissalKey: expected.dismissalKey,
+ requestId: suggestion.request_id,
+ sponsoredImpressionUrl: suggestion.impression_url,
+ sponsoredClickUrl: suggestion.click_url,
+ sponsoredBlockId: suggestion.block_id,
+ sponsoredAdvertiser: suggestion.advertiser,
+ sponsoredIabCategory: suggestion.iab_category,
+ qsSuggestion: suggestion.full_keyword,
+ isBlockable: true,
+ isManageable: true,
+ isSponsored: true,
+ source: "merino",
+ telemetryType: "adm_sponsored",
+ descriptionL10n: { id: "urlbar-result-action-sponsored" },
+ },
+ };
+
+ // Do a search. The Merino suggestion should be matched.
+ let context = createContext(SEARCH_STRING, {
+ providers: [UrlbarProviderQuickSuggest.name],
+ isPrivate: false,
+ });
+ await check_results({
+ context,
+ matches: [expectedResult],
+ // Ignore values related to the timestamp template. They're not important
+ // for this test.
+ conditionalPayloadProperties: {
+ url: { ignore: true },
+ urlTimestampIndex: { ignore: true },
+ displayUrl: { ignore: true },
+ },
+ });
+
+ let result = context.results[0];
+ Assert.equal(
+ QuickSuggest.getFeatureByResult(result)?.name,
+ "AmpSuggestions",
+ "Sanity check: The actual result should be managed by AmpSuggestions"
+ );
+
+ // Dismiss the Merino result.
+ await QuickSuggest.dismissResult(result);
+ Assert.ok(
+ await QuickSuggest.isResultDismissed(result),
+ "isResultDismissed should return true after dismissing result"
+ );
+
+ Assert.ok(
+ await QuickSuggest.rustBackend.isDismissedByKey(expected.dismissalKey),
+ "isDismissedByKey should return true after dismissing result"
+ );
+ if (expected.notDismissalKeys) {
+ for (let value of expected.notDismissalKeys) {
+ Assert.ok(
+ !(await QuickSuggest.rustBackend.isDismissedByKey(value)),
+ "isDismissedByKey should return false for notDismissalKey: " + value
+ );
+ }
+ }
+
+ // Do another search. The remote settings suggestion should now be matched.
+ await check_results({
+ context: createContext(SEARCH_STRING, {
+ providers: [UrlbarProviderQuickSuggest.name],
+ isPrivate: false,
+ }),
+ matches: [EXPECTED_REMOTE_SETTINGS_URLBAR_RESULT],
+ });
+
+ // Clear dismissals.
+ await QuickSuggest.clearDismissedSuggestions();
+ Assert.ok(
+ !(await QuickSuggest.isResultDismissed(result)),
+ "isResultDismissed should return false after clearing dismissals"
+ );
+
+ // The Merino suggestion should be matched again.
+ await check_results({
+ context: createContext(SEARCH_STRING, {
+ providers: [UrlbarProviderQuickSuggest.name],
+ isPrivate: false,
+ }),
+ matches: [expectedResult],
+ conditionalPayloadProperties: {
+ url: { ignore: true },
+ urlTimestampIndex: { ignore: true },
+ displayUrl: { ignore: true },
+ },
+ });
+ }
+
+ MerinoTestUtils.server.reset();
+ merinoClient().resetSession();
+});
+
// Tests dismissals of unmanaged Merino suggestions (suggestions that are not
// managed by a `SuggestFeature`).
add_task(async function dismissals_unmanaged_1() {
@@ -720,6 +953,7 @@ add_task(async function dismissals_unmanaged_2() {
);
}
+ await QuickSuggest.clearDismissedSuggestions();
MerinoTestUtils.server.reset();
merinoClient().resetSession();
});
diff --git a/browser/components/urlbar/tests/unit/head.js b/browser/components/urlbar/tests/unit/head.js
@@ -992,6 +992,9 @@ function makeGlobalActionsResult({
* @param {string} [options.completed]
* The value that would be filled if the autofill result was confirmed.
* Has no effect if `autofilled` is not specified.
+ * @param {object} [options.conditionalPayloadProperties]
+ * An object mapping payload property names to objects { optional, ignore }.
+ * See the code below.
* @param {Array} options.matches
* An array of UrlbarResults.
*/
@@ -1001,6 +1004,7 @@ async function check_results({
autofilled,
completed,
matches = [],
+ conditionalPayloadProperties = {},
} = {}) {
if (!context) {
return;
@@ -1093,7 +1097,7 @@ async function check_results({
// Always ignore the property.
// optional:
// Ignore the property if it's not in the expected result.
- let conditionalPayloadProperties = {
+ conditionalPayloadProperties = {
frecency: { optional: true },
lastVisit: { optional: true },
// `suggestionObject` is only used for dismissing Suggest Rust results, and
@@ -1101,6 +1105,7 @@ async function check_results({
// payload object, so ignore it. There are Suggest tests specifically for
// dismissals that indirectly test the important aspects of this property.
suggestionObject: { ignore: true },
+ ...conditionalPayloadProperties,
};
for (let i = 0; i < matches.length; i++) {
