(core) Fix a bug with editing numbers in some locales.

Summary:
Adds a new test for formatting and fix several related bugs it uncovered:
1. When editing a number with "," decimal separator, ensure it opens in
   the editor with "," (rather than ".", the original bug motivating this).
2. When guessing number format, set maxDecimals when it's needed
   (otherwise, e.g. "$1.234", or "4.5%" weren't guessed as numeric)
3. When guessing number format, ignore whitespace when deciding if
   guessed format is correct (otherwise percents can't be guessed in
   locales which add "%" with a non-breaking space before it).

Test Plan: Added a test case that exercises all fixed behaviors.

Reviewers: paulfitz

Reviewed By: paulfitz

Subscribers: paulfitz

Differential Revision: https://phab.getgrist.com/D4177
This commit is contained in:
Dmitry S 2024-01-29 22:15:34 -05:00
parent cb298e63d4
commit 93a2d26182
8 changed files with 208 additions and 13 deletions

View File

@ -0,0 +1,17 @@
import {FieldOptions} from 'app/client/widgets/NewBaseEditor';
import {NTextEditor} from 'app/client/widgets/NTextEditor';
export class NumericEditor extends NTextEditor {
constructor(protected options: FieldOptions) {
if (!options.editValue && typeof options.cellValue === 'number') {
// If opening a number for editing, we render it using the basic string representation (e.g.
// no currency symbols or groupings), but it's important to use the right locale so that the
// number can be parsed back (e.g. correct decimal separator).
const locale = options.field.documentSettings.peek().locale;
const fmt = new Intl.NumberFormat(locale, {useGrouping: false, maximumFractionDigits: 20});
const editValue = fmt.format(options.cellValue);
options = {...options, editValue};
}
super(options);
}
}

View File

@ -84,7 +84,7 @@ export const typeDefs: any = {
widgets: { widgets: {
TextBox: { TextBox: {
cons: 'NumericTextBox', cons: 'NumericTextBox',
editCons: 'TextEditor', editCons: 'NumericEditor',
icon: 'FieldTextbox', icon: 'FieldTextbox',
options: { options: {
alignment: 'right', alignment: 'right',
@ -98,7 +98,7 @@ export const typeDefs: any = {
}, },
Spinner: { Spinner: {
cons: 'Spinner', cons: 'Spinner',
editCons: 'TextEditor', editCons: 'NumericEditor',
icon: 'FieldSpinner', icon: 'FieldSpinner',
options: { options: {
alignment: 'right', alignment: 'right',

View File

@ -15,6 +15,7 @@ import {NewAbstractWidget} from 'app/client/widgets/NewAbstractWidget';
import {NewBaseEditor} from 'app/client/widgets/NewBaseEditor'; import {NewBaseEditor} from 'app/client/widgets/NewBaseEditor';
import {NTextBox} from 'app/client/widgets/NTextBox'; import {NTextBox} from 'app/client/widgets/NTextBox';
import {NTextEditor} from 'app/client/widgets/NTextEditor'; import {NTextEditor} from 'app/client/widgets/NTextEditor';
import {NumericEditor} from 'app/client/widgets/NumericEditor';
import {NumericTextBox} from 'app/client/widgets/NumericTextBox'; import {NumericTextBox} from 'app/client/widgets/NumericTextBox';
import {Reference} from 'app/client/widgets/Reference'; import {Reference} from 'app/client/widgets/Reference';
import {ReferenceEditor} from 'app/client/widgets/ReferenceEditor'; import {ReferenceEditor} from 'app/client/widgets/ReferenceEditor';
@ -32,6 +33,7 @@ export const nameToWidget = {
'TextBox': NTextBox, 'TextBox': NTextBox,
'TextEditor': NTextEditor, 'TextEditor': NTextEditor,
'NumericTextBox': NumericTextBox, 'NumericTextBox': NumericTextBox,
'NumericEditor': NumericEditor,
'HyperLinkTextBox': HyperLinkTextBox, 'HyperLinkTextBox': HyperLinkTextBox,
'HyperLinkEditor': HyperLinkEditor, 'HyperLinkEditor': HyperLinkEditor,
'Spinner': Spinner, 'Spinner': Spinner,

View File

@ -7,6 +7,7 @@
import {DocumentSettings} from 'app/common/DocumentSettings'; import {DocumentSettings} from 'app/common/DocumentSettings';
import {getDistinctValues} from 'app/common/gutil'; import {getDistinctValues} from 'app/common/gutil';
import {getCurrency, NumberFormatOptions, NumMode, parseNumMode} from 'app/common/NumberFormat'; import {getCurrency, NumberFormatOptions, NumMode, parseNumMode} from 'app/common/NumberFormat';
import {buildNumberFormat} from 'app/common/NumberFormat';
import escapeRegExp = require('lodash/escapeRegExp'); import escapeRegExp = require('lodash/escapeRegExp');
import last = require('lodash/last'); import last = require('lodash/last');
@ -72,7 +73,7 @@ export default class NumberParse {
// with corresponding digits from 0123456789. // with corresponding digits from 0123456789.
private readonly _replaceDigits: (s: string) => string; private readonly _replaceDigits: (s: string) => string;
constructor(locale: string, currency: string) { constructor(public readonly locale: string, public readonly currency: string) {
const parts = new Map<NumMode, Intl.NumberFormatPart[]>(); const parts = new Map<NumMode, Intl.NumberFormatPart[]>();
for (const numMode of NumMode.values) { for (const numMode of NumMode.values) {
const formatter = Intl.NumberFormat(locale, parseNumMode(numMode, currency)); const formatter = Intl.NumberFormat(locale, parseNumMode(numMode, currency));
@ -316,6 +317,12 @@ export default class NumberParse {
result.decimals = decimals; result.decimals = decimals;
} }
// We should only set maxDecimals if the default maxDecimals is too low.
const tmpNF = buildNumberFormat(result, {locale: this.locale, currency: this.currency}).resolvedOptions();
if (maxDecimals > tmpNF.maximumFractionDigits) {
result.maxDecimals = maxDecimals;
}
return result; return result;
} }
} }

View File

@ -69,7 +69,8 @@ abstract class ValueGuesser<T> {
const parsed = this.parse(value); const parsed = this.parse(value);
// Give up if too many strings failed to parse or if the parsed value changes when converted back to text // Give up if too many strings failed to parse or if the parsed value changes when converted back to text
if (typeof parsed === "string" && ++unparsed > maxUnparsed || formatter.formatAny(parsed) !== value) { if ((typeof parsed === "string" && ++unparsed > maxUnparsed) ||
!this.isEqualFormatted(formatter.formatAny(parsed), value)) {
return null; return null;
} }
result.push(parsed); result.push(parsed);
@ -83,6 +84,10 @@ abstract class ValueGuesser<T> {
protected allowBlank(): boolean { protected allowBlank(): boolean {
return true; return true;
} }
protected isEqualFormatted(formatted1: string, formatted2: string): boolean {
return formatted1 === formatted2;
}
} }
class BoolGuesser extends ValueGuesser<boolean> { class BoolGuesser extends ValueGuesser<boolean> {
@ -126,6 +131,14 @@ class NumericGuesser extends ValueGuesser<number> {
public parse(value: string): number | string { public parse(value: string): number | string {
return this._parser.cleanParse(value); return this._parser.cleanParse(value);
} }
protected isEqualFormatted(formatted1: string, formatted2: string): boolean {
// Consider format guessing successful if it returns the typed-in numeric value exactly or
// differing only in whitespace.
formatted1 = formatted1.replace(NumberParse.removeCharsRegex, "");
formatted2 = formatted2.replace(NumberParse.removeCharsRegex, "");
return formatted1 === formatted2;
}
} }
class DateGuesser extends ValueGuesser<number> { class DateGuesser extends ValueGuesser<number> {

View File

@ -230,7 +230,7 @@ describe("NumberParse", function() {
assert.deepEqual(parser.guessOptions(["$1"]), {numMode: "currency", decimals: 0}); assert.deepEqual(parser.guessOptions(["$1"]), {numMode: "currency", decimals: 0});
assert.deepEqual(parser.guessOptions(["$1.2"]), {numMode: "currency", decimals: 0}); assert.deepEqual(parser.guessOptions(["$1.2"]), {numMode: "currency", decimals: 0});
assert.deepEqual(parser.guessOptions(["$1.23"]), {numMode: "currency"}); assert.deepEqual(parser.guessOptions(["$1.23"]), {numMode: "currency"});
assert.deepEqual(parser.guessOptions(["$1.234"]), {numMode: "currency"}); assert.deepEqual(parser.guessOptions(["$1.234"]), {numMode: "currency", maxDecimals: 3});
// Otherwise decimal places are guessed based on trailing zeroes // Otherwise decimal places are guessed based on trailing zeroes
assert.deepEqual(parser.guessOptions(["$1.0"]), {numMode: "currency", decimals: 1}); assert.deepEqual(parser.guessOptions(["$1.0"]), {numMode: "currency", decimals: 1});

View File

@ -23,8 +23,9 @@ describe("LanguageSettings", function() {
for (const [locale, countryCode, language] of locales) { for (const [locale, countryCode, language] of locales) {
describe(`correctly detects browser language ${locale}`, () => { describe(`correctly detects browser language ${locale}`, () => {
// Change the language to the one we want to test. // Change the language to the one we want to test.
withLang(locale); const skipStatus = withLang(locale);
before(async function() { before(async function() {
if (skipStatus.skipped) { return; }
const session = await gu.session().personalSite.anon.login(); const session = await gu.session().personalSite.anon.login();
await session.loadRelPath("/"); await session.loadRelPath("/");
await gu.waitForDocMenuToLoad(); await gu.waitForDocMenuToLoad();
@ -116,34 +117,38 @@ describe("LanguageSettings", function() {
const anonym = await gu.session().personalSite.anon.login(); const anonym = await gu.session().personalSite.anon.login();
await anonym.loadRelPath("/"); await anonym.loadRelPath("/");
await gu.waitForDocMenuToLoad(); await gu.waitForDocMenuToLoad();
assert.isNull(await languageInCookie());
// Change language to french. // Change language to french.
await langButton().click(); await langButton().click();
await driver.find(".test-language-lang-fr").click(); await driver.find(".test-language-lang-fr").click();
await waitForLangButton("fr"); await waitForLangButton("fr");
assert.equal(await languageInCookie(), "fr");
// Login as user. // Login as user.
await user.login(); await user.login();
await anonym.loadRelPath("/"); await anonym.loadRelPath("/");
await gu.waitForDocMenuToLoad(); await gu.waitForDocMenuToLoad();
// But now we should have a cookie (cookie is reused).
assert.equal(await languageInCookie(), 'fr');
// Language should still be french. // Language should still be french.
await waitForHiddenButton("fr"); await waitForHiddenButton("fr");
// But now we should have a cookie (cookie is reused).
assert.equal(await languageInCookie(), 'fr');
await clearCookie(); await clearCookie();
}); });
}); });
describe("for logged in user with nb-NO", function() { describe("for logged in user with nb-NO", function() {
withLang("de"); const skipStatus = withLang("de");
let session: gu.Session; let session: gu.Session;
before(async function() { before(async function() {
if (skipStatus.skipped) { return; }
session = await gu.session().login(); session = await gu.session().login();
await session.loadRelPath("/"); await session.loadRelPath("/");
await gu.waitForDocMenuToLoad(); await gu.waitForDocMenuToLoad();
}); });
after(async function() { after(async function() {
if (skipStatus.skipped) { return; }
await clearCookie(); await clearCookie();
const api = session.createHomeApi(); const api = session.createHomeApi();
await api.updateUserLocale(null); await api.updateUserLocale(null);
@ -215,12 +220,13 @@ async function languageInCookie(): Promise<string | null> {
return cookie2.match(/grist_user_locale=([^;]+)/)?.[1] ?? null; return cookie2.match(/grist_user_locale=([^;]+)/)?.[1] ?? null;
} }
function withLang(locale: string) { function withLang(locale: string): {skipped: boolean} {
let customDriver: WebDriver; let customDriver: WebDriver;
let oldLanguage: string | undefined; let oldLanguage: string | undefined;
const skipStatus = {skipped: false};
before(async function() { before(async function() {
// On Mac we can't change the language, so skip the test. // On Mac we can't change the language (except for English), so skip the test.
if (await gu.isMac()) { return this.skip(); } if (await gu.isMac() && locale !== 'en') { skipStatus.skipped = true; return this.skip(); }
oldLanguage = process.env.LANGUAGE; oldLanguage = process.env.LANGUAGE;
// How to run chrome with a different language: // How to run chrome with a different language:
// https://developer.chrome.com/docs/extensions/reference/i18n/#how-to-set-browsers-locale // https://developer.chrome.com/docs/extensions/reference/i18n/#how-to-set-browsers-locale
@ -238,12 +244,13 @@ function withLang(locale: string) {
await gu.waitForDocMenuToLoad(); await gu.waitForDocMenuToLoad();
}); });
after(async function() { after(async function() {
if (await gu.isMac()) { return this.skip(); } if (skipStatus.skipped) { return; }
gu.setDriver(); gu.setDriver();
server.setDriver(); server.setDriver();
await customDriver.quit(); await customDriver.quit();
process.env.LANGUAGE = oldLanguage; process.env.LANGUAGE = oldLanguage;
}); });
return skipStatus;
} }
function langButton() { function langButton() {

View File

@ -0,0 +1,149 @@
import type {UserAPI} from 'app/common/UserAPI';
import * as gu from 'test/nbrowser/gristUtils';
import {setupTestSuite} from 'test/nbrowser/testUtils';
import {assert, driver, Key} from 'mocha-webdriver';
describe('NumericEditor', function() {
this.timeout(20000);
const cleanup = setupTestSuite();
afterEach(() => gu.checkForErrors());
interface Entry {
initial: string;
workaround?: () => Promise<void>;
expect: string;
exp0: string;
expPlain: string;
expFmt: string;
}
interface TestOpts {
localeCode: string;
locale: string;
entriesByColumn: Entry[];
valPlain: string;
valFmt: string;
}
describe('locale en-US', testSuite({
localeCode: 'en-US',
locale: 'United States (English)',
entriesByColumn: [
{initial: '17', expect: '17', exp0: '0', expPlain: '-4.4', expFmt: '-1234.56' },
{initial: '17.500', expect: '17.500', exp0: '0.000', expPlain: '-4.400', expFmt: '-1234.560' },
{initial: '(1.2)', expect: '(1.2)', exp0: ' 0 ', expPlain: '(4.4)', expFmt: '(1234.56)' },
{initial: '1000.456', expect: '1000.456', exp0: '0', expPlain: '-4.4', expFmt: '-1234.56' },
{initial: '1,000.0', expect: '1,000.0', exp0: '0.0', expPlain: '-4.4', expFmt: '-1,234.56' },
{initial: '$5.00', expect: '$5.00', exp0: '$0.00', expPlain: '-$4.40', expFmt: '-$1,234.56'},
{initial: '4.5%', expect: '4.5%', exp0: '0%', expPlain: '-440%', expFmt: '-123,456%' },
],
valPlain: '-4.4',
valFmt: '(1,234.56)',
}));
describe('locale de-DE', testSuite({
localeCode: 'de-DE',
locale: 'Germany (German)',
entriesByColumn: [
{initial: '17', expect: '17', exp0: '0', expPlain: '-4,4', expFmt: '-1234,56' },
{initial: '17,500', expect: '17,500', exp0: '0,000', expPlain: '-4,400', expFmt: '-1234,560' },
{initial: '(1,2)', expect: '(1,2)', exp0: ' 0 ', expPlain: '(4,4)', expFmt: '(1234,56)' },
{initial: '1000,456', expect: '1000,456', exp0: '0', expPlain: '-4,4', expFmt: '-1234,56' },
{initial: '1.000,0', expect: '1.000,0', exp0: '0,0', expPlain: '-4,4', expFmt: '-1.234,56', },
{initial: '5,00€', expect: '5,00 €', exp0: '0,00 €', expPlain: '-4,40 €', expFmt: '-1.234,56 €' },
{initial: '4,5%', expect: '4,5 %', exp0: '0 %', expPlain: '-440 %', expFmt: '-123.456 %' },
],
valPlain: '-4,4',
valFmt: '(1.234,56)',
}));
function testSuite(options: TestOpts) {
const {entriesByColumn} = options;
return function() {
let docId: string;
let api: UserAPI;
let MODKEY: string;
before(async function() {
MODKEY = await gu.modKey();
const session = await gu.session().login();
docId = await session.tempNewDoc(cleanup, `NumericEditor-${options.localeCode}`);
api = session.createHomeApi();
await api.applyUserActions(docId, [
// Make sure there are as many columns as entriesByColumn.
...entriesByColumn.slice(3).map(() => ['AddVisibleColumn', 'Table1', '', {}]),
]);
// Set locale for the document.
await gu.openDocumentSettings();
await driver.findWait('.test-locale-autocomplete', 500).click();
await driver.sendKeys(options.locale, Key.ENTER);
await gu.waitForServer();
assert.equal(await driver.find('.test-locale-autocomplete input').value(), options.locale);
await gu.openPage('Table1');
});
beforeEach(async function() {
// Scroll grid to the left before each test case.
await gu.sendKeys(Key.HOME);
});
it('should create Numeric columns with suitable format', async function() {
// Entering a value into an empty column should switch it to a suitably formatted Numeric.
for (const [i, entry] of Object.entries(entriesByColumn)) {
await gu.getCell({rowNum: 1, col: Number(i)}).click();
await entry.workaround?.();
await gu.enterCell(entry.initial);
assert.equal(await gu.getCell({rowNum: 1, col: Number(i)}).getText(), entry.expect);
}
// Add a new row, which should be filled with 0's. Check what formatted 0's look like.
await gu.sendKeys(Key.chord(MODKEY, Key.ENTER));
await gu.waitForServer();
// Check what 0's look like.
for (const [i, entry] of Object.entries(entriesByColumn)) {
assert.equal(await gu.getCell({rowNum: 2, col: Number(i)}).getText(), entry.exp0);
}
});
it('should support entering plain numbers into a formatted column', async function() {
const rowNum = 3;
const cols = entriesByColumn.map((_, i) => i);
await gu.enterGridRows({rowNum, col: 0},
[ entriesByColumn.map(() => options.valPlain) ]);
assert.deepEqual(await gu.getVisibleGridCells({rowNums: [rowNum], cols}),
entriesByColumn.map((entry) => entry.expPlain));
});
it('should support entering formatted numbers into a formatted column', async function() {
const rowNum = 4;
const cols = entriesByColumn.map((_, i) => i);
await gu.enterGridRows({rowNum, col: 0},
[ entriesByColumn.map(() => options.valFmt) ]);
assert.deepEqual(await gu.getVisibleGridCells({rowNums: [rowNum], cols}),
entriesByColumn.map((entry) => entry.expFmt));
});
it('should allow editing and saving a formatted value', async function() {
for (const [i, entry] of Object.entries(entriesByColumn)) {
await gu.getCell({rowNum: 1, col: Number(i)}).click();
await gu.sendKeys(Key.ENTER);
await gu.waitAppFocus(false);
// Save the value the way it's opened in the editor. It's important that it doesn't
// change interpretation (there was a bug related to this, when a value with "," decimal
// separator would open with a "." decimal separator, and get saved back incorrectly).
await gu.sendKeys(Key.ENTER);
await gu.waitForServer();
await gu.waitAppFocus(true);
assert.equal(await gu.getCell({rowNum: 1, col: Number(i)}).getText(), entry.expect);
}
});
};
}
});