[lexical-list] Bullet item color matches text color #7024

ivailop7 · 2025-01-07T23:34:14Z

Most text editors do bullet lists with the marker color matching the font color in that list item. Investigated a few editors and how they handle different colors in the same list item.
They do differ in behaviour quite a bit apparently.

One of the popular editors, follows the color of the last text node. Another editor, follows the color only if all the text nodes inside the bullet have the same font color. A third one doesn't support it at all.

In the end, I decided to go with the first one, it's kind of the standard behaviour most people expect. Also because it's easiest to reason about and implement. I'm also unsure if we should do first node or last node. Personally, I think the first text node makes more sense vs the last.

Before:

Screen.Recording.2025-01-07.at.23.31.29.mov

After:

Screen.Recording.2025-01-12.at.22.45.22.mov

vercel · 2025-01-07T23:34:18Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 28, 2025 9:11pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 28, 2025 9:11pm

github-actions · 2025-01-07T23:35:56Z

size-limit report 📦

Path	Size
lexical - cjs	29.08 KB (+0.03% 🔺)
lexical - esm	28.87 KB (+0.03% 🔺)
@lexical/rich-text - cjs	38.04 KB (-0.01% 🔽)
@lexical/rich-text - esm	30.94 KB (+0.07% 🔺)
@lexical/plain-text - cjs	36.6 KB (+0.14% 🔺)
@lexical/plain-text - esm	28.19 KB (-0.13% 🔽)
@lexical/react - cjs	39.85 KB (+0.02% 🔺)
@lexical/react - esm	32.36 KB (+0.26% 🔺)

ivailop7 · 2025-01-07T23:39:20Z

There is the pseudoclass '::marker' that can isolate the color change to the marker only, but I didn't find a nice way to set this via javascript.

ivailop7 · 2025-01-07T23:43:15Z

@etrepum @zurfyx for ideas and comments

etrepum · 2025-01-08T00:43:06Z

This strategy is easy to implement but I don't think it's correct, IIRC updateDOM for parent nodes happens before the child nodes.

You couldn't do this with CSS and something like color: var(--marker-color) and set that from js (possibly from a mutation listener I guess, that would happen after everything is reconciled including children)?

ivailop7 · 2025-01-08T00:52:35Z

This strategy is easy to implement but I don't think it's correct, IIRC updateDOM for parent nodes happens before the child nodes.

You couldn't do this with CSS and something like color: var(--marker-color) and set that from js (possibly from a mutation listener I guess, that would happen after everything is reconciled including children)?

This sounds like a better idea. I wanted to avoid another listener just for this, but I guess even updateDom itself is just another listener implementation. Will give it a go.

etrepum · 2025-01-08T01:06:36Z

