Skip to content

fix(input, input-number): allows numeric characters. #7213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 26, 2023
Merged

fix(input, input-number): allows numeric characters. #7213

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b4cc71c
fix(input,input-number): supports French AZERTY keyboard layout
anveshmekala Jun 22, 2023
fbc5210
fix tests
anveshmekala Jun 22, 2023
0eb2956
remove unused imports
anveshmekala Jun 22, 2023
5cb15ec
Merge branch 'master' into anveshmekala/6854-input-allow-AZERTY-keybo…
anveshmekala Jun 22, 2023
ec55735
remove extra space in test header
anveshmekala Jun 22, 2023
d51b3ad
add tests
anveshmekala Jun 23, 2023
f6ef851
feedback changes
anveshmekala Jun 23, 2023
8136b4a
remove newline spaces
anveshmekala Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions packages/calcite-components/src/components/input-number/input-number.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,20 +899,12 @@ describe("calcite-input-number", () => {
expect(Number(await element.getProperty("value"))).toBe(195);
});

it("disallows typing number with shift modifier key down", async () => {
it("disallows typing characters with shift modifier key down", async () => {
const page = await newE2EPage();
await page.setContent(html`<calcite-input-number></calcite-input-number>`);
const calciteInput = await page.find("calcite-input-number");
const input = await page.find("calcite-input-number >>> input");
await calciteInput.callMethod("setFocus");

for (let i = 0; i < numberKeys.length; i++) {
await page.keyboard.down("Shift");
await page.keyboard.press(numberKeys[i] as KeyInput);
await page.keyboard.up("Shift");
expect(await calciteInput.getProperty("value")).toBeFalsy();
expect(await input.getProperty("value")).toBeFalsy();
}
const nonELetterKeys = letterKeys.filter((key) => key !== "e");
for (let i = 0; i < nonELetterKeys.length; i++) {
await page.keyboard.down("Shift");
Expand All @@ -923,6 +915,26 @@ describe("calcite-input-number", () => {
}
});

// refer to issue here https://github.com/Esri/calcite-components/issues/6854
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the issue number in the test name instead? Similar to https://github.com/Esri/calcite-components/blob/14036c1354419222bc5b839af8b68675b17ebf62/packages/calcite-components/src/components/tree/tree.e2e.ts#L962.

it("allows typing numbers with shift modifier key down", async () => {
const page = await newE2EPage();
await page.setContent(html`<calcite-input type="number"></calcite-input>`);
const calciteInput = await page.find("calcite-input");
const input = await page.find("calcite-input >>> input");
await calciteInput.callMethod("setFocus");
const numberKeysExcludingZero = numberKeys.slice(1);

let result = "";
for (let i = 0; i < numberKeysExcludingZero.length; i++) {
await page.keyboard.down("Shift");
await page.keyboard.press(numberKeysExcludingZero[i] as KeyInput);
result += numberKeysExcludingZero[i];
await page.keyboard.up("Shift");
expect(await calciteInput.getProperty("value")).toBe(result);
expect(await input.getProperty("value")).toBe(result);
}
});

it("allows shift tabbing", async () => {
const page = await newE2EPage();
await page.setContent(html`
Expand Down
5 changes: 5 additions & 0 deletions packages/calcite-components/src/components/input-number/input-number.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,11 @@ export class InputNumber
return;
}

// identifies French AZERTY keyboard input
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you generalize this comment? I think this would apply to other keyboard layouts. Bonus points if you use a self-documenting const for this. ⭐

Actually, do we need to care about the Shift key being pressed? Wouldn't we want to process any number from KeyboardEvent.key? I tracked that part of logic down to #2127. @eriklharper Do you recall why we bail when the Shift key is pressed? This could possibly extend to the Alt, Ctrl and Meta keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for verifying Shift key here is to make sure the numbers are being returned. In AZERTY keyboard , users need to keep shift key activated for numbers to be printed.

Ideally we would like to process any number but as you mentioned there is a custom logic that prevents numberKeys, ArrowKeys and Meta keys with Tab+Shift which results in the bug #6854. I wonder if there will be a case where the event.key will return a numberKey when Shift+Tab or Shift key is activated.

Either we need to remove numberKeys from supportedKeys array or remove the check for shiftKey. With input and input-number , i am very hesitant to remove any existing code since it is very impractical to iterate tests for every single locale we support.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for verifying Shift key here is to make sure the numbers are being returned. In AZERTY keyboard , users need to keep shift key activated for numbers to be printed.

I wonder if there will be a case where the event.key will return a numberKey when Shift+Tab or Shift key is activated.

This is what I'm saying. In AZERTY, when the key combination for a number is pressed, you get a number in KeyboardEvent.key.

or remove the check for shiftKey.

Unless I'm missing something, I think this is the way to go (Shift + Tab being the exception).

if (numberKeys.includes(event.key) && event.shiftKey) {
return;
}

numberStringFormatter.numberFormatOptions = {
locale: this.effectiveLocale,
numberingSystem: this.numberingSystem,
Expand Down
30 changes: 21 additions & 9 deletions packages/calcite-components/src/components/input/input.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,20 +1055,12 @@ describe("calcite-input", () => {
expect(Number(await element.getProperty("value"))).toBe(195);
});

it("disallows typing any letter or number with shift modifier key down", async () => {
it("disallows typing any characters with shift modifier key down", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would non-numeric characters be more precise for this?

Applies to other test names.

const page = await newE2EPage();
await page.setContent(html`<calcite-input type="number"></calcite-input>`);
const calciteInput = await page.find("calcite-input");
const input = await page.find("calcite-input >>> input");
await calciteInput.callMethod("setFocus");

for (let i = 0; i < numberKeys.length; i++) {
await page.keyboard.down("Shift");
await page.keyboard.press(numberKeys[i] as KeyInput);
await page.keyboard.up("Shift");
expect(await calciteInput.getProperty("value")).toBeFalsy();
expect(await input.getProperty("value")).toBeFalsy();
}
const nonELetterKeys = letterKeys.filter((key) => key !== "e");
for (let i = 0; i < nonELetterKeys.length; i++) {
await page.keyboard.down("Shift");
Expand All @@ -1079,6 +1071,26 @@ describe("calcite-input", () => {
}
});

// refer to issue here https://github.com/Esri/calcite-components/issues/6854
it("allows typing numbers with shift modifier key down", async () => {
const page = await newE2EPage();
await page.setContent(html`<calcite-input type="number"></calcite-input>`);
const calciteInput = await page.find("calcite-input");
const input = await page.find("calcite-input >>> input");
await calciteInput.callMethod("setFocus");
const numberKeysExcludingZero = numberKeys.slice(1);

let result = "";
for (let i = 0; i < numberKeysExcludingZero.length; i++) {
await page.keyboard.down("Shift");
await page.keyboard.press(numberKeysExcludingZero[i] as KeyInput);
result += numberKeysExcludingZero[i];
await page.keyboard.up("Shift");
expect(await calciteInput.getProperty("value")).toBe(result);
expect(await input.getProperty("value")).toBe(result);
}
});

it("allows shift tabbing", async () => {
const page = await newE2EPage();
await page.setContent(html`
Expand Down
6 changes: 6 additions & 0 deletions packages/calcite-components/src/components/input/input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,12 @@ export class Input
}
return;
}

// identifies French AZERTY keyboard input
if (numberKeys.includes(event.key) && event.shiftKey) {
return;
}

numberStringFormatter.numberFormatOptions = {
locale: this.effectiveLocale,
numberingSystem: this.numberingSystem,
Expand Down