fix: correct toggle logic for partial selection#840
Draft
makhnatkin wants to merge 6 commits intomainfrom
Draft
Conversation
Contributor
|
Preview is ready. |
Contributor
|
Visual Tests Report is ready. |
makhnatkin
force-pushed
the
fix/toggle-mark
branch
from
355a4a5 to
b1765cc
Compare
|
|
|
|
Add jest coverage for the wysiwyg/markdown changes shipping in this PR
and pin the known markup-mode regression so a future fix flips the
assertion intentionally.
- Color.action.test.ts: 15 cases for the rewritten run() — empty/range
branches, addMark replace under PM default `excludes`, isActive/meta
semantics after the storedMarks switch, multi-paragraph selection.
- actions.test.ts: createMarkdownInlineMarkAction × removeWhenPresent:false
— confirms partial-coverage now applies on safe boundaries, documents
the silent no-op on flanking-blocked boundaries.
- markup/commands/marks.test.ts: pinning tests for colorify wrap/unwrap
paths plus M3 capturing the malformed nested wrapping when switching
colours over an existing `{<color>}(...)` (tracked in #1129) and
sanity coverage for toggleBold/toggleItalic.
- marks.test.ts: H1–H4 edge cases for selectionAllHasMarkWithAttr —
cross-node coverage, attr-value mismatch, parent.allowsMarkType skip,
empty-range vacuous true.
- MToolbarColors.tsx: TODO now references issue #1129.
All 84 suites / 778 tests pass. tsc clean, eslint+prettier clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
@sourcery-ai review |
Reviewer's GuideRefactors mark toggling so boolean marks and the Color mark handle partially covered selections correctly, adds a helper to detect full-coverage parameterised marks, rewrites the Color action to differentiate cursor vs range behaviour using stored marks and attribute-aware coverage checks, aligns Color’s isActive/meta with stored marks semantics, simplifies toolbar color toggling in both WYSIWYG and markup modes, and adds focused Jest tests for the new behaviour plus a pinning test for a known markup-mode color bug. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/editor/src/extensions/yfm/Color/index.ts" line_range="32" />
<code_context>
+ isActive: (state) =>
+ Boolean(type.isInSet(state.storedMarks ?? state.selection.$to.marks())),
isEnable: toggleMark(type),
run: (state, dispatch, _view, attrs) => {
const params = attrs as ColorActionParams | undefined;
- const hasMark = isMarkActive(state, type);
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting cursor and range color‑toggling logic into separate helpers and adding a small helper for effective marks to flatten `run` and reduce duplication.
You can keep the new behaviour but make it easier to reason about by extracting a few small helpers and reducing nesting in `run`.
### 1. Split cursor vs range handling
The `run` body is doing both cursor and range logic inline. Pulling these into helpers makes `run` a simple dispatcher and flattens the nesting:
```ts
function toggleColorAtCursor(
state: EditorState,
dispatch: (tr: Transaction) => void,
type: MarkType,
color?: string,
) {
const { $cursor } = state.selection as TextSelection;
if (!$cursor) return false;
const storedMark = type.isInSet(getEffectiveMarks(state, $cursor));
if (!color || storedMark?.attrs[colorMarkName] === color) {
dispatch(state.tr.removeStoredMark(type));
} else {
dispatch(state.tr.addStoredMark(type.create({ [colorMarkName]: color })));
}
return true;
}
function toggleColorInSelection(
state: EditorState,
dispatch: (tr: Transaction) => void,
type: MarkType,
color?: string,
) {
const tr = state.tr;
if (!color) {
state.selection.ranges.forEach(({ $from, $to }) =>
tr.removeMark($from.pos, $to.pos, type),
);
} else {
const allSameColor = selectionAllHasMarkWithAttr(
state,
type,
colorMarkName,
color,
);
state.selection.ranges.forEach(({ $from, $to }) => {
if (allSameColor) {
tr.removeMark($from.pos, $to.pos, type);
} else {
tr.addMark(
$from.pos,
$to.pos,
type.create({ [colorMarkName]: color }),
);
}
});
}
dispatch(tr.scrollIntoView());
return true;
}
```
Then `run` becomes:
```ts
run: (state, dispatch, _view, attrs) => {
const color = (attrs as ColorActionParams | undefined)?.[colorMarkName];
if (!dispatch) return true;
const { empty } = state.selection as TextSelection;
if (empty) {
return toggleColorAtCursor(state, dispatch, type, color);
}
return toggleColorInSelection(state, dispatch, type, color);
},
```
This keeps all current behaviour but makes each branch smaller and single‑purpose.
### 2. De‑duplicate effective marks access
You currently repeat `state.storedMarks ?? state.selection.$to.marks()` (and a variant in the cursor branch). A small helper makes the intent explicit and cuts duplication:
```ts
function getEffectiveMarks(state: EditorState, $pos = state.selection.$to) {
return state.storedMarks ?? $pos.marks();
}
```
Use it in `isActive` and `meta`:
```ts
isActive: (state) =>
Boolean(type.isInSet(getEffectiveMarks(state))),
meta(state): Colors {
return type.isInSet(getEffectiveMarks(state))?.attrs[colorMarkName];
},
```
And in the cursor helper above (`getEffectiveMarks(state, $cursor)`).
These changes keep the richer selection/cursor semantics while making the flow flatter and reducing cognitive load.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| isActive: (state) => | ||
| Boolean(type.isInSet(state.storedMarks ?? state.selection.$to.marks())), | ||
| isEnable: toggleMark(type), | ||
| run: (state, dispatch, _view, attrs) => { |
There was a problem hiding this comment.
issue (complexity): Consider extracting cursor and range color‑toggling logic into separate helpers and adding a small helper for effective marks to flatten run and reduce duplication.
You can keep the new behaviour but make it easier to reason about by extracting a few small helpers and reducing nesting in run.
1. Split cursor vs range handling
The run body is doing both cursor and range logic inline. Pulling these into helpers makes run a simple dispatcher and flattens the nesting:
function toggleColorAtCursor(
state: EditorState,
dispatch: (tr: Transaction) => void,
type: MarkType,
color?: string,
) {
const { $cursor } = state.selection as TextSelection;
if (!$cursor) return false;
const storedMark = type.isInSet(getEffectiveMarks(state, $cursor));
if (!color || storedMark?.attrs[colorMarkName] === color) {
dispatch(state.tr.removeStoredMark(type));
} else {
dispatch(state.tr.addStoredMark(type.create({ [colorMarkName]: color })));
}
return true;
}
function toggleColorInSelection(
state: EditorState,
dispatch: (tr: Transaction) => void,
type: MarkType,
color?: string,
) {
const tr = state.tr;
if (!color) {
state.selection.ranges.forEach(({ $from, $to }) =>
tr.removeMark($from.pos, $to.pos, type),
);
} else {
const allSameColor = selectionAllHasMarkWithAttr(
state,
type,
colorMarkName,
color,
);
state.selection.ranges.forEach(({ $from, $to }) => {
if (allSameColor) {
tr.removeMark($from.pos, $to.pos, type);
} else {
tr.addMark(
$from.pos,
$to.pos,
type.create({ [colorMarkName]: color }),
);
}
});
}
dispatch(tr.scrollIntoView());
return true;
}Then run becomes:
run: (state, dispatch, _view, attrs) => {
const color = (attrs as ColorActionParams | undefined)?.[colorMarkName];
if (!dispatch) return true;
const { empty } = state.selection as TextSelection;
if (empty) {
return toggleColorAtCursor(state, dispatch, type, color);
}
return toggleColorInSelection(state, dispatch, type, color);
},This keeps all current behaviour but makes each branch smaller and single‑purpose.
2. De‑duplicate effective marks access
You currently repeat state.storedMarks ?? state.selection.$to.marks() (and a variant in the cursor branch). A small helper makes the intent explicit and cuts duplication:
function getEffectiveMarks(state: EditorState, $pos = state.selection.$to) {
return state.storedMarks ?? $pos.marks();
}Use it in isActive and meta:
isActive: (state) =>
Boolean(type.isInSet(getEffectiveMarks(state))),
meta(state): Colors {
return type.isInSet(getEffectiveMarks(state))?.attrs[colorMarkName];
},And in the cursor helper above (getEffectiveMarks(state, $cursor)).
These changes keep the richer selection/cursor semantics while making the flow flatter and reducing cognitive load.
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
2 participants
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.
Summary
Fix toggle behaviour for partial selections so toolbar actions and markdown shortcuts behave the same. Mixed inline-mark selections now apply the mark to the whole selection, fully covered selections still toggle off, and color keeps the expected ProseMirror whitespace and active-state semantics.
What changed
removeWhenPresent: false, so mixed selections apply the mark instead of removing it.Colornow handles cursor and range selections explicitly, while preserving:isActivefor partially colored non-empty selectionsWToolbarColorsno longer decides whether a click should clear the color; that logic now lives in the action.colorifycan unwrap the same color and replace another color when the selection exactly matches an existing{color}(...)wrapper.Known limitation
#1129.Tests
Added unit coverage for:
utils/actions.test.tsColorcursor/range behaviour, including trailing-whitespace handling and partially colored selection statecolorifyunwrap/replace behaviour for exact wrapper selections