Skip to content

SD-1830 - use fragment.lines for click position in multi-column layouts#1963

Merged
caio-pizzol merged 3 commits into
mainfrom
SD-1830
Feb 27, 2026
Merged

SD-1830 - use fragment.lines for click position in multi-column layouts#1963
caio-pizzol merged 3 commits into
mainfrom
SD-1830

Conversation

@mattConnHarbour
Copy link
Copy Markdown
Contributor

No description provided.

@linear
Copy link
Copy Markdown

linear Bot commented Feb 7, 2026

Copy link
Copy Markdown
Contributor

@caio-pizzol caio-pizzol left a comment

Choose a reason for hiding this comment

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

nice one @mattConnHarbour!

let's try to simplify a bit the code here

Comment thread packages/layout-engine/layout-bridge/src/index.ts Outdated
Comment thread packages/layout-engine/layout-bridge/test/clickToPosition.test.ts
Comment thread packages/layout-engine/layout-bridge/src/index.ts Outdated
lineIndex = fragment.fromLine + lineIndex;
}

// Guard against undefined line (defensive check)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

findLineIndexAtY already returns null when lines[i] is undefined (line 2127-2128), and if it returns a non-null index, every line in the range was verified to exist during iteration.

this guard can never trigger -- removing it avoids making a reader wonder "wait, can this actually be undefined?"

Comment thread packages/layout-engine/layout-bridge/src/index.ts Outdated
@mattConnHarbour
Copy link
Copy Markdown
Contributor Author

@caio-pizzol Thanks for the review! Just addressed your feedback - could you merge when you get a chance?

@caio-pizzol caio-pizzol self-requested a review February 27, 2026 16:11
End-to-end Playwright tests that verify clicking in two-column
documents works correctly across Chromium, Firefox, and WebKit.
Covers the customer-reported TypeError from IT-407.
Copy link
Copy Markdown
Contributor

@caio-pizzol caio-pizzol left a comment

Choose a reason for hiding this comment

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

@mattConnHarbour the feedback from last round is all addressed, and there are no new issues. also pushed a behavior test (Playwright, all 3 browsers) that loads a real two-column docx and clicks in the second column — end-to-end proof that IT-407 is fixed. lgtm, approving.

@caio-pizzol caio-pizzol merged commit 6a5a2e6 into main Feb 27, 2026
8 checks passed
@caio-pizzol caio-pizzol deleted the SD-1830 branch February 27, 2026 17:06
@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot Bot commented Feb 27, 2026

🎉 This PR is included in superdoc v1.17.0-next.35

The release is available on GitHub release

@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot Bot commented Feb 27, 2026

🎉 This PR is included in superdoc-cli v0.2.0-next.30

The release is available on GitHub release

@harbournick
Copy link
Copy Markdown
Collaborator

🎉 This PR is included in superdoc v1.17.0

The release is available on GitHub release

@superdoc-bot
Copy link
Copy Markdown
Contributor

superdoc-bot Bot commented Mar 11, 2026

🎉 This PR is included in superdoc-cli v0.2.0

The release is available on GitHub release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants