corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1759879 - Add error messages to add engine dialog. r=search-reviewers,fluent-reviewers,settings-reviewers,desktop-theme-reviewers,urlbar-reviewers,bolsson,mossop,Standard8,dao

Browse Source 
Differential Revision: https://phabricator.services.mozilla.com/D245411
This commit is contained in:
Moritz Beier
2025-05-07 12:41:14 +00:00
committed by mbeier@mozilla.com
parent af48d212c2
commit 167482d903
10 changed files with 379 additions and 168 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
browser/components/preferences/search.inc.xhtml
View File

@@ -174,8 +174,8 @@
search-l10n-ids="
add-engine-button,
add-engine-name,
add-engine-keyword,
add-engine-url,
add-engine-keyword2,
add-engine-url2,
add-engine-dialog.buttonlabelaccept,
"
/>

103
browser/components/preferences/tests/browser_search_userEngineDialog.js
View File

@@ -81,42 +81,84 @@ add_task(async function test_validation() {
let name = dialogWin.document.getElementById("engineName");
let url = dialogWin.document.getElementById("engineUrl");
let alias = dialogWin.document.getElementById("engineAlias");
let suggestUrl = dialogWin.document.getElementById("suggestUrl");
Assert.ok(
name.value == "" && url.value == "" && alias.value == "",
"Everything is empty initially."
);
Assert.ok(button.disabled, "Button is disabled initially.");
await assertError(name, "add-engine-no-name");
await assertError(url, "add-engine-no-url");
await assertError(alias, null);
await assertError(suggestUrl, null);
setName("Example", dialogWin);
setUrl("https://example.com/search?q=%s", dialogWin);
await setAlias("abc", dialogWin);
setSuggestUrl("https://example.com/search?q=%s", dialogWin);
Assert.ok(!button.disabled, "Button is enabled when everything is there.");
// Check name
setName("", dialogWin);
await assertError(name, "add-engine-no-name");
Assert.ok(button.disabled, "Name is required.");
setName(existingEngine.name, dialogWin);
await assertError(name, "add-engine-name-exists");
Assert.ok(button.disabled, "Existing name is not allowed.");
setName("Example", dialogWin);
await assertError(name, null);
Assert.ok(!button.disabled, "Good name enables the button.");
// Check URL
setUrl("", dialogWin);
Assert.ok(button.disabled, "URL is required.");
setUrl("javascript://%s", dialogWin);
Assert.ok(button.disabled, "Javascript URLs are not allowed.");
setUrl("https://example.com/search?q=%s", dialogWin);
Assert.ok(!button.disabled, "Good URL enables the button.");
await assertError(url, "add-engine-no-url");
// Check name
setName("", dialogWin);
Assert.ok(button.disabled, "Name is required.");
setName(existingEngine.name, dialogWin);
Assert.ok(button.disabled, "Existing name is not allowed.");
setName("Example", dialogWin);
Assert.ok(!button.disabled, "Good name enables the button.");
setUrl("javascript://%s", dialogWin);
await assertError(url, "add-engine-invalid-protocol");
Assert.ok(button.disabled, "Javascript URLs are not allowed.");
setUrl("not a url", dialogWin);
await assertError(url, "add-engine-invalid-url");
Assert.ok(button.disabled, "Invalid URLs are not allowed.");
setUrl("https://example.com/search?q=%s", dialogWin);
await assertError(url, null);
Assert.ok(!button.disabled, "Good URL enables the button.");
// Check alias
await setAlias("", dialogWin);
await assertError(alias, null);
Assert.ok(!button.disabled, "Alias is not required.");
await setAlias(existingEngine.alias, dialogWin);
await assertError(alias, "add-engine-keyword-exists");
Assert.ok(button.disabled, "Existing alias is not allowed.");
await setAlias(existingEngine.alias.toUpperCase(), dialogWin);
await assertError(alias, "add-engine-keyword-exists");
Assert.ok(button.disabled, "Alias duplicate test is case insensitive.");
await setAlias("abc", dialogWin);
await assertError(alias, null);
Assert.ok(!button.disabled, "Good alias enables the button.");
// Check suggest URL
setSuggestUrl("javascript://%s", dialogWin);
await assertError(suggestUrl, "add-engine-invalid-protocol");
Assert.ok(button.disabled, "Javascript URLs are not allowed.");
setSuggestUrl("not a url", dialogWin);
await assertError(suggestUrl, "add-engine-invalid-url");
Assert.ok(button.disabled, "Invalid URLs are not allowed.");
setSuggestUrl("https://example.com/search?q=%s", dialogWin);
await assertError(suggestUrl, null);
Assert.ok(!button.disabled, "Good URL enables the button.");
// Clean up.
BrowserTestUtils.removeTab(gBrowser.selectedTab);
await Services.search.removeEngine(existingEngine);
@@ -158,8 +200,12 @@ add_task(async function test_editEngine() {
Assert.ok(!!userEngineIndex, "User engine is in the table.");
view.selection.select(userEngineIndex);
// Open the dialog.
// Open the dialog and check values.
let dialogWin = await openDialogWith(doc, () => editButton.click());
let acceptButton = dialogWin.document
.querySelector("dialog")
.shadowRoot.querySelector('button[dlgtype="accept"]');
Assert.equal(
dialogWin.document.getElementById("titleContainer").style.display,
"none",
@@ -201,7 +247,7 @@ add_task(async function test_editEngine() {
SearchUtils.TOPIC_ENGINE_MODIFIED,
3
);
EventUtils.synthesizeKey("VK_RETURN", {}, dialogWin);
acceptButton.click();
await promiseChanged;
Assert.ok(true, "Got 3 change notifications.");
@@ -301,11 +347,11 @@ add_task(async function test_editPostEngine() {
let promiseChanged = SearchTestUtils.promiseSearchNotification(
SearchUtils.MODIFIED_TYPE.CHANGED,
SearchUtils.TOPIC_ENGINE_MODIFIED,
3
4
);
EventUtils.synthesizeKey("VK_RETURN", {}, dialogWin);
await promiseChanged;
Assert.ok(true, "Got 3 change notifications.");
Assert.ok(true, "Got 4 change notifications.");
// Open dialog again and check values.
dialogWin = await openDialogWith(doc, () => editButton.click());
@@ -370,6 +416,25 @@ add_task(async function test_editPostEngine() {
await Services.search.removeEngine(engine);
});
/**
* Checks the error label of an input of the add engine dialog.
*
* @param {HTMLInputElement} elt
* The element whose error should be checked
* @param {?string} error
* The l10n id of the expected error message or null if no error is expected.
*/
async function assertError(elt, error = null) {
let errorLabel = elt.parentElement.querySelector(".error-label");
if (error) {
let msg = await document.l10n.formatValue(error);
Assert.equal(errorLabel.textContent, msg);
} else {
Assert.equal(errorLabel.textContent, "valid");
}
}
async function openDialogWith(doc, fn) {
let dialogLoaded = TestUtils.topicObserved("subdialog-loaded");
await fn();
@@ -387,14 +452,14 @@ function setUrl(value, win) {
fillTextField("engineUrl", value, win);
}
function setPostData(value, win) {
fillTextField("enginePostData", value, win);
}
function setSuggestUrl(value, win) {
fillTextField("suggestUrl", value, win);
}
function setPostData(value, win) {
fillTextField("enginePostData", value, win);
}
async function setAlias(value, win) {
fillTextField("engineAlias", value, win);
await TestUtils.waitForTick();

45
browser/components/search/content/addEngine.css
View File

@@ -10,21 +10,52 @@ hbox {
width: 100%;
}
.dialogRow > label {
/* Align the labels with the inputs */
margin-inline-start: 4px;
#titleContainer {
padding-bottom: var(--space-xlarge);
--icon-url: url("chrome://browser/skin/preferences/category-search.svg")
}
#engineUrl,
#suggestUrl {
.dialogRow {
& > label,
& > .error-label {
/* Align the labels with the inputs */
margin-inline-start: 4px;
}
}
input[type="url"] {
/* Full URLs should always be displayed as LTR */
direction: ltr;
text-align: match-parent;
}
.dialogRow:not([hidden]) {
--grid-padding: 16px;
margin-block: 0 var(--grid-padding);
display: flex;
flex-direction: column;
}
.error-label {
display: flex;
font-size: var(--font-size-small);
margin-top: var(--space-xxsmall);
margin-bottom: var(--space-medium);
align-items: center;
gap: var(--space-xsmall);
color: var(--text-color-error);
visibility: hidden;
input:user-invalid + & {
visibility: visible;
}
&::before {
content: url("chrome://global/skin/icons/error.svg");
fill: var(--icon-color-critical);
-moz-context-properties: fill;
height: var(--icon-size-default);
width: var(--icon-size-default);
display: flex;
}
}

271
browser/components/search/content/addEngine.js
View File

@@ -9,7 +9,7 @@
// an HTML form. Depending on the scenario where it is used, different arguments
// must be supplied in an object in `window.arguments[0]`:
// - `mode` [required] - The type of dialog: NEW, EDIT or FORM.
// - `title` [optional] - To display a title in the window element.
// - `title` [optional] - Whether to display a title in the window element.
// - all arguments required by the constructor of the dialog class
const lazy = {};
@@ -29,6 +29,14 @@ document.l10n.setAttributes(
mode + "-engine-window"
);
let loadedResolvers = Promise.withResolvers();
document.mozSubdialogReady = loadedResolvers.promise;
/** @type {?EngineDialog} */
let gAddEngineDialog = null;
/** @type {?Map<string, string>} */
let l10nCache = null;
/**
* The abstract base class for all types of user search engine dialogs.
* All subclasses must implement the abstract method `onAddEngine`.
@@ -36,70 +44,181 @@ document.l10n.setAttributes(
class EngineDialog {
constructor() {
this._dialog = document.querySelector("dialog");
this._form = document.getElementById("addEngineForm");
this._name = document.getElementById("engineName");
this._alias = document.getElementById("engineAlias");
this._url = document.getElementById("engineUrl");
this._postData = document.getElementById("enginePostData");
this._suggestUrl = document.getElementById("suggestUrl");
this._name.addEventListener("input", this.onNameInput.bind(this));
this._alias.addEventListener("input", this.onAliasInput.bind(this));
this._url.addEventListener("input", this.onFormInput.bind(this));
document.addEventListener("dialogaccept", this.onAddEngine.bind(this));
this._form.addEventListener("input", e => this.validateInput(e.target));
document.addEventListener("dialogaccept", this.onAccept.bind(this));
}
onAddEngine() {
onAccept() {
throw new Error("abstract");
}
isNameValid(name) {
if (!name) {
return false;
}
return !Services.search.getEngineByName(name);
}
onNameInput() {
validateName() {
let name = this._name.value.trim();
let validity = this.isNameValid(name)
? ""
: document.getElementById("engineNameExists").textContent;
this._name.setCustomValidity(validity);
this.onFormInput();
}
async isAliasValid(alias) {
if (!alias) {
return true;
if (!name) {
this.setValidity(this._name, "add-engine-no-name");
return;
}
let existingEngine = Services.search.getEngineByName(name);
if (existingEngine && !this.allowedNames.includes(name)) {
this.setValidity(this._name, "add-engine-name-exists");
} else {
this.setValidity(this._name, null);
}
return !(await Services.search.getEngineByAlias(alias));
}
async onAliasInput() {
async validateAlias() {
let alias = this._alias.value.trim();
let validity = (await this.isAliasValid(alias))
? ""
: document.getElementById("engineAliasExists").textContent;
this._alias.setCustomValidity(validity);
this.onFormInput();
if (!alias) {
this.setValidity(this._alias, null);
return;
}
let existingEngine = await Services.search.getEngineByAlias(alias);
if (existingEngine && !this.allowedAliases.includes(alias)) {
this.setValidity(this._alias, "add-engine-keyword-exists");
} else {
this.setValidity(this._alias, null);
}
}
// This function is not set as a listener but called directly because it
// depends on the output of `isAliasValid`, but `isAliasValid` contains an
// await, so it would finish after this function.
onFormInput() {
/**
* Validates the passed URL input element and updates error messages.
*
* @param {HTMLInputElement} urlInput
* The URL input to validate.
* @param {boolean} required
* Whether the input is required or optional.
*/
validateUrlInput(urlInput, required) {
let urlString = urlInput.value.trim();
if (!urlString) {
if (required) {
this.setValidity(urlInput, "add-engine-no-url");
return;
}
this.setValidity(urlInput, null);
return;
}
let url = URL.parse(urlString);
if (!url) {
this.setValidity(urlInput, "add-engine-invalid-url");
return;
}
if (url.protocol == "http:" || url.protocol == "https:") {
this.setValidity(urlInput, null);
} else {
this.setValidity(urlInput, "add-engine-invalid-protocol");
}
}
/**
* Validates the passed input element and updates error messages.
*
* @param {HTMLInputElement} input
* The input element to validate.
*/
async validateInput(input) {
switch (input.id) {
case this._name.id:
this.validateName();
break;
case this._alias.id:
await this.validateAlias();
break;
case this._url.id:
this.validateUrlInput(this._url, true);
break;
case this._suggestUrl.id:
this.validateUrlInput(this._suggestUrl, false);
break;
}
}
async validateAll() {
for (let input of this._form.elements) {
await this.validateInput(input);
}
}
/**
* Sets the validity of the passed input element to the string belonging
* to the passed l10n id. Also updates the input's error label and
* the accept button.
*
* @param {HTMLInputElement} inputElement
* @param {string} l10nId
* The l10n id of the string to use as validity.
* Must be a key of `l10nCache`.
*/
setValidity(inputElement, l10nId) {
if (l10nId) {
inputElement.setCustomValidity(l10nCache.get(l10nId));
} else {
inputElement.setCustomValidity("");
}
let errorLabel = inputElement.parentElement.querySelector(".error-label");
let validationMessage = inputElement.validationMessage;
// If valid, set the error label to "valid" to ensure the layout doesn't shift.
// The CSS already hides the error label based on the validity of `inputElement`.
errorLabel.textContent = validationMessage || "valid";
this._dialog.setAttribute(
"buttondisabledaccept",
!this._form.checkValidity()
);
}
/**
* Engine names that always are allowed, even if they are already in use.
* This is needed for the edit engine dialog.
*
* @type {string[]}
*/
get allowedNames() {
return [];
}
/**
* Engine aliases that always are allowed, even if they are already in use.
* This is needed for the edit engine dialog.
*
* @type {string[]}
*/
get allowedAliases() {
return [];
}
}
/**
* This dialog is opened when adding a new search engine in preferences.
*/
class NewEngineDialog extends EngineDialog {
onAddEngine() {
constructor() {
super();
document.l10n.setAttributes(this._name, "add-engine-name-placeholder");
document.l10n.setAttributes(this._url, "add-engine-url-placeholder");
document.l10n.setAttributes(this._alias, "add-engine-keyword-placeholder");
document.getElementById("enginePostDataRow").remove();
this._postData = null;
this.validateAll();
}
onAccept() {
Services.search.addUserEngine({
name: this._name.value.trim(),
url: this._url.value.trim().replace(/%s/, "{searchTerms}"),
@@ -124,8 +243,6 @@ class EditEngineDialog extends EngineDialog {
*/
constructor({ engine }) {
super();
this._postData = document.getElementById("enginePostData");
this.#engine = engine;
this._name.value = engine.name;
this._alias.value = engine.alias ?? "";
@@ -135,20 +252,22 @@ class EditEngineDialog extends EngineDialog {
);
this._url.value = url;
if (postData) {
document.getElementById("enginePostDataRow").hidden = false;
this._postData.value = postData;
} else {
document.getElementById("enginePostDataRow").hidden = true;
}
let [suggestUrl] = this.getSubmissionTemplate(
lazy.SearchUtils.URL_TYPE.SUGGEST_JSON
);
this._suggestUrl.value = suggestUrl ?? "";
if (suggestUrl) {
this._suggestUrl.value = suggestUrl;
}
this.onNameInput();
this.onAliasInput();
this.validateAll();
}
onAddEngine() {
onAccept() {
this.#engine.wrappedJSObject.rename(this._name.value.trim());
this.#engine.alias = this._alias.value.trim();
@@ -180,20 +299,12 @@ class EditEngineDialog extends EngineDialog {
}
}
isNameValid(name) {
if (!name) {
return false;
}
let engine = Services.search.getEngineByName(this._name.value);
return !engine || engine.id == this.#engine.id;
get allowedAliases() {
return [this.#engine.alias];
}
async isAliasValid(alias) {
if (!alias) {
return true;
}
let engine = await Services.search.getEngineByAlias(this._alias.value);
return !engine || engine.id == this.#engine.id;
get allowedNames() {
return [this.#engine.name];
}
/**
@@ -249,24 +360,19 @@ class NewEngineFromFormDialog extends EngineDialog {
*/
constructor({ nameTemplate }) {
super();
this._name.value = nameTemplate;
this.onNameInput();
this.onAliasInput();
document.getElementById("engineUrlRow").remove();
this._url = null;
document.getElementById("suggestUrlRow").remove();
this._suggestUrl = null;
document.getElementById("enginePostDataRow").remove();
this._postData = null;
let title = { raw: document.title };
document.documentElement.setAttribute("headertitle", JSON.stringify(title));
document.documentElement.style.setProperty(
"--icon-url",
'url("chrome://browser/skin/preferences/category-search.svg")'
);
this._name.value = nameTemplate;
this.validateAll();
}
onAddEngine() {
onAccept() {
// Return the input to the caller.
window.arguments[0].engineInfo = {
name: this._name.value.trim(),
// Empty string means no alias.
@@ -275,16 +381,37 @@ class NewEngineFromFormDialog extends EngineDialog {
}
}
let loadedResolvers = Promise.withResolvers();
document.mozSubdialogReady = loadedResolvers.promise;
async function initL10nCache() {
const errorIds = [
"add-engine-name-exists",
"add-engine-keyword-exists",
"add-engine-no-name",
"add-engine-no-url",
"add-engine-invalid-protocol",
"add-engine-invalid-url",
];
let gAddEngineDialog = null;
window.addEventListener("DOMContentLoaded", () => {
if (!window.arguments[0].title) {
AdjustableTitle.hide();
let msgs = await document.l10n.formatValues(errorIds.map(id => ({ id })));
l10nCache = new Map();
for (let i = 0; i < errorIds.length; i++) {
l10nCache.set(errorIds[i], msgs[i]);
}
}
window.addEventListener("DOMContentLoaded", async () => {
try {
if (window.arguments[0].title) {
document.documentElement.setAttribute(
"headertitle",
JSON.stringify({ raw: document.title })
);
} else {
AdjustableTitle.hide();
}
await initL10nCache();
switch (window.arguments[0].mode) {
case "NEW":
gAddEngineDialog = new NewEngineDialog();

76
browser/components/search/content/addEngine.xhtml
View File

@@ -33,39 +33,45 @@
<script src="chrome://global/content/adjustableTitle.js" />
<script src="chrome://browser/content/search/addEngine.js" />
<separator class="thin" />
<html:form id="addEngineForm">
<html:div class="dialogRow">
<html:span
id="engineNameExists"
hidden="hidden"
data-l10n-id="engine-name-exists"
/>
<html:label
id="engineNameLabel"
for="engineName"
data-l10n-id="add-engine-name"
/>
<hbox>
<html:input id="engineName" type="text" required="required" />
</hbox>
<html:input id="engineName" type="text" required="required" />
<html:div class="error-label">valid</html:div>
</html:div>
<html:div class="dialogRow" id="engineUrlRow">
<html:label
id="engineUrlLabel"
for="engineUrl"
data-l10n-id="add-engine-url"
data-l10n-id="add-engine-url2"
/>
<hbox>
<html:input
id="engineUrl"
type="url"
required="required"
pattern="https?:.*"
/>
</hbox>
<html:input id="engineUrl" type="url" required="required" />
<html:div class="error-label">valid</html:div>
</html:div>
<html:div class="dialogRow" id="enginePostDataRow">
<html:label
id="enginePostDataLabel"
for="enginePostData"
data-l10n-id="add-engine-post-data"
/>
<html:input id="enginePostData" type="text" />
<html:div class="error-label">valid</html:div>
</html:div>
<html:div class="dialogRow">
<html:label
id="engineAliasLabel"
for="engineAlias"
data-l10n-id="add-engine-keyword2"
/>
<html:input id="engineAlias" type="text" />
<html:div class="error-label">valid</html:div>
</html:div>
<html:div class="dialogRow" id="suggestUrlRow">
@@ -74,36 +80,8 @@
for="suggestUrl"
data-l10n-id="add-engine-suggest-url"
/>
<hbox>
<html:input id="suggestUrl" type="url" pattern="https?:.*" />
</hbox>
</html:div>
<html:div class="dialogRow" id="enginePostDataRow" hidden="hidden">
<html:label
id="enginePostDataLabel"
for="enginePostData"
data-l10n-id="add-engine-post-data"
/>
<hbox>
<html:input id="enginePostData" type="text" />
</hbox>
</html:div>
<html:div class="dialogRow">
<html:span
id="engineAliasExists"
hidden="hidden"
data-l10n-id="engine-keyword-exists"
/>
<html:label
id="engineAliasLabel"
for="engineAlias"
data-l10n-id="add-engine-keyword"
/>
<hbox>
<html:input id="engineAlias" type="text" />
</hbox>
<html:input id="suggestUrl" type="url" />
<html:div class="error-label">valid</html:div>
</html:div>
</html:form>
</dialog>

28
browser/components/urlbar/content/enUS-searchFeatures.ftl
View File

@@ -315,7 +315,13 @@ urlbar-search-mode-actions-en = Actions
## Add search engine dialog
## These strings will be moved to search.ftl once the dialog is finished.
add-engine-suggest-url = Search suggestion URL, use %s in place of the search term
add-engine-url2 = URL with %s in place of search term
add-engine-keyword2 = Keyword (optional)
add-engine-post-data = Post data with %s in place of search term
add-engine-suggest-url = Suggestions URL with %s in place of search term (optional)
# Variables:
# $name (string) - Name of a search engine.
@@ -329,4 +335,22 @@ edit-engine-window =
.title = Edit Search Engine
.style = min-width: 32em;
add-engine-post-data = Post data
## The following placeholders are shown when adding a new engine.
add-engine-name-placeholder =
.placeholder = e.g., Mozilla Developer Network
add-engine-url-placeholder =
.placeholder = e.g., https://developer.mozilla.com/search?q=%s
add-engine-keyword-placeholder =
.placeholder = e.g., @mdn
## The following strings are used as error labels.
add-engine-keyword-exists = That keyword is already being used. Try a different one.
add-engine-name-exists = That name is already being used. Please choose a different one.
add-engine-no-name = Please add a name.
add-engine-no-url = Please enter a URL.
add-engine-invalid-url = That URL doesn’t look right. Please check it and try again.
add-engine-invalid-protocol = That URL doesn’t look right. Use a URL that starts with http or https.

7
browser/locales/en-US/browser/search.ftl
View File

@@ -63,13 +63,6 @@ add-engine-button = Add Custom Engine
add-engine-name = Search engine name
add-engine-keyword = Keyword
add-engine-url = Engine URL, use %s in place of the search term
add-engine-dialog =
.buttonlabelaccept = Add Engine
.buttonaccesskeyaccept = A
engine-name-exists = An engine with that name already exists
engine-keyword-exists = An engine with that keyword already exists

2
toolkit/components/search/UserSearchEngine.sys.mjs
View File

@@ -130,7 +130,7 @@ export class UserSearchEngine extends SearchEngine {
*
* @param {string} newName
* The new name.
* @returns {bool}
* @returns {boolean}
* Whether the name was changed successfully.
*/
rename(newName) {

9
toolkit/themes/osx/global/in-content/common.css
View File

@@ -47,12 +47,3 @@ html|button {
/* XUL button min-width */
min-width: 79px;
}
html|input[type="email"],
html|input[type="tel"],
html|input[type="text"],
html|input[type="password"],
html|input[type="number"],
html|textarea {
margin: 4px;
}

2
tools/lint/fluent-lint/exclusions.yml
View File

@@ -200,6 +200,8 @@ CO01:
# toolkit/neterror/certError.ftl
- cert-error-mitm-mozilla
- cert-error-mitm-connection
# browser/components/urlbar/content/enUS-searchFeatures.ftl
- add-engine-name-placeholder
files:
- browser/locales/en-US/browser/profile/default-bookmarks.ftl
- toolkit/locales/en-US/toolkit/about/aboutMozilla.ftl