mirror of
https://github.com/mozilla/gecko-dev.git
synced 2024-10-22 09:45:41 +00:00
Bug 1579612 - Properly extract tags from urlbar results in all cases. r=mak
We extract tags from the result title only when `info.style.includes("bookmark")`, but that only captures one of the two cases where we include tags in the title: the "bookmark" style. We also include tags for the "tag" style. So the bug reports in this bug are hitting that "tag" case. It doesn't have anything to do with non-Latin tags afaict (see the bug summary).
I took the opportunity to streamline `UnifiedComplete._addFilteredQueryMatch`, which was a little hard to follow. I had to look at it to make sure I captured all the cases where tags are included in the title. I think I've made it easier to follow.
Differential Revision: https://phabricator.services.mozilla.com/D49223
--HG--
extra : moz-landing-system : lando
This commit is contained in:
Drew Willcoxon
parent
d9a4fb7dd3
commit
1643004a5d
@ -386,32 +386,44 @@ function makeUrlbarResult(tokens, info) {
|
||||
let source;
|
||||
let tags = [];
|
||||
let comment = info.comment;
|
||||
|
||||
// UnifiedComplete may return "bookmark", "bookmark-tag" or "tag". In the last
|
||||
// case it should not be considered a bookmark, but an history item with tags.
|
||||
// We don't show tags for non bookmarked items though.
|
||||
if (info.style.includes("bookmark")) {
|
||||
source = UrlbarUtils.RESULT_SOURCE.BOOKMARKS;
|
||||
if (info.style.includes("tag")) {
|
||||
// Split title and tags.
|
||||
[comment, tags] = info.comment.split(UrlbarUtils.TITLE_TAGS_SEPARATOR);
|
||||
// Tags are separated by a comma and in a random order.
|
||||
// We should also just include tags that match the searchString.
|
||||
tags = tags
|
||||
.split(",")
|
||||
.map(t => t.trim())
|
||||
.filter(tag => {
|
||||
let lowerCaseTag = tag.toLocaleLowerCase();
|
||||
return tokens.some(token =>
|
||||
lowerCaseTag.includes(token.lowerCaseValue)
|
||||
);
|
||||
})
|
||||
.sort();
|
||||
}
|
||||
} else if (info.style.includes("preloaded-top-sites")) {
|
||||
source = UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL;
|
||||
} else {
|
||||
source = UrlbarUtils.RESULT_SOURCE.HISTORY;
|
||||
}
|
||||
|
||||
// If the style indicates that the result is tagged, then the tags are
|
||||
// included in the title, and we must extract them.
|
||||
if (info.style.includes("tag")) {
|
||||
[comment, tags] = info.comment.split(UrlbarUtils.TITLE_TAGS_SEPARATOR);
|
||||
|
||||
// However, as mentioned above, we don't want to show tags for non-
|
||||
// bookmarked items, so we include tags in the final result only if it's
|
||||
// bookmarked, and we drop the tags otherwise.
|
||||
if (source != UrlbarUtils.RESULT_SOURCE.BOOKMARKS) {
|
||||
tags = "";
|
||||
}
|
||||
|
||||
// Tags are separated by a comma and in a random order.
|
||||
// We should also just include tags that match the searchString.
|
||||
tags = tags
|
||||
.split(",")
|
||||
.map(t => t.trim())
|
||||
.filter(tag => {
|
||||
let lowerCaseTag = tag.toLocaleLowerCase();
|
||||
return tokens.some(token =>
|
||||
lowerCaseTag.includes(token.lowerCaseValue)
|
||||
);
|
||||
})
|
||||
.sort();
|
||||
}
|
||||
|
||||
return new UrlbarResult(
|
||||
UrlbarUtils.RESULT_TYPE.URL,
|
||||
source,
|
||||
|
||||
@ -109,4 +109,124 @@ add_task(async function test_unifiedComplete() {
|
||||
["moz", "mozilla", "org"],
|
||||
"Check tags"
|
||||
);
|
||||
|
||||
await PlacesUtils.history.clear();
|
||||
await PlacesUtils.bookmarks.eraseEverything();
|
||||
UrlbarProviderOpenTabs.unregisterOpenTab("https://tab.mozilla.org/", 0);
|
||||
});
|
||||
|
||||
add_task(async function test_bookmarkBehaviorDisabled_tagged() {
|
||||
Services.prefs.setBoolPref(SUGGEST_PREF, false);
|
||||
Services.prefs.setBoolPref(SUGGEST_ENABLED_PREF, false);
|
||||
|
||||
// Disable the bookmark behavior in UnifiedComplete.
|
||||
Services.prefs.setBoolPref("browser.urlbar.suggest.bookmark", false);
|
||||
|
||||
let controller = new UrlbarController({
|
||||
browserWindow: {
|
||||
location: {
|
||||
href: AppConstants.BROWSER_CHROME_URL,
|
||||
},
|
||||
},
|
||||
});
|
||||
// Also check case insensitivity.
|
||||
let searchString = "MoZ oRg";
|
||||
let context = createContext(searchString, { isPrivate: false });
|
||||
|
||||
// Add a tagged bookmark that's also visited.
|
||||
await PlacesUtils.bookmarks.insert({
|
||||
url: "https://bookmark.mozilla.org/",
|
||||
title: "Test bookmark",
|
||||
parentGuid: PlacesUtils.bookmarks.unfiledGuid,
|
||||
});
|
||||
PlacesUtils.tagging.tagURI(
|
||||
Services.io.newURI("https://bookmark.mozilla.org/"),
|
||||
["mozilla", "org", "ham", "moz", "bacon"]
|
||||
);
|
||||
await PlacesTestUtils.addVisits("https://bookmark.mozilla.org/");
|
||||
|
||||
await controller.startQuery(context);
|
||||
|
||||
info(
|
||||
"Results:\n" +
|
||||
context.results.map(m => `${m.title} - ${m.payload.url}`).join("\n")
|
||||
);
|
||||
Assert.equal(
|
||||
context.results.length,
|
||||
2,
|
||||
"Found the expected number of matches"
|
||||
);
|
||||
|
||||
Assert.deepEqual(
|
||||
[UrlbarUtils.RESULT_TYPE.SEARCH, UrlbarUtils.RESULT_TYPE.URL],
|
||||
context.results.map(m => m.type),
|
||||
"Check result types"
|
||||
);
|
||||
|
||||
Assert.deepEqual(
|
||||
[searchString, "Test bookmark"],
|
||||
context.results.map(m => m.title),
|
||||
"Check match titles"
|
||||
);
|
||||
|
||||
Assert.deepEqual(context.results[1].payload.tags, [], "Check tags");
|
||||
|
||||
await PlacesUtils.history.clear();
|
||||
await PlacesUtils.bookmarks.eraseEverything();
|
||||
});
|
||||
|
||||
add_task(async function test_bookmarkBehaviorDisabled_untagged() {
|
||||
Services.prefs.setBoolPref(SUGGEST_PREF, false);
|
||||
Services.prefs.setBoolPref(SUGGEST_ENABLED_PREF, false);
|
||||
|
||||
// Disable the bookmark behavior in UnifiedComplete.
|
||||
Services.prefs.setBoolPref("browser.urlbar.suggest.bookmark", false);
|
||||
|
||||
let controller = new UrlbarController({
|
||||
browserWindow: {
|
||||
location: {
|
||||
href: AppConstants.BROWSER_CHROME_URL,
|
||||
},
|
||||
},
|
||||
});
|
||||
// Also check case insensitivity.
|
||||
let searchString = "MoZ oRg";
|
||||
let context = createContext(searchString, { isPrivate: false });
|
||||
|
||||
// Add an *untagged* bookmark that's also visited.
|
||||
await PlacesUtils.bookmarks.insert({
|
||||
url: "https://bookmark.mozilla.org/",
|
||||
title: "Test bookmark",
|
||||
parentGuid: PlacesUtils.bookmarks.unfiledGuid,
|
||||
});
|
||||
await PlacesTestUtils.addVisits("https://bookmark.mozilla.org/");
|
||||
|
||||
await controller.startQuery(context);
|
||||
|
||||
info(
|
||||
"Results:\n" +
|
||||
context.results.map(m => `${m.title} - ${m.payload.url}`).join("\n")
|
||||
);
|
||||
Assert.equal(
|
||||
context.results.length,
|
||||
2,
|
||||
"Found the expected number of matches"
|
||||
);
|
||||
|
||||
Assert.deepEqual(
|
||||
[UrlbarUtils.RESULT_TYPE.SEARCH, UrlbarUtils.RESULT_TYPE.URL],
|
||||
context.results.map(m => m.type),
|
||||
"Check result types"
|
||||
);
|
||||
|
||||
Assert.deepEqual(
|
||||
[searchString, "Test bookmark"],
|
||||
context.results.map(m => m.title),
|
||||
"Check match titles"
|
||||
);
|
||||
|
||||
Assert.deepEqual(context.results[1].payload.tags, [], "Check tags");
|
||||
|
||||
await PlacesUtils.history.clear();
|
||||
await PlacesUtils.bookmarks.eraseEverything();
|
||||
});
|
||||
|
||||
@ -2390,9 +2390,8 @@ Search.prototype = {
|
||||
},
|
||||
|
||||
_addFilteredQueryMatch(row) {
|
||||
let match = {};
|
||||
match.placeId = row.getResultByIndex(QUERYINDEX_PLACEID);
|
||||
let escapedURL = row.getResultByIndex(QUERYINDEX_URL);
|
||||
let placeId = row.getResultByIndex(QUERYINDEX_PLACEID);
|
||||
let url = row.getResultByIndex(QUERYINDEX_URL);
|
||||
let openPageCount = row.getResultByIndex(QUERYINDEX_SWITCHTAB) || 0;
|
||||
let historyTitle = row.getResultByIndex(QUERYINDEX_TITLE) || "";
|
||||
let bookmarked = row.getResultByIndex(QUERYINDEX_BOOKMARKED);
|
||||
@ -2402,70 +2401,39 @@ Search.prototype = {
|
||||
let tags = row.getResultByIndex(QUERYINDEX_TAGS) || "";
|
||||
let frecency = row.getResultByIndex(QUERYINDEX_FRECENCY);
|
||||
|
||||
// If actions are enabled and the page is open, add only the switch-to-tab
|
||||
// result. Otherwise, add the normal result.
|
||||
let url = escapedURL;
|
||||
let action = null;
|
||||
let match = {
|
||||
placeId,
|
||||
value: url,
|
||||
comment: bookmarkTitle || historyTitle,
|
||||
icon: iconHelper(url),
|
||||
frecency: frecency || FRECENCY_DEFAULT,
|
||||
};
|
||||
|
||||
if (
|
||||
this._enableActions &&
|
||||
openPageCount > 0 &&
|
||||
this.hasBehavior("openpage")
|
||||
) {
|
||||
url = PlacesUtils.mozActionURI("switchtab", { url: escapedURL });
|
||||
action = "switchtab";
|
||||
if (frecency == null) {
|
||||
frecency = FRECENCY_DEFAULT;
|
||||
}
|
||||
}
|
||||
|
||||
// Always prefer the bookmark title unless it is empty
|
||||
let title = bookmarkTitle || historyTitle;
|
||||
|
||||
// Return tags as part of the title, unless the match has an action, like
|
||||
// switch-to-tab, that doesn't care about them.
|
||||
let showTags = !!tags && !action;
|
||||
|
||||
// We'll act as if the page is not bookmarked when the user wants
|
||||
// only history and not bookmarks and there are no tags.
|
||||
if (
|
||||
// Actions are enabled and the page is open. Add a switch-to-tab result.
|
||||
match.value = PlacesUtils.mozActionURI("switchtab", { url: match.value });
|
||||
match.style = "action switchtab";
|
||||
} else if (
|
||||
this.hasBehavior("history") &&
|
||||
!this.hasBehavior("bookmark") &&
|
||||
!showTags
|
||||
!tags
|
||||
) {
|
||||
showTags = false;
|
||||
// The consumer wants only history and not bookmarks and there are no
|
||||
// tags. We'll act as if the page is not bookmarked.
|
||||
match.style = "favicon";
|
||||
} else if (tags) {
|
||||
// Store the tags in the title. It's up to the consumer to extract them.
|
||||
match.comment += UrlbarUtils.TITLE_TAGS_SEPARATOR + tags;
|
||||
// If we're not suggesting bookmarks, then this shouldn't display as one.
|
||||
match.style = this.hasBehavior("bookmark") ? "bookmark-tag" : "tag";
|
||||
} else if (bookmarked) {
|
||||
match.style = "bookmark";
|
||||
}
|
||||
|
||||
// If we have tags and should show them, we need to add them to the title.
|
||||
if (showTags) {
|
||||
title += UrlbarUtils.TITLE_TAGS_SEPARATOR + tags;
|
||||
}
|
||||
|
||||
// We have to determine the right style to display. Tags show the tag icon,
|
||||
// bookmarks get the bookmark icon, and keywords get the keyword icon. If
|
||||
// the result does not fall into any of those, it just gets the favicon.
|
||||
if (!match.style) {
|
||||
// It is possible that we already have a style set (from a keyword
|
||||
// search or because of the user's preferences), so only set it if we
|
||||
// haven't already done so.
|
||||
if (showTags) {
|
||||
// If we're not suggesting bookmarks, then this shouldn't
|
||||
// display as one.
|
||||
match.style = this.hasBehavior("bookmark") ? "bookmark-tag" : "tag";
|
||||
} else if (bookmarked) {
|
||||
match.style = "bookmark";
|
||||
}
|
||||
}
|
||||
|
||||
if (action) {
|
||||
match.style = "action " + action;
|
||||
}
|
||||
|
||||
match.value = url;
|
||||
match.comment = title;
|
||||
match.icon = iconHelper(escapedURL);
|
||||
match.frecency = frecency;
|
||||
|
||||
this._addMatch(match);
|
||||
},
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user