fix(popover): correct left/right tips for RTL, increase VRT coverage (#…

…2753)
adobe · May 22, 2024 · 54faea2 · 54faea2
1 parent efe0415
commit 54faea2
Show file tree

Hide file tree

Showing 4 changed files with 122 additions and 32 deletions.
diff --git a/.changeset/three-geese-relax.md b/.changeset/three-geese-relax.md
@@ -0,0 +1,7 @@
+---
+"@spectrum-css/popover": patch
+---
+
+fix(popover): correct left/right tips for RTL
+
+Includes exposing Start and End tip placement variants in Storybook, and increased VRT coverage for Chromatic.
diff --git a/components/popover/index.css b/components/popover/index.css
@@ -150,7 +150,8 @@ governing permissions and limitations under the License.
 	/* spacing to include tip in calculation of offset from source */
 	&.spectrum-Popover--withTip {
 		/* tip size minus where it overlaps with popover border */
-		margin-inline-start: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width)));
+		/* stylelint-disable-next-line csstools/use-logical -- intentional, right stays on the same side even for RTL */
+		margin-left: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width)));
 	}
 
 	/* popover animates towards right ⮕ */
@@ -166,7 +167,8 @@ governing permissions and limitations under the License.
 	/* spacing to include tip in calculation of offset from source */
 	&.spectrum-Popover--withTip {
 		/* tip size minus where it overlaps with popover border */
-		margin-inline-end: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width)));
+		/* stylelint-disable-next-line csstools/use-logical -- intentional, left stays on the same side even for RTL */
+		margin-right: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width)));
 	}
 
 	/* popover animates towards left ⬅ */
@@ -342,7 +344,9 @@ governing permissions and limitations under the License.
 	&.spectrum-Popover--left-bottom,
 	&.spectrum-Popover--left-top {
 		.spectrum-Popover-tip {
-			inset-inline: 100% auto;
+			/* stylelint-disable-next-line csstools/use-logical -- intentional, left stays on the same side even for RTL */
+			left: 100%;
+			right: auto;
 		}
 	}
 
@@ -351,7 +355,9 @@ governing permissions and limitations under the License.
 	&.spectrum-Popover--right-bottom,
 	&.spectrum-Popover--right-top {
 		.spectrum-Popover-tip {
-			inset-inline: auto 100%;
+			/* stylelint-disable-next-line csstools/use-logical -- intentional, right stays on the same side even for RTL */
+			right: 100%;
+			left: auto;
 
 			/* flip tip to point left ◁ */
 			transform: scaleX(-1);

diff --git a/components/popover/stories/popover.stories.js b/components/popover/stories/popover.stories.js
@@ -1,11 +1,38 @@
+import { Template as Typography } from "@spectrum-css/typography/stories/template.js";
 import { userEvent, within } from "@storybook/testing-library";
 import { html } from "lit";
+import { when } from "lit/directives/when.js";
 
 import { Template } from "./template";
 
 import { Template as ActionButton } from "@spectrum-css/actionbutton/stories/template.js";
 import { Template as Menu } from "@spectrum-css/menu/stories/template.js";
 
+const placementOptions = [
+	"top",
+	"top-left",
+	"top-right",
+	"top-start",
+	"top-end",
+	"bottom",
+	"bottom-left",
+	"bottom-right",
+	"bottom-start",
+	"bottom-end",
+	"right",
+	"right-bottom",
+	"right-top",
+	"left",
+	"left-bottom",
+	"left-top",
+	"start",
+	"start-top",
+	"start-bottom",
+	"end",
+	"end-top",
+	"end-bottom",
+];
+
 /**
  * A popover is used to display transient content (menus, options, additional actions etc.) and appears when clicking/tapping on a source (tools, buttons, etc.). It stands out via its visual style (stroke and drop shadow) and floats on top of the rest of the interface.
  */
@@ -44,20 +71,7 @@ export default {
 				category: "Component",
 			},
 			control: "select",
-			options: [
-				"top",
-				"top-start",
-				"top-end",
-				"bottom",
-				"bottom-start",
-				"bottom-end",
-				"left",
-				"left-top",
-				"left-bottom",
-				"right",
-				"right-top",
-				"right-bottom",
-			],
+			options: placementOptions,
 			if: { arg: "nested", truthy: false },
 		},
 	},
@@ -113,21 +127,72 @@ export default {
 		},
 	},
 	decorators: [
-		(Story, context) => html`<div style="padding: 16px">${Story(context)}</div>`
+		(Story, context) => html`
+			<style>
+				.spectrum-Detail { display: inline-block; }
+				.spectrum-Typography > div {
+					/* Why seafoam? Because it separates it from the component styles. */
+					--mod-detail-font-color: var(--spectrum-seafoam-900);
+				}
+			</style>
+			<div style="padding: 16px">${Story(context)}</div>
+		`,
 	],
 };
 
+
+const ChromaticTipPlacementVariants = (args) => html`
+	${placementOptions.map(option => {
+		const optionDescription = () => {
+			if (option.startsWith("start") || option.startsWith("end"))
+				return "Changes side with text direction (like a logical property)";
+			if (option.startsWith("left") || option.startsWith("right"))
+				return "Text direction does not affect the position";
+			return null;
+		};
+
+		return html`
+			<div class="spectrum-Typography" style="padding-bottom: 12rem;">
+				${Typography({
+					semantics: "detail",
+					size: "l",
+					content: [`${option}`],
+				})}
+				<div>
+					${when(optionDescription() !== null, () => html`
+						${Typography({
+							semantics: "detail",
+							size: "s",
+							content: [`${optionDescription()}`],
+						})}
+					`)}
+				</div>
+				<div style="padding-top: 2rem">
+					${Template({
+						...args,
+						position: option,
+						isOpen: true,
+						trigger: () => null,
+					})}
+				</div>
+			</div>
+		`;
+	})}
+`;
+
 export const Default = Template.bind({});
 Default.play = async ({ canvasElement }) => {
 	const canvas = within(canvasElement);
 	await userEvent.click(canvas.getByRole("button"));
 };
 Default.args = {};
 
-export const WithTip = Template.bind({});
+export const WithTip = (args) => html`
+	${window.isChromatic() ? ChromaticTipPlacementVariants(args) : Template(args)}
+`;
 WithTip.play = async ({ canvasElement }) => {
 	const canvas = within(canvasElement);
-	await userEvent.click(canvas.getByRole("button"));
+	window.isChromatic() ? null : await userEvent.click(canvas.getByRole("button"));
 };
 WithTip.args = {
 	withTip: true,

diff --git a/components/popover/stories/template.js b/components/popover/stories/template.js
@@ -60,49 +60,61 @@ export const Template = ({
 					const popWidth = popover.offsetWidth ?? 0;
 					const popHeight = popover.offsetHeight ?? 0;
 					const textDir = getComputedStyle(document.querySelector("html")).direction;
+
 					let x, y;
 					let xOffset = "+ 0px";
 					let yOffset = "+ 0px";
-					if (position.includes("top") || position.includes("bottom") && !(position.includes("-top") || position.includes("-bottom"))) {
+
+					if (position.startsWith("top") || position.startsWith("bottom")) {
 						x = triggerXCenter - (popWidth > 0 ? popWidth / 2 : popWidth);
 					}
-					if (position.includes("left") || position.includes("right")) {
+					if (position.includes("left") || position.includes("right") || position.startsWith("start") || position.startsWith("end")) {
 						y = triggerYCenter - (popHeight > 0 ? popHeight / 2 : popHeight);
 					}
-					if (position.includes("top") && !position.includes("-top")) {
+					if (position.startsWith("top")) {
 						y = rect.top - popHeight;
 						yOffset = withTip
 							? "- (var(--spectrum-popover-pointer-height) + var(--spectrum-popover-animation-distance) - 1px)"
 							: "- var(--spectrum-popover-animation-distance)";
 					}
- else if (position.includes("bottom") && !position.includes("-bottom")) {
+					else if (position.startsWith("bottom")) {
 						y = rect.bottom;
 						yOffset = "+ (var(--spectrum-popover-animation-distance))";
 					}
- else if (position.includes("left")) {
+					else if (position.includes("left")) {
 						if (textDir == "rtl") {
 							x = rect.right;
 							xOffset = withTip ? "+ 0px" : "+ var(--spectrum-popover-animation-distance)";
 						}
- else {
+						else {
 							x = rect.left - popWidth;
 							xOffset = withTip
 								? "- ((var(--spectrum-popover-pointer-width) / 2) + var(--spectrum-popover-animation-distance) - 2px)"
 								: "- var(--spectrum-popover-animation-distance)";
 						}
 					}
- else if (position.includes("right")) {
+					else if (position.includes("right")) {
 						if (textDir == "rtl") {
 							x = rect.left - popWidth;
 							xOffset = withTip
 								? "- ((var(--spectrum-popover-pointer-width) / 2) + var(--spectrum-popover-animation-distance) - 2px)"
 								: "- var(--spectrum-popover-animation-distance)";
 						}
- else {
+						else {
 							x = rect.right;
 							xOffset = withTip ? "+ 0px" : "+ var(--spectrum-popover-animation-distance)";
 						}
 					}
+					else if (position.includes("start")) {
+						x = rect.left - popWidth;
+						xOffset = withTip
+							? "- ((var(--spectrum-popover-pointer-width) / 2) + var(--spectrum-popover-animation-distance) - 2px)"
+							: "- var(--spectrum-popover-animation-distance)";
+					}
+					else if (position.includes("end")) {
+						x = rect.right;
+						xOffset = withTip ? "+ 0px" : "+ var(--spectrum-popover-animation-distance)";
+					}
 
 					if (x) transforms.push(`translateX(calc(var(--flow-direction) * calc(${parseInt(x, 10)}px ${xOffset})))`);
 					if (y) transforms.push(`translateY(calc(${y}px ${yOffset}))`);
@@ -111,13 +123,13 @@ export const Template = ({
 					if (position === "top-start" || position === "bottom-start") {
 						additionalStyles["inset-inline-start"] = "calc(" + (popWidth / 2) + "px - var(--spectrum-popover-pointer-edge-offset))";
 					}
- else if (position === "top-end" || position === "bottom-end") {
+					else if (position === "top-end" || position === "bottom-end") {
 						additionalStyles["inset-inline-start"] = "calc(-1 *" + (popWidth / 2) + "px + var(--spectrum-popover-pointer-edge-offset))";
 					}
- else if (position === "left-top" || position === "right-top") {
+					else if (position === "left-top" || position === "right-top" || position === "start-top" || position === "end-top") {
 						additionalStyles["inset-block-start"] = "calc(" + (popHeight / 2) + "px - var(--spectrum-popover-pointer-edge-offset))";
 					}
- else if (position === "left-bottom" || position === "right-bottom") {
+					else if (position === "left-bottom" || position === "right-bottom" || position === "start-bottom" || position === "end-bottom") {
 						additionalStyles["inset-block-start"] = "calc(-1 *" + (popHeight / 2) + "px + var(--spectrum-popover-pointer-edge-offset))";
 					}