Skip to content

Conversation

@srobinson
Copy link
Member

CRITICAL FIX: 4-value position syntax was blindly pairing first+second as X,
third+fourth as Y, causing incorrect expansion when vertical keyword comes first.

Example failure:
background-position: top 20px left 15%
Before: { x: 'top 20px', y: 'left 15%' } ❌
After: { x: 'left 15%', y: 'top 20px' } ✅

Now checks first keyword's axis to determine pairing.

Affects: background-position, mask-position, object-position expansion

CRITICAL FIX: 4-value position syntax was blindly pairing first+second as X,
third+fourth as Y, causing incorrect expansion when vertical keyword comes first.

Example failure:
  background-position: top 20px left 15%
  Before: { x: 'top 20px', y: 'left 15%' } ❌
  After:  { x: 'left 15%', y: 'top 20px' } ✅

Now checks first keyword's axis to determine pairing.

Affects: background-position, mask-position, object-position expansion
Copilot AI review requested due to automatic review settings November 13, 2025 13:25
@srobinson srobinson merged commit 769bbd2 into main Nov 13, 2025
12 checks passed
@srobinson srobinson deleted the develop branch November 13, 2025 13:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a critical bug in the 4-value position syntax parser where values were incorrectly paired when a vertical keyword appeared first. The fix adds axis detection logic to correctly determine which value pairs represent horizontal vs. vertical components.

Key Changes

  • Added conditional logic to check if the first keyword is horizontal or vertical before pairing values
  • Updated test descriptions to clarify horizontal-first vs vertical-first scenarios
  • Added new test cases for vertical-first 4-value syntax (e.g., "top 20px left 15%")

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/internal/position-parser.ts Adds axis detection in 4-value syntax handler to correctly pair horizontal and vertical components based on the first keyword
src/internal/tests/position-parser.test.ts Splits 4-value tests into separate cases for horizontal-first and vertical-first scenarios, adding coverage for the vertical-first bug fix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +115 to +121
// Determine which pair is horizontal vs vertical based on first keyword
if (HORIZONTAL_KEYWORDS.has(first)) {
// first+second is horizontal, third+fourth is vertical
return { x: `${first} ${second}`, y: `${third} ${fourth}` };
}
// first+second is vertical, third+fourth is horizontal
return { x: `${third} ${fourth}`, y: `${first} ${second}` };
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The 4-value syntax logic doesn't validate that the input is well-formed. Two issues:

  1. Missing keyword validation: If the first value is not a keyword (e.g., "10px 20px 30px 40px"), the code falls through to line 121 and incorrectly treats it as vertical-first.

  2. Missing axis validation: The code doesn't verify that the first and third keywords are on opposite axes. For example, "left 10px right 20px" (both horizontal) would be incorrectly parsed.

According to CSS spec, 4-value syntax must be "edge offset edge offset" where edges are on opposite axes. Consider:

// Validate that first and third are keywords on opposite axes
if (!POSITION_KEYWORDS.has(first) || !POSITION_KEYWORDS.has(third)) {
  return { x: parts[0], y: parts[1] || "center" }; // fallback
}

const firstIsHorizontal = HORIZONTAL_KEYWORDS.has(first);
const thirdIsHorizontal = HORIZONTAL_KEYWORDS.has(third);

// Both must be on opposite axes (unless using "center" which is both)
if (first !== "center" && third !== "center" && firstIsHorizontal === thirdIsHorizontal) {
  return { x: parts[0], y: parts[1] || "center" }; // fallback
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants