Fix marginTop/marginBottom returning undefined when selection spans unmeaningful paragraphs#3306
Open
BryanValverdeU wants to merge 3 commits intomasterfrom
Open
Fix marginTop/marginBottom returning undefined when selection spans unmeaningful paragraphs#3306BryanValverdeU wants to merge 3 commits intomasterfrom
BryanValverdeU wants to merge 3 commits intomasterfrom
Conversation
…nmeaningful paragraphs When a selection spans multiple paragraphs and the cursor lands on an unmeaningful paragraph (one that contains only a trailing/leading SelectionMarker), the previous call to getSelectedParagraphs with removeUnmeaningful=true would skip those paragraphs. This caused getFormatState to see a mix of paragraphs — some with marginTop/marginBottom set and some without — which triggered the conflict-resolution logic and returned undefined for those margin values, breaking the "space before/after" toolbar buttons. Fix: pass removeUnmeaningful=false in formatParagraphWithContentModel so that all selected paragraphs (including unmeaningful ones at the edges) receive the same format, keeping margins consistent across the selection. Also fix the isChecked logic in spaceBeforeAfterButtons so the button highlights correctly when a positive margin is present, and add overload + implementation guard for the new removeUnmeaningful parameter in getSelectedParagraphs. Add unit tests for the new removeUnmeaningful=false and mutate=true paths in collectSelections, and for formatParagraphWithContentModel covering multiple paragraphs, list items, table cells, no-selection return value, and apiName threading. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BryanValverdeU
force-pushed
the
u/bvalverde/fixSpacingIssue
branch
from
126b470 to
d10f567
Compare
BryanValverdeU
requested review from
JiuqingSong,
Copilot,
flyingbee2012,
ianeli1,
juliaroldi and
vinguyen12
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses inconsistent paragraph margin format state when a selection spans “unmeaningful” paragraphs (e.g., paragraphs containing only SelectionMarkers), which previously caused marginTop/marginBottom to resolve to undefined and break the space-before/after toolbar toggle state.
Changes:
- Extend
getSelectedParagraphsto optionally skip removing “unmeaningful” selections via a newremoveUnmeaningfulparameter (defaulting to current behavior). - Update
formatParagraphWithContentModelto include unmeaningful edge paragraphs during formatting to keep margins consistent. - Fix ribbon toggle logic for the “space after” button and add unit tests covering the new selection/formatting paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/roosterjs-content-model-dom/test/modelApi/selection/collectSelectionsTest.ts | Adds tests for removeUnmeaningful=false and mutate=true behaviors in getSelectedParagraphs. |
| packages/roosterjs-content-model-dom/lib/modelApi/selection/collectSelections.ts | Introduces removeUnmeaningful parameter handling and a new overload for the mutable path. |
| packages/roosterjs-content-model-api/test/publicApi/utils/formatParagraphWithContentModelTest.ts | Expands test coverage for multi-paragraph, list, and table scenarios, plus return value and apiName threading. |
| packages/roosterjs-content-model-api/lib/publicApi/utils/formatParagraphWithContentModel.ts | Passes removeUnmeaningful=false to ensure all selected paragraphs get formatted consistently. |
| demo/scripts/controlsV2/demoButtons/spaceBeforeAfterButtons.ts | Fixes isChecked logic so “space after” highlights when a positive margin is present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/roosterjs-content-model-dom/lib/modelApi/selection/collectSelections.ts
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a selection spans multiple paragraphs and the cursor lands on an unmeaningful paragraph (one that contains only a trailing/leading SelectionMarker), the previous call to getSelectedParagraphs with removeUnmeaningful=true would skip those paragraphs. This caused getFormatState to see a mix of paragraphs — some with marginTop/marginBottom set and some without — which triggered the conflict-resolution logic and returned undefined for those margin values, breaking the "space before/after" toolbar buttons.
Fix: pass removeUnmeaningful=false in formatParagraphWithContentModel so that all selected paragraphs (including unmeaningful ones at the edges) receive the same format, keeping margins consistent across the selection.
Also fix the isChecked logic in spaceBeforeAfterButtons so the button highlights correctly when a positive margin is present, and add overload + implementation guard for the new removeUnmeaningful parameter in getSelectedParagraphs.
Add unit tests for the new removeUnmeaningful=false and mutate=true paths in collectSelections, and for formatParagraphWithContentModel covering multiple paragraphs, list items, table cells, no-selection return value, and apiName threading.
To fix https://outlookweb.visualstudio.com/Outlook%20Web/_workitems/edit/370198
Torepro
Restore this snapshot:
Before
Toggle state of the buttons is not working well
After
Is working well.