Skip to content

fix(🐛): compare isEdge against the far edges of the rect - #4028

Open
dennytosp wants to merge 1 commit into
Shopify:mainfrom
dennytosp:fix/is-edge-right-and-bottom-edges
Open

fix(🐛): compare isEdge against the far edges of the rect#4028
dennytosp wants to merge 1 commit into
Shopify:mainfrom
dennytosp:fix/is-edge-right-and-bottom-edges

Conversation

@dennytosp

Copy link
Copy Markdown
Contributor

isEdge is exported from the package root (dom/nodesdatatypesRect) and compares the far edges against the rect's width/height rather than x + width / y + height:

pos.x === b.x || pos.y === b.y || pos.x === b.width || pos.y === b.height

It is inconsistent with itself — the near edges are offset by b.x/b.y, the far ones are not. The history explains why: #500 extracted it from an inline check in the Aurora example,

pt.pos.x === 0 || pt.pos.y === 0 ||
pt.pos.x === Math.fround(width) || pt.pos.y === Math.fround(height)

whose rect was always Skia.XYWHRect(0, 0, width, height). At the origin the offset makes no difference, so only half the generalisation was needed and only half got written.

For a rect away from the origin it reports two edges that fall inside the rect and misses the two real ones. For rect(10, 20, 100, 50) the right edge is at x = 110, but the current code answers true for x = 100 and false for x = 110.

Test added to Geometry.spec.ts; it fails on main at the first far-edge assertion.

isEdge was extracted in Shopify#500 from an inline check whose rect was always at the
origin, so `pos.x === width` happened to be right there. Generalising it to an
SkRect only carried the near edges over: the far ones still compare against the
rect's width and height instead of x + width and y + height, so for any rect
not at the origin the function reports two edges that are inside the rect and
misses the two real ones.
@dennytosp
dennytosp force-pushed the fix/is-edge-right-and-bottom-edges branch from 98bf54c to fcdec72 Compare August 24, 2026 15:11
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.

1 participant