Skip to content
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.

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

fix(foundation): checkbox remove read-only support #6515

Merged
merged 7 commits into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "checkbox remove irrelevant read-only property",
"packageName": "@microsoft/fast-foundation",
"email": "[email protected]",
"dependentChangeType": "patch"
}
3 changes: 0 additions & 3 deletions packages/web-components/fast-foundation/docs/api-report.md
Original file line number Diff line number Diff line change
Expand Up @@ -883,9 +883,6 @@ export class FASTCheckbox extends FormAssociatedCheckbox {
initialValue: string;
// @internal (undocumented)
keypressHandler: (e: KeyboardEvent) => void;
readOnly: boolean;
// (undocumented)
protected readOnlyChanged(): void;
}

// Warning: (ae-different-release-tags) This symbol has another declaration with a different release tag
Expand Down
21 changes: 4 additions & 17 deletions packages/web-components/fast-foundation/src/checkbox/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,30 +110,17 @@ export const myCheckbox = Checkbox.compose<CheckboxOptions>({

#### Fields

| Name | Privacy | Type | Default | Description | Inherited From |
| --------------- | ------- | --------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------- |
| `readOnly` | public | `boolean` | | When true, the control will be immutable by user interaction. See [readonly HTML attribute](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly) for more information. | |
| `indeterminate` | public | `boolean` | `false` | The indeterminate state of the control | |
| `proxy` | | | | | FormAssociatedCheckbox |

#### Methods

| Name | Privacy | Description | Parameters | Return | Inherited From |
| ----------------- | --------- | ----------- | ---------- | ------ | -------------- |
| `readOnlyChanged` | protected | | | `void` | |
| Name | Privacy | Type | Default | Description | Inherited From |
| --------------- | ------- | --------- | ------- | -------------------------------------- | ---------------------- |
| `indeterminate` | public | `boolean` | `false` | The indeterminate state of the control | |
| `proxy` | | | | | FormAssociatedCheckbox |

#### Events

| Name | Type | Description | Inherited From |
| -------- | ---- | ---------------------------------------------------------- | -------------- |
| `change` | | Emits a custom change event when the checked state changes | |

#### Attributes

| Name | Field | Inherited From |
| ---------- | -------- | -------------- |
| `readonly` | readOnly | |

#### CSS Parts

| Name | Description |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,38 +150,6 @@ test.describe("Checkbox", () => {
await expect(element).not.toHaveAttribute("tabindex", "0");
});

test("should NOT set a default `aria-readonly` value when `readonly` is not defined", async () => {
await root.evaluate(node => {
node.innerHTML = /* html */ `
<fast-checkbox></fast-checkbox>
`;
});

await expect(element).not.toHaveBooleanAttribute("readonly");

await expect(element).not.hasAttribute("aria-readonly");
});

test("should set the `aria-readonly` attribute equal to the `readonly` property", async () => {
await root.evaluate(node => {
node.innerHTML = /* html */ `
<fast-checkbox></fast-checkbox>
`;
});

await element.evaluate((node: FASTCheckbox) => {
node.readOnly = true;
});

await expect(element).toHaveAttribute("aria-readonly", "true");

await element.evaluate((node: FASTCheckbox) => {
node.readOnly = false;
});

await expect(element).toHaveAttribute("aria-readonly", "false");
});

test("should set the aria-checked value to 'mixed' when indeterminate property is true", async () => {
await root.evaluate(node => {
node.innerHTML = /* html */ `
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ Extends [form associated custom element](../form-associated/form-associated-cust
- The indeterminate state. Independent of checked

*Content attributes*
- `readonly`
- The checkbox should be submitted with the form but should not be editable.
- `value`
- Defaults to "on" to match `input[type="checkbox"]`
- `checked`
Expand Down Expand Up @@ -70,7 +68,6 @@ Extends [form associated custom element](../form-associated/form-associated-cust
- checked
- disabled
- required
- readonly

*Slotted Content/Slotted Classes*
*CSS Parts*
Expand All @@ -90,9 +87,6 @@ The checked state can be toggled by:
**disabled**: `true` or `false`
When disabled, the value will not be changeable through user interaction. It should also not expose it's value to a form submission.

**readonly**: `true` or `false`
When readonly, the value will not be changeable through user interaction. The value will still be exposed to forms on submission.

### Accessibility
The root element inside the shadow-dom of the checkbox will be a focusable element with the following accessability content attributes:
- role: checkbox
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export function checkboxTemplate<T extends FASTCheckbox>(
aria-checked="${x => (x.indeterminate ? "mixed" : x.checked)}"
aria-required="${x => x.required}"
aria-disabled="${x => x.disabled}"
aria-readonly="${x => x.readOnly}"
tabindex="${x => (x.disabled ? null : 0)}"
@keypress="${(x, c) => x.keypressHandler(c.event as KeyboardEvent)}"
@click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
Expand Down
35 changes: 11 additions & 24 deletions packages/web-components/fast-foundation/src/checkbox/checkbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,6 @@ export type CheckboxOptions = {
* @public
*/
export class FASTCheckbox extends FormAssociatedCheckbox {
/**
* When true, the control will be immutable by user interaction. See {@link https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/readonly | readonly HTML attribute} for more information.
* @public
* @remarks
* HTML Attribute: readonly
*/
@attr({ attribute: "readonly", mode: "boolean" })
public readOnly: boolean; // Map to proxy element
protected readOnlyChanged(): void {
if (this.proxy instanceof HTMLInputElement) {
this.proxy.readOnly = this.readOnly;
}
}

/**
* The element's value to be included in form submission when checked.
* Default to "on" to reach parity with input[type="checkbox"]
Expand All @@ -65,20 +51,24 @@ export class FASTCheckbox extends FormAssociatedCheckbox {
this.proxy.setAttribute("type", "checkbox");
}

private toggleChecked() {
if (this.indeterminate) {
this.indeterminate = false;
}
this.checked = !this.checked;
}

/**
* @internal
*/
public keypressHandler = (e: KeyboardEvent): void => {
if (this.readOnly) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be updated to be if (this.disabled) in order to match the clickHandler?

Copy link
Member

Choose a reason for hiding this comment

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

Opening this back up as I do think that this is a bug if the event is emitted even when disabled here...likely should be a noop...should we handle here or in a new fix? Considering we've already condensed another existing method as part of this, while I prefer to not batch commits, this may be a reasonable addition. Thoughts @yinonov?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course! done

if (this.disabled) {
return;
}

switch (e.key) {
case keySpace:
if (this.indeterminate) {
this.indeterminate = false;
}
this.checked = !this.checked;
this.toggleChecked();
break;
}
};
Expand All @@ -87,11 +77,8 @@ export class FASTCheckbox extends FormAssociatedCheckbox {
* @internal
*/
public clickHandler = (e: MouseEvent): void => {
if (!this.disabled && !this.readOnly) {
if (this.indeterminate) {
this.indeterminate = false;
}
this.checked = !this.checked;
if (!this.disabled) {
yinonov marked this conversation as resolved.
Show resolved Hide resolved
this.toggleChecked();
}
};
}