feat(input): rebased version of "list state, toolbar, docs, and e2e (lists 5/5)"#545
feat(input): rebased version of "list state, toolbar, docs, and e2e (lists 5/5)"#545hsource wants to merge 7 commits into
Conversation
Introduces BlockType.ANCHORED (Android) and ENRMBlockTypePersistsWhenEmpty (iOS) and routes the block store's adjust/normalize passes, the formatter's zero-length anchor stamping, and the views' orphan-anchor prune through them instead of heading-only checks. No behavior change — headings are the only anchored type until the list block type lands on top. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
UNORDERED_LIST_ITEM and ORDERED_LIST_ITEM with 0-based nesting depth in the range's level payload, joined to the anchored-type set. The store's list-metadata pass clamps depths to valid ancestry (an item nests at most one level under the previous adjacent item, so every state serializes) and computes each ordered item's 1-based ordinal from its position among adjacent same-depth, same-type siblings. One handler instance per type: Android renders via LeadingMarginSpans (bullet dot/ring/square by depth; right-aligned number for ordered) plus a LineHeightSpan for spacing; iOS reserves the indent column via the paragraph style and draws the glyph or number in the layout manager, with the ordinal stamped as a text attribute. listItemSpacing prop spaces items on both platforms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Parsers map list items to their block type with depth derived from AST UL/OL nesting (never leading spaces); iOS tags MD_BLOCK_LI itself since md4c emits inner paragraphs only for loose lists, clipping each item to its own first line. Serializers emit "- " / computed "N. " via the handler's line prefix, indented a uniform three spaces per depth so ordered and bullet nesting round-trip in any combination; an empty item's line serializes bare — a marker-only line re-parses as a setext underline for the previous line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
toggleUnorderedList / toggleOrderedList / indentList / outdentList on both platforms (indent preserves the item's list type and clamps to the previous item's depth + 1; outdent at depth 0 removes the marker; toggling one list type over the other replaces it). Enter behavior is handler-driven via continuesOnNewline: a non-empty item continues as a same-type sibling at the same depth, an empty item exits the list. Tab indents and Backspace at an item's start outdents/un-lists, including at the document start. Android anchors an empty list line with a ZWSP so its marker draws and the caret indents (a LeadingMarginSpan cannot indent an empty paragraph); iOS drives an explicit empty-line marker through the layout manager and keeps typing attributes block-styled on empty block lines. Edits that change the line count re-stamp the whole document so ordered renumbering and the depth clamp propagate past the edited line. External plain-text paste parses as markdown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
onChangeState and the context-menu styleState gain unorderedList and
orderedList { isActive, depth }, matching the isActive shape of the other
entries and emitted only when the value changes. Example toolbar gains
bullet / numbered / indent / outdent buttons; INPUT.md and API_REFERENCE.md
document both list types; maestro flow + iOS baseline cover toggle,
continuation, and indent.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
acf1090 to
3f41cc2
Compare
hryhoriiK97
left a comment
There was a problem hiding this comment.
@hsource thanks again for the PR! I've left a few comments – can you address them, please?
- clamp list depth into [0, kENRMMaxListDepth] in the iOS parser and in ENRMBlockStore's metadata pass so deep markdown nesting can't over-index the ordinal counters - mirror input list markers for RTL paragraphs on iOS (bullet position and ".N" ordered labels, matching the readonly ListMarkerDrawer) and flip Android's input ordered marker to ".N" when dir < 0 - normalize the block store after newline continuation so continued ordered items renumber instead of sticking at "1." - changeListDepthBy: indents/outdents every selected paragraph (one range per line), mirroring Android's forEachSelectedLine - toggling unordered <-> ordered keeps each line's nesting depth (both platforms) - strip the empty-list-line ZWSP anchor from Android's onChangeText payload - replace the per-keystroke O(document) line-count scan with O(edit-size) newline detection over the inserted/replaced runs (Android's editTouchedNewline approach); _lastLineCount is gone entirely, which also removes the stale-count reformat after importMarkdown/pasteMarkdown - docs: rename "Bullet Lists" to "Lists", note items are single-line (loose items keep only their first line), listItemSpacing covers both list types - e2e: add ordered-list (continuation + indent/outdent renumbering) and UL<->OL toggle (depth kept) input flows - fix "two spaces" -> "three spaces" comment nit Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@hsource - I addressed the comments in a PR to your branch |
fix(input): address list-state review feedback from upstream software-mansion#545
|
I merged it in! @hryhoriiK97 - take a look |
| * LineHeightSpan. | ||
| * @default 0 | ||
| */ | ||
| listItemSpacing?: CodegenTypes.WithDefault<CodegenTypes.Int32, 0>; |
There was a problem hiding this comment.
This should be part of markdownStyle, not a separate prop. All other visual properties (colors, fonts, heading styles, spoiler styles) go through markdownStyle — having one list spacing property break that pattern will confuse consumers. Something like markdownStyle.list.itemSpacing would be consistent. That would also eliminate the applyComposedStyle() workaround on Android.
| * LineHeightSpan. | ||
| * @default 0 | ||
| */ | ||
| listItemSpacing?: CodegenTypes.WithDefault<CodegenTypes.Int32, 0>; |
There was a problem hiding this comment.
Should be Float, not Int32. All other spacing values in the library (marginTop, marginBottom, gapWidth, padding, etc.) are floats. This prevents sub-point values like 4.5 and is inconsistent with everything else.
| /** Display density (px per dp) so block handlers can build density-correct spans without a Context. */ | ||
| val displayDensity: Float = 1f, | ||
| /** Extra vertical spacing (px) above each bullet item, from the `listItemSpacing` prop. */ | ||
| val listItemSpacingPx: Int = 0, |
There was a problem hiding this comment.
Nit: these two don't really belong here — InputFormatterStyle is meant to be a pure style data class. displayDensity is a platform constant that could be passed to the BlockHandler at construction time or resolved from Context at the span-creation site. listItemSpacingPx goes away if listItemSpacing moves into markdownStyle.
| // Heading level of the cursor's paragraph: 0 = none, 1-6 = H1-H6. | ||
| heading: { isActive: boolean; level: number }; | ||
| unorderedList: { isActive: boolean; depth: number }; | ||
| orderedList: { isActive: boolean; depth: number }; |
There was a problem hiding this comment.
Worth documenting that depth is always 0 when isActive is false. The existing heading.level works differently (level 0 = no heading), so without a note here people will wonder about the semantics.
| if (!isTextChanging && !isProcessingTextChange) { | ||
| // The caret moving on/off an empty bullet line toggles the ZWSP anchor and the | ||
| // placeholder visibility; skip during a text-change pass (handled there). | ||
| syncEmptyListAnchor() |
There was a problem hiding this comment.
syncEmptyListAnchor() scans the entire document backwards on every selection change. For a short document that's fine, but on a large document with frequent caret moves this is O(n) per event. Could be worth short-circuiting early — e.g. skip the full scan when the document has no list blocks at all, or track ZWSP positions in a set instead of scanning.
| } | ||
|
|
||
| return markdownLines.joinToString("\n") | ||
| return markdownLines.joinToString("\n").replace(ZWSP, "") |
There was a problem hiding this comment.
The ZWSP anchor is stripped in 4 separate places: emitChangeText, two paths in serialize, and the empty-item skip on line 71. If a future code path emits text without stripping, the ZWSP leaks to JS. A single stripZWSP() call at the serialization boundary (or a shared helper all output paths funnel through) would be less fragile than scattering the strips.
|
|
||
| - (nullable ENRMBlockRange *)listBlockForParagraphAtPosition:(NSUInteger)position | ||
| { | ||
| NSString *text = _textView.textStorage.string; |
There was a problem hiding this comment.
This method linearly scans _blockStore.allRanges on every call — and it gets called from emitOnChangeState, updateEmptyBulletMarker, syncTypingAttributesWithCursorBlock, handleTextChanged, capturePreEditBlockForRange, changeListDepthBy, handleListKeyForReplacementRange, etc. Same pattern on Android with blockStore.allRanges.firstOrNull { ... } in 5+ places. A lineStart → BlockRange lookup map in BlockStore would make all of these O(1) and be a single-point change.
|
|
||
| - (void)drawGlyphsForGlyphRange:(NSRange)glyphsToShow atPoint:(CGPoint)origin | ||
| { | ||
| [super drawGlyphsForGlyphRange:glyphsToShow atPoint:origin]; |
There was a problem hiding this comment.
This entire method is a single 140+ line enumerateLineFragmentsForGlyphRange:usingBlock: callback doing paragraph lookup, attribute reads, RTL detection, baseline math, and drawing. It also allocates NSMutableSet<NSNumber *> and boxes NSNumber objects on every draw pass. Worth splitting into helpers (drawBulletMarker:…, drawOrderedMarker:…, resolveMarkerOrigin:…) for maintainability, and the per-frame allocations are worth watching for scroll perf in long documents.
| /// in-line replacement (autocorrect/paste) that wipes the line can heal it. | ||
| - (void)capturePreEditBlockForRange:(NSRange)range | ||
| { | ||
| _preEditBlockType = ENRMInputBlockTypeParagraph; |
There was a problem hiding this comment.
_preEditBlockType / _preEditBlockLevel are set here as instance variables and consumed later by reconcileBlockContinuationAfterNewlineAt:. If two rapid edits overlap (fast typing + autocorrect, or programmatic setValue during an edit), the pre-edit state may belong to the wrong edit. Android handles this inline in handleNewlineBlockContinuation using the post-edit text, which is safer — no window for staleness.
| }] mutableCopy]; | ||
| [self recomputeListMetadata]; | ||
| } | ||
|
|
There was a problem hiding this comment.
The "one block per paragraph" invariant relies entirely on removeBlocksOverlapping being called inside setBlock. There is no assertion or validation — if a code path mutates ranges without going through the standard toggle path, a line could end up with both a heading and a list item. An NSAssert / check() in normalizeToLineBounds that no two ranges share the same paragraph start would catch this early.
| /// paragraphs). Whether a type continues is the handler's continuesOnNewline. | ||
| - (void)reconcileBlockContinuationAfterNewlineAt:(NSUInteger)newlineLocation previousItemWasEmpty:(BOOL)previousWasEmpty | ||
| { | ||
| id<ENRMBlockHandler> handler = [_formatter handlerForBlockType:_preEditBlockType]; |
There was a problem hiding this comment.
When exiting a list (Return on empty item), this method directly strips NSParagraphStyleAttributeName via beginEditing/removeAttribute/endEditing. The formatter normally owns paragraph styles — it strips and applies them during applyBlockRanges:. This manual strip is a parallel path the formatter does not know about. If the formatter later expects its last-stamped ranges to still be present, it could leave stale styles or re-apply something that was just removed.
| pruneOrphanedAnchors() | ||
| handleNewlineBlockContinuation(editStart, deletedLength, insertedLength) | ||
| text?.let { blockStore.normalizeToLineBounds(it) } | ||
| applyPendingStyles(editStart, insertedLength) |
There was a problem hiding this comment.
This runs inside onAfterTextChanged and calls runAsATransaction { editable.delete(...) } — deleting characters, adjusting the block store, and moving the selection, all inside the callback responding to a text change. The isProcessingTextChange guard prevents infinite recursion, but modifying the Editable inside afterTextChanged is a known Android pitfall — it can interact badly with IME composition (Chinese/Japanese input), undo/redo, and accessibility services.
| // Switching unordered <-> ordered keeps each line's nesting depth. | ||
| val existing = blockStore.allRanges.firstOrNull { it.start == ls && it.type in BlockType.LIST_ITEMS } | ||
| blockStore.setBlock(type, existing?.level ?: 0, ls, le, editable) | ||
| } |
There was a problem hiding this comment.
This blockStore.allRanges.firstOrNull { it.start == ls && it.type in BlockType.LIST_ITEMS } pattern appears verbatim in at least 5 places: toggleListType, changeListDepthBy, handleListKey, syncEmptyListAnchor (twice). Each is a linear scan. iOS has listBlockForParagraphAtPosition: as a single method. Android should have the same — reduces duplication and makes a future O(1) lookup a one-line change.
hryhoriiK97
left a comment
There was a problem hiding this comment.
@hsource @wildseansy thank you! I've left a few additional comments.
What/Why?
This is a stub PR consisting of the rebased changes from #514 by @wildseansy. I created this because we can't wait to have this PR be merged in!
The commits consist of purely the following PRs, all by @wildseansy. I claim no credit for the code.
When rebasing, I resolved these conflicts manually:
Testing
Automated testing:
@wildseansy has already added
Manual testing:
Per https://github.com/software-mansion/react-native-enriched-markdown/blob/main/CONTRIBUTING.md
yarnfollowed byyarn prepareyarn react-native-example androidandyarn react-native-example iosto run on devicesiOS demo gif:
Android demo video:
Android.list.demo.mp4
PR Checklist