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(label): fix issue where clicking on a wrapped labelable component would not update its value correctly #3161

Merged
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
19 changes: 9 additions & 10 deletions src/components/calcite-checkbox/calcite-checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
Event,
EventEmitter,
h,
Host,
Listen,
Method,
Prop,
Expand Down Expand Up @@ -145,10 +146,6 @@ export class CalciteCheckbox implements LabelableComponent {
};

private clickHandler = (): void => {
if (this.labelEl) {
return;
}

this.toggle();
};

Expand Down Expand Up @@ -257,12 +254,14 @@ export class CalciteCheckbox implements LabelableComponent {

render(): VNode {
return (
<div class={{ focused: this.focused }} onClick={this.clickHandler}>
<svg class="check-svg" viewBox="0 0 16 16">
<path d={this.getPath()} />
</svg>
<slot />
</div>
<Host onClick={this.clickHandler}>
<div class={{ focused: this.focused, test: true }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to keep the test class on this div, or was this an erroneous addition?

Copy link
Member

Choose a reason for hiding this comment

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

Whoooops 👻

<svg class="check-svg" viewBox="0 0 16 16">
<path d={this.getPath()} />
</svg>
<slot />
</div>
</Host>
);
}
}
10 changes: 7 additions & 3 deletions src/components/calcite-label/calcite-label.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,20 @@ export class CalciteLabel {
/**
* @internal
*/
@Event({ bubbles: false }) calciteInternalLabelClick: EventEmitter<void>;
@Event({ bubbles: false }) calciteInternalLabelClick: EventEmitter<{
sourceEvent: MouseEvent;
}>;

//--------------------------------------------------------------------------
//
// Private Methods
//
//--------------------------------------------------------------------------

labelClickHandler = (): void => {
this.calciteInternalLabelClick.emit();
labelClickHandler = (event: MouseEvent): void => {
this.calciteInternalLabelClick.emit({
sourceEvent: event
});
};

//--------------------------------------------------------------------------
Expand Down
24 changes: 11 additions & 13 deletions src/components/calcite-radio-button/calcite-radio-button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -182,26 +182,24 @@ export class CalciteRadioButton implements LabelableComponent {
};

private clickHandler = (): void => {
if (this.labelEl) {
return;
}

this.toggle();
};

onLabelClick = (event: CustomEvent): void => {
if (!this.disabled && !this.hidden) {
this.uncheckOtherRadioButtonsInGroup();
const label = event.currentTarget as HTMLCalciteLabelElement;
const firstButton = this.rootNode.querySelector<HTMLCalciteRadioButtonElement>(
label.for
? `calcite-radio-button[id="${label.for}"]`
: `calcite-radio-button[name="${this.name}"]`
);

if (firstButton) {
firstButton.checked = true;
firstButton.focused = true;
const radioButton = label.for
? this.rootNode.querySelector<HTMLCalciteRadioButtonElement>(
`calcite-radio-button[id="${label.for}"]`
)
: label.querySelector<HTMLCalciteRadioButtonElement>(
`calcite-radio-button[name="${this.name}"]`
);

if (radioButton) {
radioButton.checked = true;
radioButton.focused = true;
}

this.calciteRadioButtonChange.emit();
Expand Down
4 changes: 0 additions & 4 deletions src/components/calcite-switch/calcite-switch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,6 @@ export class CalciteSwitch implements LabelableComponent {
}

private clickHandler = (): void => {
if (this.labelEl) {
return;
}

this.toggle();
};

Expand Down
10 changes: 9 additions & 1 deletion src/tests/commonTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,15 @@ async function assertLabelable({
}

if (propertyToToggle) {
expect(await component.getProperty(propertyToToggle)).toBe(!initialPropertyValue);
const toggledPropertyValue = !initialPropertyValue;
expect(await component.getProperty(propertyToToggle)).toBe(toggledPropertyValue);

// assert that direct clicks on component toggle property correctly
await component.setProperty(propertyToToggle, initialPropertyValue); // we reset as not all components toggle when clicked
await page.waitForChanges();
await component.click();
await page.waitForChanges();
expect(await component.getProperty(propertyToToggle)).toBe(toggledPropertyValue);
}
}

Expand Down
31 changes: 28 additions & 3 deletions src/utils/label.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export interface LabelableComponent {

const labelTagName = "calcite-label";
const labelClickEvent = "calciteInternalLabelClick";
const onLabelClickMap = new WeakMap<HTMLElement, typeof onLabelClick>();

const findLabelForComponent = (componentEl: HTMLElement): HTMLCalciteLabelElement | null => {
const id = componentEl.id;
Expand All @@ -37,15 +38,29 @@ const findLabelForComponent = (componentEl: HTMLElement): HTMLCalciteLabelElemen
* Helper to set up label interactions on connectedCallback.
*/
export function connectLabel(component: LabelableComponent): void {
component.labelEl = findLabelForComponent(component.el);
component.labelEl?.addEventListener(labelClickEvent, component.onLabelClick);
const labelEl = findLabelForComponent(component.el);

if (!labelEl) {
return;
}

component.labelEl = labelEl;
const boundOnLabelClick = onLabelClick.bind(component);
onLabelClickMap.set(component.el, boundOnLabelClick);
component.labelEl.addEventListener(labelClickEvent, boundOnLabelClick);
}

/**
* Helper to tear down label interactions on disconnectedCallback.
*/
export function disconnectLabel(component: LabelableComponent): void {
component.labelEl?.removeEventListener(labelClickEvent, component.onLabelClick);
if (!component.labelEl) {
return;
}

const boundOnLabelClick = onLabelClickMap.get(component.el);
component.labelEl.removeEventListener(labelClickEvent, boundOnLabelClick);
onLabelClickMap.delete(component.el);
}

/**
Expand All @@ -54,3 +69,13 @@ export function disconnectLabel(component: LabelableComponent): void {
export function getLabelText(component: LabelableComponent): string {
return component.label || component.labelEl?.textContent?.trim() || "";
}

function onLabelClick(this: LabelableComponent, event: CustomEvent<{ sourceEvent: MouseEvent }>): void {
const containedLabelableChildClicked = this.el.contains(event.detail.sourceEvent.target as HTMLElement);

if (containedLabelableChildClicked) {
return;
}

this.onLabelClick(event);
}