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: ensure beforeOpen/open and beforeClose/close events emit properly #9958

Merged
Show file tree
Hide file tree
Changes from 13 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
Expand Up @@ -38,7 +38,6 @@ $border-style: 1px solid var(--calcite-color-border-3);
inline-size: var(--calcite-alert-width);
max-inline-size: calc(100% - (var(--calcite-alert-edge-distance) * 2));
transition:
var(--calcite-internal-animation-timing-slow) $easing-function,
opacity var(--calcite-internal-animation-timing-slow) $easing-function,
all var(--calcite-animation-timing) ease-in-out;

Expand Down
3 changes: 3 additions & 0 deletions packages/calcite-components/src/components/block/block.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from "../../tests/commonTests";
import { html } from "../../../support/formatting";
import { openClose } from "../../tests/commonTests";
import { skipAnimations } from "../../tests/utils";
import { CSS, SLOTS } from "./resources";

describe("calcite-block", () => {
Expand Down Expand Up @@ -189,6 +190,7 @@ describe("calcite-block", () => {
const heading = "heading";
const page = await newE2EPage();
await page.setContent(html`<calcite-block collapsible heading=${heading}></calcite-block>`);
await skipAnimations(page);
const messages = await import(`./assets/block/t9n/messages.json`);

const element = await page.find("calcite-block");
Expand Down Expand Up @@ -251,6 +253,7 @@ describe("calcite-block", () => {
<div class="nested-control" tabindex="0" slot=${SLOTS.control}>fake space/enter-bubbling control</div>
</calcite-block>
`);
await skipAnimations(page);
const control = await page.find(".nested-control");
expect(await control.isVisible()).toBe(true);

Expand Down
3 changes: 2 additions & 1 deletion packages/calcite-components/src/components/block/block.scss
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
@extend %component-host;
@extend %component-spacing;
@apply transition-margin ease-cubic border-color-3 flex flex-shrink-0 flex-grow-0
flex-col border-0 border-b border-solid p-0 duration-150;
flex-col border-0 border-b border-solid p-0;
flex-basis: auto;
transition-duration: var(--calcite-animation-timing);
}

@include disabled();
Expand Down
9 changes: 3 additions & 6 deletions packages/calcite-components/src/components/block/block.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ export class Block

@State() hasEndActions = false;

openTransitionProp = "opacity";
openTransitionProp = "margin-top";

transitionEl: HTMLElement;

Expand All @@ -239,6 +239,8 @@ export class Block
connectInteractive(this);
connectLocalized(this);
connectMessages(this);

this.transitionEl = this.el;
}

disconnectedCallback(): void {
Expand Down Expand Up @@ -301,10 +303,6 @@ export class Block
this.calciteBlockToggle.emit();
};

private setTransitionEl = (el: HTMLElement): void => {
this.transitionEl = el;
};

private actionsEndSlotChangeHandler = (event: Event): void => {
this.hasEndActions = slotChangeHasAssignedElement(event);
};
Expand Down Expand Up @@ -490,7 +488,6 @@ export class Block
class={CSS.content}
hidden={!open}
id={IDS.content}
ref={this.setTransitionEl}
>
{this.renderScrim()}
</section>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -859,8 +859,8 @@ describe("calcite-input-time-picker", () => {
html`<calcite-input-time-picker></calcite-input-time-picker>
<div id="next-sibling" tabindex="0">next sibling</div>`,
);
await skipAnimations(page);
const popover = await page.find("calcite-input-time-picker >>> calcite-popover");
const stopgapDelayUntilOpenCloseEventsAreImplemented = 500;

await page.keyboard.press("Tab");
expect(await getFocusedElementProp(page, "tagName")).toBe("CALCITE-INPUT-TIME-PICKER");
Expand All @@ -876,7 +876,6 @@ describe("calcite-input-time-picker", () => {

await page.keyboard.press("ArrowDown");
await page.waitForChanges();
await page.waitForTimeout(stopgapDelayUntilOpenCloseEventsAreImplemented);

expect(await popover.isVisible()).toBe(true);
expect(await getFocusedElementProp(page, "tagName", { shadow: true })).toBe("CALCITE-TIME-PICKER");
Expand All @@ -891,7 +890,6 @@ describe("calcite-input-time-picker", () => {

await page.keyboard.press("Escape");
await page.waitForChanges();
await page.waitForTimeout(stopgapDelayUntilOpenCloseEventsAreImplemented);

expect(await popover.isVisible()).toBe(false);
expect(await getFocusedElementProp(page, "tagName")).toBe("CALCITE-INPUT-TIME-PICKER");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ import {
updateMessages,
} from "../../utils/t9n";
import { getSupportedLocale } from "../../utils/locale";
import { onToggleOpenCloseComponent, OpenCloseComponent } from "../../utils/openCloseComponent";
import { decimalPlaces } from "../../utils/math";
import { getIconScale } from "../../utils/component";
import { Validation } from "../functional/Validation";
Expand Down Expand Up @@ -169,7 +168,6 @@ export class InputTimePicker
LabelableComponent,
LoadableComponent,
LocalizedComponent,
OpenCloseComponent,
T9nComponent
{
//--------------------------------------------------------------------------
Expand All @@ -184,14 +182,13 @@ export class InputTimePicker

@Watch("open")
openHandler(): void {
onToggleOpenCloseComponent(this);

if (this.disabled || this.readOnly) {
this.open = false;
return;
}

this.reposition(true);
// we set the property instead of the attribute to ensure popover's open/close events are emitted properly
this.popoverEl.open = this.open;
driskull marked this conversation as resolved.
Show resolved Hide resolved
}

/** When `true`, interaction is prevented and the component is displayed with lower opacity. */
Expand Down Expand Up @@ -395,10 +392,6 @@ export class InputTimePicker

private referenceElementId = `input-time-picker-${guid()}`;

openTransitionProp = "opacity";

transitionEl: HTMLCalciteInputElement;

//--------------------------------------------------------------------------
//
// State
Expand Down Expand Up @@ -559,21 +552,42 @@ export class InputTimePicker
//
// --------------------------------------------------------------------------

onBeforeOpen(): void {
private popoverBeforeOpenHandler = (event: CustomEvent<void>): void => {
event.stopPropagation();
this.calciteInputTimePickerBeforeOpen.emit();
}
};

onOpen(): void {
private popoverOpenHandler = (event: CustomEvent<void>): void => {
event.stopPropagation();
this.calciteInputTimePickerOpen.emit();
}

onBeforeClose(): void {
activateFocusTrap(this, {
onActivate: () => {
if (this.focusOnOpen) {
this.calciteTimePickerEl.setFocus();
this.focusOnOpen = false;
}
},
});
};

private popoverBeforeCloseHandler = (event: CustomEvent<void>): void => {
event.stopPropagation();
this.calciteInputTimePickerBeforeClose.emit();
}
};

onClose(): void {
private popoverCloseHandler = (event: CustomEvent<void>): void => {
event.stopPropagation();
this.calciteInputTimePickerClose.emit();
}

deactivateFocusTrap(this, {
onDeactivate: () => {
this.calciteInputEl.setFocus();
this.focusOnOpen = false;
},
});
this.open = false;
};

syncHiddenFormInput(input: HTMLInputElement): void {
syncHiddenFormInput("time", this, input);
Expand Down Expand Up @@ -685,27 +699,6 @@ export class InputTimePicker
return timeString;
}

private popoverCloseHandler = () => {
deactivateFocusTrap(this, {
onDeactivate: () => {
this.calciteInputEl.setFocus();
this.focusOnOpen = false;
},
});
this.open = false;
};

private popoverOpenHandler = () => {
activateFocusTrap(this, {
onActivate: () => {
if (this.focusOnOpen) {
this.calciteTimePickerEl.setFocus();
this.focusOnOpen = false;
}
},
});
};

keyDownHandler = (event: KeyboardEvent): void => {
const { defaultPrevented, key } = event;

Expand Down Expand Up @@ -867,9 +860,8 @@ export class InputTimePicker
this.popoverEl = el;
};

private setInputAndTransitionEl = (el: HTMLCalciteInputElement): void => {
private setInputEl = (el: HTMLCalciteInputElement): void => {
this.calciteInputEl = el;
this.transitionEl = el;
};

private setCalciteTimePickerEl = (el: HTMLCalciteTimePickerElement): void => {
Expand Down Expand Up @@ -978,9 +970,6 @@ export class InputTimePicker
async componentWillLoad(): Promise<void> {
setUpLoadableComponent(this);
await Promise.all([setUpMessages(this), this.loadDateTimeLocaleData()]);
if (this.open) {
onToggleOpenCloseComponent(this);
Copy link
Member

Choose a reason for hiding this comment

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

With this removed, can the popover be open by default?

}
}

componentDidLoad() {
Expand Down Expand Up @@ -1034,7 +1023,7 @@ export class InputTimePicker
onCalciteInputTextInput={this.calciteInternalInputInputHandler}
onCalciteInternalInputTextFocus={this.calciteInternalInputFocusHandler}
readOnly={readOnly}
ref={this.setInputAndTransitionEl}
ref={this.setInputEl}
role="combobox"
scale={this.scale}
status={this.status}
Expand All @@ -1046,9 +1035,10 @@ export class InputTimePicker
id={dialogId}
label={messages.chooseTime}
lang={this.effectiveLocale}
onCalcitePopoverBeforeClose={this.popoverBeforeCloseHandler}
onCalcitePopoverBeforeOpen={this.popoverBeforeOpenHandler}
onCalcitePopoverClose={this.popoverCloseHandler}
onCalcitePopoverOpen={this.popoverOpenHandler}
open={this.open}
Copy link
Member

Choose a reason for hiding this comment

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

With this removed, can the popover be open by default?

overlayPositioning={this.overlayPositioning}
placement={this.placement}
ref={this.setCalcitePopoverEl}
Expand Down
11 changes: 5 additions & 6 deletions packages/calcite-components/src/components/notice/notice.scss
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,14 @@
pointer-events-none
my-0
box-border
hidden
flex
w-full
opacity-0
transition-default;
opacity-0;
max-block-size: 0;
transition-property: opacity, max-block-size;
transition-duration: var(--calcite-animation-timing);
text-align: start;
border-inline-start: 0 solid;
border-inline-start: var(--calcite-border-width-md) solid;
box-shadow: 0 0 0 0 transparent;
}

Expand All @@ -100,10 +101,8 @@
:host([open]) .container {
@apply shadow-1
pointer-events-auto
flex
max-h-full
items-center
border-2
opacity-100;
}

Expand Down
14 changes: 13 additions & 1 deletion packages/calcite-components/src/components/sheet/sheet.e2e.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { newE2EPage } from "@stencil/core/testing";
import { html } from "../../../support/formatting";
import { focusable, renders, hidden, defaults, accessible } from "../../tests/commonTests";
import { accessible, defaults, focusable, hidden, openClose, renders } from "../../tests/commonTests";
import { GlobalTestProps, newProgrammaticE2EPage, skipAnimations } from "../../tests/utils";
import { CSS } from "./resources";

Expand Down Expand Up @@ -79,6 +79,18 @@ describe("calcite-sheet properties", () => {
});
});

describe("openClose", () => {
describe("default", () => {
openClose("calcite-sheet");
});

describe("initially open", () => {
openClose("calcite-sheet", {
initialToggleValue: true,
});
});
});

it("sets custom width correctly", async () => {
const page = await newE2EPage();
// set large page to ensure test sheet isn't becoming fullscreen
Expand Down
8 changes: 6 additions & 2 deletions packages/calcite-components/src/components/sheet/sheet.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,14 @@ export class Sheet implements OpenCloseComponent, FocusTrapComponent, LoadableCo
[CSS.containerEmbedded]: this.embedded,
[CSS_UTILITY.rtl]: dir === "rtl",
}}
ref={this.setTransitionEl}
>
<calcite-scrim class={CSS.scrim} onClick={this.handleOutsideClose} />
<div
class={{
[CSS.content]: true,
}}
ref={this.setTransitionEl}
ref={this.setContentId}
>
<slot />
</div>
Expand Down Expand Up @@ -297,9 +298,12 @@ export class Sheet implements OpenCloseComponent, FocusTrapComponent, LoadableCo
deactivateFocusTrap(this);
}

private setContentId = (el: HTMLDivElement): void => {
this.contentId = ensureId(el);
};

private setTransitionEl = (el: HTMLDivElement): void => {
this.transitionEl = el;
this.contentId = ensureId(el);
};

private openEnd = (): void => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,10 @@ describe("calcite-tooltip", () => {

describe("openClose", () => {
openClose(simpleTooltipHtml);
openClose(tooltipDisplayNoneHtml);

describe("parent has display none", () => {
openClose(tooltipDisplayNoneHtml, { willUseFallback: true });
});
});

it("should have zIndex of 901", async () => {
Expand Down
Loading
Loading