Skip to content

fix(walker): preserve attributes on <ul>/<ol>/<li> in markdown→storage#170

Merged
pchuri merged 1 commit into
pchuri:mainfrom
devlimits:fix/walker-preserve-list-attributes
May 5, 2026
Merged

fix(walker): preserve attributes on <ul>/<ol>/<li> in markdown→storage#170
pchuri merged 1 commit into
pchuri:mainfrom
devlimits:fix/walker-preserve-list-attributes

Conversation

@devlimits
Copy link
Copy Markdown
Contributor

Description

Addresses follow-up 1 from the #153 review (<ul> / <ol> missing renderAttrs).

<ul>, <ol>, and <li> were the only block-level elements in lib/html-to-storage.js that dropped their attributes. The <table> family already threaded renderAttrs(node.attribs) through, so the same plumbing is added here. For raw-HTML input via --input-format html, this means <ul class="task-list"> and <ol start="3"> now reach the storage payload with attributes intact instead of as bare <ul> / <ol>.

The <ul> / <ol> and <table> cases are kept as separate case blocks (rather than collapsed into one) to preserve the per-domain whitelist intent the rest of dispatchTag follows.

For <li>, the opening tag is hoisted into an open variable so both wrap and unwrap branches share it (same pattern as <th> / <td>).

Verification (Confluence Cloud)

Attribute Storage save Notes
class on <ul> / <ol> / <li> preserved
style on <ul> / <ol> / <li> preserved (normalized)
start on <ol> preserved
id, data-*, target, rel, type, value, reversed stripped server-side Safe to emit anyway

Also verified that class position on <li> vs the inner <p> does not affect storage preservation or rendered HTML — the current walker output shape (<li class="x"><p>content</p></li>) is fully accepted.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change
  • Documentation update

Testing

  • Tests pass locally with my changes (633 passing, was 630)
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes

4 new regression tests in tests/html-to-storage.test.js:

  • <ul> with class + style
  • <ol> with class + start
  • <li> with class on the wrap path
  • <li> with attribute on the unwrap path (block child suppresses <p> wrap)

Out of scope

Round-trip preservation. The storage walker strips list attributes on storage → markdown export, so <ul class> round-trip loses its class regardless. That side needs its own design call (markdown has no native attribute syntax).

`<ul>`, `<ol>`, and `<li>` were the only block-level elements in
`html-to-storage.js` that dropped their attributes — `<table>` /
`<thead>` / `<tbody>` / `<tfoot>` / `<tr>` / `<th>` / `<td>` already
threaded `renderAttrs(node.attribs)` through. For raw-HTML input via
`--input-format html`, this meant `<ul class="task-list">` and
`<ol start="3">` reached the storage payload as bare `<ul>` / `<ol>`,
silently losing list-rendering hints that Confluence's storage layer
otherwise preserves on save (verified that `class`, `style`, and `start`
on `<ol>` are accepted; other attributes are stripped server-side
regardless of what we emit).

The previous regex pipeline matched any `<ul>` / `<ol>` / `<li>` shape
without rewriting it, so attributes survived; the walker rewrites every
recognized tag and needed the same `renderAttrs` plumbing the table
family already had. Four regression tests cover `<ul>` class+style,
`<ol>` class+start, `<li>` class on the wrap path, and `<li>`
attributes on the unwrap path (block child suppresses the `<p>` wrap).
Copy link
Copy Markdown
Owner

@pchuri pchuri left a comment

Choose a reason for hiding this comment

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

Thanks for the clean follow-up to #153!

The fix mirrors the existing <th>/<td> open-variable pattern exactly, so the change reads as a natural continuation of dispatchTag rather than a new shape. Keeping <ul>/<ol> as separate case blocks (instead of folding them into the <table> family) is the right call — preserves the per-domain whitelist intent.

Verification table in the description and the four regression tests (wrap path, unwrap path, <ol start>, <li> with block child) cover the meaningful axes. No new attack surface since renderAttrs already runs through escapeAttrValue for double-quote handling.

Round-trip preservation being out of scope is the correct framing — that's a separate design call on the storage→markdown side.

LGTM.

@pchuri pchuri merged commit b5c172a into pchuri:main May 5, 2026
6 checks passed
github-actions Bot pushed a commit that referenced this pull request May 6, 2026
# [2.5.0](v2.4.0...v2.5.0) (2026-05-06)

### Bug Fixes

* **deps:** bump axios to ~1.15.2 to address security advisories ([#174](#174)) ([0a1492b](0a1492b)), closes [GHSA-w9j2-pv#6h63](https://github.com/GHSA-w9j2-pv/issues/6h63) [#173](#173)
* **walker:** preserve attributes on <ul>/<ol>/<li> in markdown→storage ([#170](#170)) ([b5c172a](b5c172a)), closes [#153](#153)

### Features

* add page version listing and purge commands ([#171](#171)) ([2bd5c37](2bd5c37))
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

🎉 This PR is included in version 2.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants