Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Combined selection format to ignore empty text nodes #5521

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Combined selection format to ignore empty text nodes #5521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 28 additions & 0 deletions packages/lexical-playground/__tests__/e2e/TextFormatting.spec.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1213,4 +1213,32 @@ test.describe('TextFormatting', () => {
`,
);
});

test('Multiline selection format ignores new lines', async ({
page,
isPlainText,
isCollab,
}) => {
test.skip(isPlainText);
let leftFrame = page;
if (isCollab) {
leftFrame = await page.frame('left');
}
await focusEditor(page);

await page.keyboard.type('Fist');
await page.keyboard.press('Enter');
await toggleUnderline(page);
await page.keyboard.type('Second');
await page.pause();

await moveLeft(page, 'Second'.length + 1);
await page.pause();
await selectCharacters(page, 'right', 'Second'.length + 1);
await page.pause();

await expect(
leftFrame.locator('.toolbar-item[title^="Underline"]'),
).toHaveClass(/active/);
});
});
46 changes: 24 additions & 22 deletions packages/lexical/src/LexicalEvents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,8 +304,6 @@ function onSelectionChange(
if ($isRangeSelection(selection)) {
const anchor = selection.anchor;
const anchorNode = anchor.getNode();
const focus = selection.focus;
const focusNode = focus.getNode();

if (selection.isCollapsed()) {
// Badly interpreted range selection when collapsed - #1482
Expand Down Expand Up @@ -351,30 +349,34 @@ function onSelectionChange(
}
}
} else {
let combinedFormat = IS_ALL_FORMATTING;
let hasTextNodes = false;
const anchorKey = anchor.key;
const focus = selection.focus;
const focusKey = focus.key;
const nodes = selection.getNodes();
if (
selection.getTextContent().startsWith('\n') &&
!selection.getTextContent().endsWith('\n') &&
(anchorNode.getTextContentSize() === anchor.offset ||
focusNode.getTextContentSize() === focus.offset) &&
$isTextNode(nodes[0])
) {
nodes.shift();
} else if (
!selection.getTextContent().startsWith('\n') &&
selection.getTextContent().endsWith('\n') &&
(anchor.offset === 0 || focus.offset === 0) &&
$isTextNode(nodes[nodes.length - 1])
) {
nodes.pop();
}

const nodesLength = nodes.length;
const isBackward = selection.isBackward();
const startOffset = isBackward ? focusOffset : anchorOffset;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be better to do an old-fashioned if/else here, since it will end up with 1 comparison and 4 assignments vs 4 comparisons and 4 assignments, or does this have some minification benefits?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nevermind, you'll need to do it with closure outside the if, so you'd end up with 1 comparison but 4 declarations and 4 reassignments. Doubt it will make any difference in bundle size or minification

const endOffset = isBackward ? anchorOffset : focusOffset;
const startKey = isBackward ? focusKey : anchorKey;
const endKey = isBackward ? anchorKey : focusKey;
let combinedFormat = IS_ALL_FORMATTING;
let hasTextNodes = false;
for (let i = 0; i < nodesLength; i++) {
const node = nodes[i];
if ($isTextNode(node)) {
const textContentSize = node.getTextContentSize();
if (
$isTextNode(node) &&
textContentSize !== 0 &&
// Exclude empty text nodes at boundaries resulting from user's selection
!(
(i === 0 &&
node.__key === startKey &&
startOffset === textContentSize) ||
(i === nodesLength - 1 &&
node.__key === endKey &&
endOffset === 0)
)
) {
// TODO: what about style?
hasTextNodes = true;
combinedFormat &= node.getFormat();
Expand Down