You may also want to use getComputedStyle since the text could be colored by means other than setting the style attribute directly (although of course that's not how the playground works today, it may be a bit more robust in other settings).

fantactuka · 2025-01-08T17:58:15Z

Curious why lastChild vs firstChild (given it's closer to the marker)

etrepum · 2025-01-08T19:03:46Z

In Google Docs it like the marker color is based on what the text color was when the marker was created, but you can change it if you apply a color to its entire contents at once (e.g. changing the left half from black to blue, then changing the right half to blue, will not change the marker to blue, but selecting all of the text and changing it to blue at once will). This would mean storing the color on the list item itself and updating it accordingly, rather than just doing DOM stuff.

ivailop7 · 2025-01-08T22:58:44Z

Curious why lastChild vs firstChild (given it's closer to the marker)

I agree it should be the first node, other editors opted for last one, thus I went with this. Probably because of their design. Will polish this sometime over the weekend.

ivailop7 · 2025-01-12T22:49:15Z

@etrepum @fantactuka updated the implementaion and the after video in the description. Behaviour is better, because it fixed also matching the marker size to the font-size. That said, still unsure if this is still the most efficient way. Because I have to listen to TextNode changes for the font color/font size. I'm unsure if the cost added on every TextNode 'update' is worth adding this weight. The CSS var approach didn't work, because after updating the variable, it would change the color of all bullets to the latest variable color value, and if having to override the variable value per list item in style is essentially the same overhead.

etrepum

This seems mostly reasonable but maybe it should be exposed as a separate register function so that people can choose to use this behavior or not? They might have different ideas for how this should work and it'll be very tricky to untangle it unless it's a separate call.

packages/lexical-list/src/index.ts

etrepum · 2025-01-13T04:35:14Z

packages/lexical-list/src/index.ts

+                if ($isRangeSelection(selection)) {
+                  listItemElement.setAttribute('style', selection.style || '');
+                }


This should probably check to see if it's the selection that created the ListItemNode, it could be from paste or whatever. I would probably just have the created and updated work the same way, with this selection case to cover scenarios where there is no text element child

This should probably check to see if it's the selection that created the ListItemNode, it could be from paste or whatever. I would probably just have the created and updated work the same way, with this selection case to cover scenarios where there is no text element child

Implement the other things, but I'm not sure I'm getting the "if it's the selection that created the ListItemNode" part. Are you referring to handling PASTE and/or SELECTION_CHANGE commands or something else?

A created mutation event happens the first time a node is rendered to DOM for whatever reason, so the style of the editor's current selection might not really have anything to do with that newly rendered ListItemNode. It is mostly only relevant to when you press return or some other UI action to create a new ListItemNode. It's reasonable to assume that case should pick up the style. In that case the selection will almost certainly be collapsed inside the ListItemNode.

It's hard to imagine what all of the other scenarios that would render a new ListItemNode could be, paste is certainly one of them. Selection change probably isn't.

If you were to always copy the style of the selection to an empty ListItemNode so long as the selection is collapsed inside of it then I think it would also work well, it doesn't need to only be applied on create (haven't looked at the code again but that could possibly simplify some other stuff?)

etrepum · 2025-01-16T16:20:23Z

Unit tests are failing, looks like they are not expecting style="" to be on these nodes. Might be worth some special casing to avoid putting style="" in the DOM

ivailop7 · 2025-01-19T01:07:06Z

Unit tests are failing, looks like they are not expecting style="" to be on these nodes. Might be worth some special casing to avoid putting style="" in the DOM

added the isCollapsed and fixed the tests, it's the same logic now for create and update for ListItemNode, where if no child, take the selection, else take the first child style. IMO, it should handle Paste correctly as well without any other additions.

etrepum

This looks very close but the update cascade is not ideal

etrepum · 2025-01-20T01:39:46Z

packages/lexical-list/src/index.ts

@@ -97,6 +101,49 @@ export function registerList(editor: LexicalEditor): () => void {
      },
      COMMAND_PRIORITY_LOW,
    ),
+    editor.registerMutationListener(


Using an editor.update in a mutation listener causes an update cascade, if this were directly updating the DOM style and not the node's state it wouldn't be a problem but this will cause a second reconciliation as implemented. Is there a reason why this can't work as a transform? I think it should be fine so long as you check to make sure that the style actually changed before setting it (and thus marking the node dirty again, causing a second iteration of the transform)

Hmm, I did remove it, but it was throwing me the error that it couldn't find an activeEditor, I'll try again, in case it was an HMR thing, but I tried a few times. OK, will look into making this a transform vs mutationListener.

well if you are reading or updating the editor from a mutation listener then you do need a read or update call, it doesn't put you in a specific editor state because there are two to choose from. Updating is something that shouldn't happen in a mutation listener for the same reason that it shouldn't happen in an update listener (an update listener is basically a mutation listener for the whole editor state)

Ok, revisiting this again. Your suggestion is to have 1 node transform on TextNode-level and have the whole logic in there or just convert the current mutationlistener to a nodetransform. I did the convert of this method, but it doesn't reflect correctly if I already have the style applied and then create a bullet, it only updates after the first character is typed. I've pushed, despite not working to get a bit better clarity

This implementation is not what I meant, a node transform shouldn't do anything with the DOM, it can just modify the lexical nodes which will reflect to the DOM with updateDOM and/or createDOM. It's possible I misread the previous commit, I'll take a look at it locally in the next day or so.

etrepum

@ivailop7 this is basically what I was thinking, PR updated

etrepum · 2025-01-28T21:11:16Z

packages/lexical-list/src/LexicalListItemNode.ts

@@ -80,6 +80,9 @@ export class ListItemNode extends ElementNode {
    }
    element.value = this.__value;
    $setListItemThemeClassNames(element, config.theme, this);
+    if (this.__style !== '') {


Using the element's style attribute in createDOM and updateDOM was part of the fix here

We'll need to add the style export to the ExportDOM too

I didn’t look there because I wasn’t trying to polish this off, just demonstrate the approach, but usually createDOM is used by exportDOM so unless it has specific stuff that would override this then it might not need extra code

etrepum · 2025-01-28T21:11:52Z

packages/lexical-list/src/index.ts

+    editor.registerCommand(
+      SELECTION_CHANGE_COMMAND,
+      $checkSelectionListener,
+      COMMAND_PRIORITY_EDITOR,
+    ),


This updates the style when a new ListItemNode is created based on the selection

Testing in the preview playground, upon creating the list item, it does not seem to pick up the selection style. Styles only get updated after the first child gets created inside the node.

It worked when I tried it so maybe you’re doing something I wasn’t, I’m pretty deep into fixing some more fundamental issues right now and am not planning to spend more hands-on time on this for at least a few days

etrepum · 2025-01-28T21:12:55Z

packages/lexical-list/src/index.ts

+    editor.registerNodeTransform(ListItemNode, (node) => {
+      const firstChild = node.getFirstChild();
+      if (firstChild && $isTextNode(firstChild)) {
+        const style = firstChild.getStyle();
+        if (node.getStyle() !== style) {
+          node.setStyle(style);
+        }
+      }
+    }),
+    editor.registerNodeTransform(TextNode, (node) => {
+      const listItemParentNode = node.getParent();
+      if (
+        $isListItemNode(listItemParentNode) &&
+        node.is(listItemParentNode.getFirstChild())
+      ) {
+        listItemParentNode.markDirty();
+      }
+    }),


Here we are only looking at the first child to propagate the style to the parent ListItemNode. It could probably be generalized a bit to work with element nodes too, I was just following your template here.

etrepum · 2025-01-28T21:14:08Z

packages/lexical-selection/src/lexical-node.ts

+    const emptyNode = selection.anchor.getNode();
+    if ($isElementNode(emptyNode) && emptyNode.isEmpty()) {
+      $patchStyle(emptyNode, patch);
+    }


This was the more fundamental fix, we want to patch an empty element's style when the selection is collapsed on it or else there will be a disagreement of which style is correct since LexicalEvents has the selection's style follow the node's

etrepum · 2025-01-28T21:14:28Z

packages/lexical/src/LexicalEvents.ts

+function $updateSelectionFormatStyle(
+  selection: RangeSelection,
+  format: number,
+  style: string,
+) {
+  if (selection.format !== format || selection.style !== style) {
+    selection.format = format;
+    selection.style = style;
+    selection.dirty = true;
+  }
+}


This was just a clean up to make sure that everything is set properly and marked dirty as appropriate

ivailop7 · 2025-01-28T23:27:28Z

packages/lexical-list/src/LexicalListItemNode.ts

-
+    if (prevNode.__style !== this.__style) {
+      dom.style.cssText = this.__style;
+    }
    return false;


I see, could you explain to me the general rule here. Why are we avoiding returning 'true' in updateDOM with something like (this.style != prevNode.style), but we are always ok on doing the comparison and changing this in here and returning false. Does updateDOM returning true, trigger a waterfall of updates that is unnecessary?

The check here is for performance and to avoid having style=“” show up in the DOM, it could be unconditionally set.

returning true should be avoided whenever possible, it creates a lot more work on the browser and can lose state if there’s anything ephemeral in the DOM. Same reason why react does DOM diffing instead of rendering everything from scratch, just more manual here.

ivailop7 requested review from zurfyx, fantactuka, acywatson, Fetz, Sahejkm and potatowagon as code owners January 7, 2025 23:34

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 7, 2025

vercel bot had a problem deploying to Preview – lexical January 7, 2025 23:35 Failure

vercel bot deployed to Preview – lexical-playground January 7, 2025 23:35 View deployment

ivailop7 added 2 commits January 12, 2025 20:22

Bullet color to follow text color

7080a79

Using Listeners

af615a0

ivailop7 force-pushed the bullet_colors branch from b44ee80 to af615a0 Compare January 12, 2025 22:44

vercel bot deployed to Preview – lexical January 12, 2025 22:45 View deployment

vercel bot deployed to Preview – lexical-playground January 12, 2025 22:45 View deployment

etrepum reviewed Jan 13, 2025

View reviewed changes

Implement suggestions

6b7f32c

vercel bot deployed to Preview – lexical-playground January 14, 2025 23:39 View deployment

vercel bot deployed to Preview – lexical January 14, 2025 23:39 View deployment

Check selection is collapsed

5869d83

vercel bot deployed to Preview – lexical-playground January 19, 2025 00:22 View deployment

vercel bot deployed to Preview – lexical January 19, 2025 00:22 View deployment

Fix test

5622672

vercel bot deployed to Preview – lexical-playground January 19, 2025 01:02 View deployment

vercel bot deployed to Preview – lexical January 19, 2025 01:02 View deployment

ivailop7 added the extended-tests Run extended e2e tests on a PR label Jan 19, 2025

etrepum reviewed Jan 20, 2025

View reviewed changes

nodetransform

f9f3cc2

vercel bot deployed to Preview – lexical January 28, 2025 00:06 View deployment

vercel bot deployed to Preview – lexical-playground January 28, 2025 00:07 View deployment

Change ListItemNode style based on transforms and selection

e8c3774

vercel bot deployed to Preview – lexical January 28, 2025 21:11 View deployment

vercel bot deployed to Preview – lexical-playground January 28, 2025 21:11 View deployment

etrepum reviewed Jan 28, 2025

View reviewed changes

ivailop7 commented Jan 28, 2025

View reviewed changes

[lexical-list] Bullet item color matches text color #7024

Are you sure you want to change the base?

[lexical-list] Bullet item color matches text color #7024

Conversation

ivailop7 commented Jan 7, 2025 • edited Loading

vercel bot commented Jan 7, 2025 • edited Loading

github-actions bot commented Jan 7, 2025 • edited Loading

size-limit report 📦

ivailop7 commented Jan 7, 2025

ivailop7 commented Jan 7, 2025

etrepum commented Jan 8, 2025

ivailop7 commented Jan 8, 2025

etrepum commented Jan 8, 2025

fantactuka commented Jan 8, 2025

etrepum commented Jan 8, 2025

ivailop7 commented Jan 8, 2025

ivailop7 commented Jan 12, 2025

etrepum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etrepum commented Jan 16, 2025

ivailop7 commented Jan 19, 2025

etrepum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivailop7 Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etrepum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivailop7 commented Jan 7, 2025 •

edited

Loading

vercel bot commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025 •

edited

Loading

ivailop7 Jan 20, 2025 •

edited

Loading