Repository navigation
fix(renderer): export live rendered content through shared markdown conversion - #1337
lanyue-llk wants to merge 10 commits into
Conversation
efe538b to
73c4a29
Compare
73c4a29 to
3fae227
Compare
3fae227 to
0476c31
Compare
0476c31 to
c98074f
Compare
c98074f to
a0ad007
Compare
Sequential Navigation Soak A/B✅ HEAD completed the 200-navigation resilience and memory observation. One browser process, one target, and one CDP session navigate CSDN → SegmentFault → Huaban → example.com repeatedly. This run issued 200 Common ancestor Session resilience
A public-page failure is reported separately from an unrecoverable session. The soak requires all navigation attempts, zero failed recovery, zero lifecycle/network ordering violations, and complete resource evidence. Process-tree memory
HEAD memory by 50-navigation quarter
Raw reports and per-navigation boundary samples: workflow run and full Public-site timing and memory are observational. Use the A/B deltas and trend shape as evidence, not as a deterministic performance threshold. |
Spider Bench A/B✅ All benchmark browser service runs completed; results are informational. Public HEAD: 184 / 240 rows; 38 / 48 sites produced rows; 10 unexpected empty sites. ✅ Deterministic fixture contract is clean. Common ancestor Public 48-site run · informational
Public-site content, timing, and memory are noisy. This single A/B run reports evidence only; site outcome counts explain missing rows without treating them as a deterministic regression. CPU and memory timelinesThese are bounded 40-point views of the same complete process-tree samples used by the HTML report. CPU uses 100% per occupied logical core; memory lines are RSS first and PSS second when complete PSS samples are available. Basexychart-beta
title "Base CPU"
x-axis "Elapsed seconds" [1.034, 2.534, 3.535, 4.537, 5.038, 6.037, 7.538, 8.54, 10.043, 11.045, 12.548, 13.55, 15.052, 16.054, 17.556, 18.555, 20.055, 21.056, 22.559, 23.56, 24.06, 24.561, 26.061, 27.061, 28.563, 29.564, 31.066, 32.068, 33.569, 34.57, 36.07, 37.071, 38.573, 39.574, 41.074, 41.575, 42.075, 43.578]
y-axis "CPU percent" 0 --> 200
line [12, 112, 105.92, 129.66, 139.85, 134.08, 136.01, 61.85, 139.83, 103.76, 143.7, 107.77, 2, 0, 4, 4, 90, 25.98, 83.83, 105.97, 103.93, 107.84, 123.79, 131.99, 129.93, 101.92, 101.88, 101.92, 81.8, 122.02, 4, 51.97, 0, 13.98, 101.9, 161.79, 11.99, 121.84]
xychart-beta
title "Base memory: RSS then PSS"
x-axis "Elapsed seconds" [0.034, 1.034, 2.534, 3.535, 4.537, 5.038, 6.037, 7.538, 8.54, 10.043, 11.045, 12.548, 13.55, 15.052, 16.054, 17.556, 18.555, 20.055, 21.056, 22.559, 23.56, 24.06, 24.561, 26.061, 27.061, 28.563, 29.564, 31.066, 32.068, 33.569, 34.57, 36.07, 37.071, 38.573, 39.574, 41.074, 41.575, 42.075, 43.578, 44.145]
y-axis "Memory MiB" 0 --> 500
line [13.11, 71.38, 105.19, 120.44, 150.8, 163.38, 176.65, 146.11, 124.7, 144.2, 141.66, 135.45, 119.63, 122.78, 121.74, 121.71, 121.73, 133.86, 152.91, 149.96, 207.65, 266.07, 190.67, 189.06, 149.54, 150.13, 172.25, 137.32, 141.06, 145.92, 186.96, 134.95, 143.73, 142.15, 135.6, 153.61, 157.23, 127.3, 172.48, 0]
line [9.73, 67.7, 101.51, 116.99, 147.16, 159.62, 172.91, 140.5, 121.14, 140.51, 137.89, 132.03, 115.79, 119, 117.9, 117.92, 117.94, 130.11, 149.19, 146.62, 203.97, 262.7, 185.77, 185.37, 145.82, 146.39, 168.47, 133.54, 136.97, 142.17, 183.18, 131.17, 140.41, 138.37, 132.52, 149.94, 153.45, 123.52, 168.77, 0]
HEADxychart-beta
title "HEAD CPU"
x-axis "Elapsed seconds" [1.535, 2.536, 4.039, 5.039, 6.539, 7.541, 9.043, 10.043, 11.545, 12.547, 14.048, 15.55, 16.552, 18.054, 18.555, 19.055, 20.556, 21.556, 23.058, 23.558, 24.06, 25.561, 26.563, 28.065, 29.065, 30.568, 32.071, 33.071, 34.573, 35.575, 37.076, 38.077, 39.577, 40.577, 42.08, 43.081, 43.582, 44.082]
y-axis "CPU percent" 0 --> 200
line [87.84, 103.85, 95.76, 118.02, 133.94, 129.73, 95.93, 134.03, 133.73, 29.94, 23.99, 59.92, 47.9, 46.01, 0, 27.96, 31.99, 114.05, 97.77, 105.88, 113.71, 125.81, 135.77, 138, 97.94, 75.8, 89.75, 7.99, 109.97, 113.91, 89.95, 7.99, 64.01, 128, 47.89, 103.85, 171.81, 127.79]
xychart-beta
title "HEAD memory: RSS"
x-axis "Elapsed seconds" [0.033, 1.535, 2.536, 4.039, 5.039, 6.539, 7.541, 9.043, 10.043, 11.545, 12.547, 14.048, 15.55, 16.552, 18.054, 18.555, 19.055, 20.556, 21.556, 23.058, 23.558, 24.06, 25.561, 26.563, 28.065, 29.065, 30.568, 32.071, 33.071, 34.573, 35.575, 37.076, 38.077, 39.577, 40.577, 42.08, 43.081, 43.582, 44.082, 44.088]
y-axis "Memory MiB" 0 --> 500
line [13.44, 92.57, 105.93, 135.24, 150.84, 136.49, 134.19, 133.92, 151.02, 138.37, 114.56, 121.56, 137.97, 136.25, 121.09, 121.12, 126.83, 155.21, 138.84, 224.25, 279.51, 175.53, 191.07, 214.08, 150.41, 167.82, 134.98, 145.11, 129.81, 171.92, 242.86, 239.81, 148.04, 136.16, 163.62, 135.1, 149.82, 171.93, 0, 0]
Deterministic fixture · required contractThe fixture exercises 15 routes: 8 are expected to emit rows and 7 intentionally exercise empty, loading, or timeout behavior.
Full HTML, JSON, CSV, logs, and page snapshots: workflow run and full The public-web A/B is informational. The exact HEAD fixture contract runs as a separate required CI check; timing and public-site content deltas remain non-blocking. |
CI Regression ReportSource CI run · source state at render:
Release regression — ✅ HEAD/base failures 0/0; raw binary +0.023974%Package and image size
Startup latency and PSS
Concurrency matrix
Frontend differential — ❌ 1,008/1,020 cases matched; 12 issues
Recovered Chromium reference infrastructure failures
First failing cases
Agent episodes · Moli vs Chromium — ✅ 8/8 Moli episodes passed; 0 failures
Runtime and CDP session contracts — ✅ 26 contract cases; 0 failures
CDP smoke — ✅ 52/52 groups passed; 621 scenariosWorkers: 4 · cumulative group time: 201.04 s · failed groups: 0
WebMainBench · 545 pages — ✅ 545/545 completed; 0 unexpected failures
Frozen HTML · Linux · Diagnostics: All artifact fields are parsed by the trusted default-branch renderer; missing or invalid inputs remain visible as unavailable. |
a0ad007 to
b7f4f45
Compare
b7f4f45 to
3d68030
Compare
3d68030 to
1acd30d
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Re-review of the updated version (3fb74bccb)
This PR makes the renderer's Markdown export read live state: computed display/visibility/baseline-shift, own-visibility provenance from Stylo, live form values and checkedness, and base-resolved URLs. It does this through a MarkdownDom adapter over the shared moli-html2md converter. This revision addresses every major point from the last round. Disclosure sibling filtering now uses only display:none ancestors. Styles are read once per dump. The font-color scope creep is gone. The Stylo pin now sits directly on main's 973988b9 with a 14-line diff, the head merges cleanly with origin/main, and #1592 is now stacked on this branch and extends the same adapter. Verdict: comment, close to approve. The remaining items are minor: the Stylo pin points at an unmerged fork PR, intermediate commits do not build, one disclosure path is inconsistent, and there are a few nits.
Previous findings
| # | Finding | Status | Evidence |
|---|---|---|---|
| 1 | [major] visibility-hidden ancestor turns disclosure path into sibling filter | FIXED | hidden_nodes is display == "none" only (visibility.rs:83-86). New test markdown_document_export_distinguishes_source_from_hidden_interface puts body{visibility:hidden} next to an aria-controls section and keeps both intro and conclusion; it would fail on the old code (old hidden_nodes put body on the path). |
| 2 | [nit] redundant id-based disclosure clause / last-wins ids / role tokens | FIXED | Clause removed (L149-151); targets.entry(id).or_insert is first-wins (L62-66); has_role splits tokens (L334-338). The duplicate-id test fails without first-wins. |
| 3 | [minor] ≤1.5px tracking-pixel heuristic | FIXED | Rule and width/height sampling removed; the test keeps a 1px image. |
| 4 | [minor] any visible code/pre bypasses hidden rules |
FIXED | Source regions now start only at pre (L198-203). Hidden inline code content and script/copy button inside pre are asserted omitted. |
| 5 | [minor] display keyword lists miss table/ruby boxes | PARTIAL (enum pushback accepted) | Table-internal boxes added and tested. The three ruby* arms cannot match, though: Stylo parses those keywords only under feature = "gecko", and Moli builds the servo feature. Multi-keyword list-item serializations are also not covered. See the inline nit. |
| 6 | [minor] URL-attribute list diverges from html2md | FIXED | moli_html2md::resource_url_attributes is consumed by the adapter (L240-264) and covers source/srcset/data-original on media; tested for video/audio/iframe. Nit: the inventory repeats LAZY_SOURCE_ATTRIBUTES literally. |
| 7 | [minor] second full style pass | FIXED | One computed_style_export_values_for_document_snapshot call returns values and the own-visibility flag from the same observation.read (styles.rs:554-580); the old two-pass API is gone. |
| 8 | [minor] color invalidation / <font color> scope creep |
FIXED | presentation.rs and mutation_effect.rs are no longer in the diff. |
| S1 | [major] Cargo conflict with main / reverting main's Stylo; dead opacity code in pin | FIXED | git merge-tree --write-tree origin/main refs/review/new-1337 is clean. Stylo e2baa14 is a descendant of main's 973988b9, and git diff 973988b9 e2baa14 touches only cascade.rs (+5) and computed_value_flags.rs (+9). Taffy and the other main updates are kept. |
| S2 | [major] overlap/contradicting contracts with #1592 | FIXED | #1592 is now based on codex/content-rendered-state and extends this MarkdownDom with per-document bases, first-wins (document, id) targets and frame roots, using the same URL inventory. Only one adapter remains. |
| S3 | [minor] style inventory computed twice | FIXED | Same as #7. |
| S4 | [minor] scope creep font color | FIXED | Same as #8. |
| S5 | [minor] document export heuristics as policy | PARTIAL | The paint/size guesses are gone, but the remaining rules (aria-controls + section/article/details/tabpanel/region targets, uncontrolled tabpanel, hidden=until-found, nav/header/footer/menu/dialog exclusion, root veil, pre source retention) are described only in the PR body and in short code comments. Neither the README nor the #1380 docs covers them. |
| S6 | [nit] long minified test scenarios | NOT FIXED | markdown_dump.rs still packs scenarios into 6 tests, with 87 lines over 100 columns. The root-veil + accordion case was added, as requested. |
| S7 | CI | n/a | Format, Clippy, CDP smoke and harness checks pass. Nextest was still running and the release builds were queued at review time. |
New findings
- [minor] The Stylo pin points at the head of an open fork PR (lexmount/stylo#2). Its branch history adds and then reverts opacity code, and its title is stale. (Cargo.toml)
- [minor] An uncontrolled
role=tabpanelorhidden=until-foundroot inside adisplay:noneancestor is lost, while anaria-controlstarget in the same place is kept. (visibility.rs:151) - [nit] Ruby display arms are unreachable in the servo build, and multi-keyword
list-itemvalues are not classified. (visibility.rs:226) - [nit]
resource_url_attributesrepeats the lazy-source names instead of reusingLAZY_SOURCE_ATTRIBUTES. (media.rs:16)
Findings not tied to a diff line
- [minor] Commits 1-4 do not compile on their own.
8ca089c70,356adafe9,d75a11a22and1bd79a738all referenceComputedValueFlags::HAS_OWN_VISIBILITY(style_engine/computed.rs), but Cargo.toml in those commits still pins Stylo973988b9, which lacks the flag. The bump only arrives inc08ba7c3b. Commit 1 also adds about 700 lines of paint heuristics (opacity, final-keyframe opacity, color, framework attributes) that commit 2 deletes.git bisectbreaks across the stack and the history misrepresents the design. Fix: squash-merge, or squash into one commit that carries the Stylo bump. Also add #1592 to the stack list in the PR body, since it now depends on this branch. - [nit] Fixture location and unrelated html2md edits.
moli-html2md/tests/fixtures/hacker-news-layout-computed.mdis consumed only bymoli/tests/fetch_cli/markdown.rs, not by any moli-html2md test, so a converter crate now stores a renderer expectation. Move it undermoli/tests/fixtures/or name the owner. The doc-comment rewrite ofhtml_fallback::needed(L5-7) belongs to the tables PR (#1336).
Automated review by Claude Code.
Stack-level review (#1334–#1337)Scope. This review covers only problems that come from how the layers interact, or that exist only in the combined state. Each layer's own review is posted separately. Refs reviewed: Summary
1. High: computed UA
|
| Pass | Where | Introduced |
|---|---|---|
Full-DOM collect_node_ids, including <head> and display:none subtrees |
page_dump.rs:17-23 |
#1337 |
| One computed-style read per element (3 serialized strings each) | page_dump.rs:24-45 |
#1337 |
6 linear passes over the element vector, plus parent-chain walks per disclosure root, plus an ancestor walk (content_context) per tabpanel or aria-controls target, which is O(n·depth) on tab-heavy pages |
visibility.rs:53-109,88-98,150,294-332 |
#1337 |
| Anchor prepass: 1–2 full walks | anchors.rs:46,62 |
#1336 |
| Main conversion walk | machine.rs |
main |
Per table: has_data_semantics + needed + anchors::contains + from_dom + (has_cells or render) = 3–5 subtree walks |
machine.rs:427-499 |
#1336 |
Per pre/code: subtree_has_link + anchors::contains (+ render) |
machine.rs:355-378 |
#1336 |
Slow path (--strip/--with-base/--with-frames): DOM clone plus strip walk |
page_dump.rs:59-74 |
main |
Every Dom query also goes through 2–5 HashSet/HashMap probes in the adapter (visibility.rs:358-402), and html2md makes many queries per node (has_block_layout, has_text_boundary, node_kind, serialized-state check, anchor targets, primary_alternative, fallback_text).
Fix. Build the adapter state during the pre-order collection itself, restricted to the body subtree when the root is body. Store a per-node flags struct in one HashMap<NodeId, Flags> rather than seven sets. Have the adapter compute referenced fragment targets once and hand them to html2md, which removes both anchor walks. Cache the table classification from one subtree walk.
9. Low: the fragment-link skip uses a different predicate (#1337 vs #1336)
visibility.rs:57-59 sets fragment_links for any href starting with #, including href="#", and it scans every element, suppressed ones included. anchors.rs:47-50 counts only non-empty fragments. Nearly every real page has an href="#" or a skip link, so may_have_fragment_links (anchors.rs:39) almost never avoids the prepass.
Fix. Use the same predicate (strip_prefix('#') and non-empty), or remove the hook in favour of passing targets in (see #8).
10. Low: inconsistent output rules across helpers
rolematching uses five rules. First token only:content.rs:10,42. Whole value, ASCII-case-insensitive:content.rs:100. Whole value with Unicodetrim():machine.rs:428-433,html_fallback.rs:63-67. Whole value with HTML-space trim:table.rs:29-32. Any token:visibility.rs:334-338. As a result,role="presentation none"counts as presentational for labels but not for tables.- URL policy differs by element. Media uses an allowlist (
media.rs:251-267), links use a denylist (media.rs:307-319), and the fallback applies both by attribute name (html_fallback.rs:304-306). If that split is intentional, document it. - The attribute inventory is copied by hand.
resource_url_attributes(media.rs:11-32, fix(renderer): export live rendered content through shared markdown conversion #1337) repeatsLAZY_SOURCE_ATTRIBUTES(media.rs:6, fix(html2md): preserve media resources and readable controls #1334) as literals and also resolvessource[srcset], which html2md never reads (media.rs:62-69reads onlysrcand the lazy attributes). Derive it from the const. - Labels follow three separate rules.
content::fallback_text(aria-label, then title),media::label(title only, then an English default) andform::input_text(value, then aria-label, then title).
Fix. Write one role_tokens() helper and one documented URL policy module, and derive the inventory from the constants the converter reads.
11. Low: stack hygiene
- Empty commits:
8663d9a89and48001fb55(fix(html2md): preserve media resources and readable controls #1334),a246449cband77af093d0(fix(html2md): preserve mathematical and inline formatting semantics #1335). 37ce36a70"fix(ci): validate standalone formatting layer and transport retries" only moves a test struct inreview_semantics.rs.2757784a9(fix(html2md): preserve media resources and readable controls #1334) changesmoli/tests/fetch_cli/http_fail.rs, ande9dccde1ereverts it; the net change is zero.8ca089c70(fix(renderer): export live rendered content through shared markdown conversion #1337) adds about 1.4k lines acrosshost_environment.rs,style_engine/{computed,mod,mutation_effect}.rsandmoli-selector/src/stylo/presentation.rs, most of which356adafe9andc08ba7c3bremove again.- Commits that belong elsewhere: see ci: add manual release workflow #5 and Fix parent strut participation in inline layout #7.
- The stack is based on the current
origin/maintip, andgit merge-tree origin/main new-1337is clean.
Fix. Drop the empty commits and squash the add-then-revert pairs before merging.
12. Info: overlapping PRs
- fix(markdown): serialize the current iframe document in place #1592 (open, base
codex/content-rendered-state, so it is stacked on fix(renderer): export live rendered content through shared markdown conversion #1337).merge-tree new-1337 new-1592is clean. It rewrites most ofMarkdownDom::new: per-documenttargets,document_bases: HashMapin place of the singlebase_url, and a generalized root veil (visibility.rs+97/−65,page_dump.rs+182). It also adds another URL and label attribute list (frame_document_has_content). fix(renderer): export live rendered content through shared markdown conversion #1337's single-document adapter API therefore lasts one PR. Consider landing them back-to-back. Either way, Fix dead link in README #1, ci: partially migrate to GitHub-hosted runners #2 and Intrinsic sizing refactor #8 apply to fix(markdown): serialize the current iframe document in place #1592's version too. - fix(html2md): preserve visible labels and literal preformatted text #1591 (closed).
merge-tree new-1334 new-1591andmerge-tree new-1337 new-1591both conflict inmachine.rs,writer.rsandtests/regressions.rs. It adds a third URL helper (resource_url) and its ownvideo/audio/sourcehandling, which duplicates fix(html2md): preserve media resources and readable controls #1334'smedia.rs. Its linkaria-labelfallback is covered bycontent::fallback_text. For links inside<pre>, fix(html2md): retain complex tables and referenced fragments #1336 uses the HTML fallback where fix(html2md): preserve visible labels and literal preformatted text #1591 used post-fence links. It is superseded and should not be reopened as is.
Automated review by Claude Code.
3fb74bc to
f5e6630
Compare
f5e6630 to
f04b0b1
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Round-3 re-review of the updated version (f04b0b151)
Re-review of the updated version (round 3, head f04b0b151, base #1336 9c416cf06). This PR makes the renderer's Markdown export read live state (computed display/visibility/baseline-shift, Stylo own-visibility provenance, live form values, base-resolved URLs) through a MarkdownDom adapter over moli-html2md. The only new commit, f04b0b151, fixes all four round-2 inline findings. It does not change the three main stack-level problems (#1–#3 in the stack review) that land in this PR. They are still reproducible by tracing the code, so this round is final and lists everything that is still open.
Verdict: request changes. Two major output regressions remain on the default live path: every list is emitted with blank lines between items (looks loose), and a hidden table cell moves later cells under the wrong headers. Neither has a live-path test.
Previous findings
| # | Finding | Status | Evidence |
|---|---|---|---|
| R2-1 | [minor] Stylo pin on an unmerged fork PR | Pushback accepted, merge-order item remains | stylo#2 is retitled "record own visibility after cascade". refs/pull/2/head keeps e2baa14 fetchable by full SHA. Remaining: stylo#2 is still OPEN, so land it before or with this PR. |
| R2-2 | [minor] uncontrolled tabpanel / hidden=until-found under display:none lost |
FIXED (one regression, see new #4) | They join disclosure_roots before paths are built (visibility.rs:83-89). The new markdown_nested_disclosures_and_resource_urls test fails without this. But content_context does not exclude the native <dialog> element, so a tabpanel inside a closed <dialog> is now exported. |
| R2-3 | [nit] dead ruby arms / multi-keyword list-item |
FIXED | Classification now uses the outer type (L208-215), and the ruby arms are gone. The new unit test covers flow-root list-item, inline list-item and contents. |
| R2-4 | [nit] lazy names repeated in resource_url_attributes |
FIXED | The list is now chained from LAZY_SOURCE_ATTRIBUTES (media.rs:11-28). Small nit about the duplicated tag list, see new #7. |
| R1-5 | [minor] display keyword lists | FIXED | Superseded by R2-3. |
| R2-S5 | [minor] export policy documented only in PR body | PARTIAL | README:105-107 adds one sentence. The rules themselves (aria-controls target kinds, uncontrolled tabpanel, until-found, nav/header/footer/menu/dialog exclusion, root veil, pre retention) are still not documented. |
| R2-S6 | [nit] minified test scenarios | NOT FIXED | The new test adds more lines over 100 columns (markdown_dump.rs:449-450). |
| R2-N1 | [minor] commits 1–4 do not compile | NOT FIXED | range-diff shows commits 1–4 unchanged. 229809ab0…a13955ea4 use HAS_OWN_VISIBILITY with Stylo 973988b9. 229809ab0 still adds about 1.4k lines that later commits delete. |
| R2-N2 | [nit] computed fixture under moli-html2md/tests/fixtures; needed doc rewrite belongs to #1336; #1592 missing from stack list |
NOT FIXED | The fixture is still consumed only by moli/tests/fetch_cli/markdown.rs. d0d841ea0 still rewrites html_fallback.rs:7-8. The PR body stack line still ends at #1380. |
| Stack #1 | [major] UA display reaches has_block_layout, so lists look loose and options get blank lines |
NOT FIXED | L209-214 is now broader: every non-inline display, including UA li/ul/option/optgroup, goes into blocks. See inline. |
| Stack #2 | [major] hidden element → NodeKind::Document breaks tables |
NOT FIXED | visibility.rs:347-351 unchanged; table.rs add_row _ => return None. See inline. |
| Stack #3 | [minor] UA inline-block controls get a boundary; Markdown writes \n, fallback writes a space |
NOT FIXED | L210-211 still matches input/button/select/textarea through the UA inline-block, and the new unit test now asserts that inline-block produces \n. |
| Stack #4 | [minor] README contract out of date | NOT FIXED (README edited here) | README:5 "implements the interface directly", :19 "four borrowed queries" (the trait has eight), :44-45 "never rescan cell descendants". |
| Stack #5 | [minor] #1336 fix inside #1337 | PARTIAL | The textarea allowed change moved to #1336 (fixed). The resource_url_attributes html2md API is still added inside renderer commit d0d841ea0. |
| Stack #6 | [minor] production adapter barely tested | NOT FIXED | render_markdown_document is still a style-less #[cfg(test)] path, and CLI tests still use data: URLs. There is still no live list, select or control test. |
| Stack #7 | [minor] Stylo bump inside an unrelated commit | NOT FIXED | Still in d0d841ea0. Moot if squash-merged. |
| Stack #8 | [minor] passes per conversion | NOT FIXED | Unchanged, and f04b0b151 adds a 7th styles pass. See inline. |
| Stack #9 | [low] fragment-link predicate differs | NOT FIXED | visibility.rs:57-59. See inline. |
| Stack #10 | [low] URL inventory resolves inputs html2md never reads | NOT FIXED | source srcset/data-srcset (media.rs:14) are resolved, but sources() reads only src and lazy attributes on <source>. |
| Stack #11 | [low] hygiene | NOT FIXED | Same as R2-N1 and Stack #7. |
New findings (all inline)
- [major] Live lists and
<select>options get blank lines from UAdisplay, and the fallback writes<div><option>(visibility.rs:213). Stack #1, still open. The fallback<div>part was missed earlier. - [major] A hidden table cell shifts later cells under the wrong headers (visibility.rs:350). Stack #2, still open.
- [minor] UA
inline-blockcontrols become newlines in Markdown,<br>in GFM cells, and spaces in fallback HTML (visibility.rs:211). Stack #3, still open. - [minor] New regression from the R2-2 fix: an uncontrolled tabpanel inside a closed native
<dialog>(or an unopened[popover]) is now exported (visibility.rs:88). - [low]
fragment_linkspredicate (visibility.rs:58). Stack #9, still open. - [minor] Live adapter not covered by unit/CLI tests (page_dump.rs:229). Stack #6, still open.
- [nit] Duplicated tag list and unread
sourcesrcset inresource_url_attributes; misnamednames(media.rs:19). - [nit] README stale lines and unwrapped line (README.md:107). Stack #4, still open.
- [minor] Per-conversion passes (page_dump.rs:19). Stack #8, still open.
Not tied to a diff line
- Commit hygiene (unchanged): commits 1–4 do not build.
229809ab0adds anda6fd0bdd1/d0d841ea0removepresentation.rs/mutation_effect.rs/host_environment.rspaint code. The Stylo bump and the html2mdresource_url_attributesAPI sit in a renderer commit. Fix: squash-merge, or squash into one commit with the Stylo bump first and the html2md API in its own commit. - Merge state:
git merge-treeagainst #1336 and against currentorigin/main(91b72e91b) is clean. The stack no longer sits on theorigin/maintip, but nothing conflicts. - CI at review time: Format, CDP smoke, harness tests and SecTrust pass. Clippy is in progress, and Nextest and both release builds are queued. The PR body claims 30 page-export tests pass, but none of them covers a list, a
<select>or a hidden table cell on the live path.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
Stack-level re-review (#1334–#1337), round 3Scope. This round covers only cross-layer and combined-state problems. Per-layer issues are left to the per-PR reviews. Refs: What changed since round 2: five new commits ( Status of the round-2 findings
1. Lists and
|
| # | Severity | PR | Location | Summary |
|---|---|---|---|---|
| N1 | Medium | #1334 | moli-renderer-v8/src/native_bridge/element/images/src.rs:776-796, moli-html2md/src/media.rs:90-151 |
An html2md commit replaces the renderer's spec srcset parser, which drives web-observable currentSrc, with a #[doc(hidden)] html2md helper that handles parentheses differently |
| N2 | Medium | #1336 × #1337 (missed in round 2) | html_fallback.rs:186-197 |
Computed display boundaries are inserted inside <pre>/<code> HTML fallback and change source text; the raw code path ignores them |
| N3 | Low | #1334 × #1337 (missed in round 2) | visibility.rs:130-143, form.rs:26-41 |
Live input values print sanitized defaults (50, #000000) for untouched range and color inputs, replacing their accessible names |
| N4 | Low | #1337 (f04b0b151) |
visibility.rs:208-215 |
The catch-all classifier makes UA table-column/table-column-group blocks, so fallback tables gain <div>s inside <table> |
N1. Medium (#1334): renderer image selection now depends on html2md's srcset parser
54c1d1cb7 ("fix(html2md): keep inline media flow and accessible alternatives", #1334) deletes the renderer's tokenizer and descriptor validator (−220 lines in images/src.rs). parse_srcset_candidates (images/src.rs:776-796) now wraps moli_html2md::parse_srcset. That function is used by selected_srcset_candidate (:688-760), so it decides img.currentSrc and which image is fetched. A Markdown converter's #[doc(hidden)] helper now sets page-visible browser behaviour, and the change sits in a PR and commit titled html2md.
The two parsers differ. The deleted state machine followed HTML's "parse a srcset attribute": ( enters in-parens and the first ) leaves it, with no nesting. media.rs:124-133 counts nesting depth instead.
- Input:
<img src="fallback.png" srcset="a.png ((x), b.png 2x">. - Before:
(,(,)returns to in-descriptor, and,ends candidate 1."((x)"is invalid and is dropped, thenb.png 2xis a valid candidate, socurrentSrcisb.png. - Now: the depth goes 2 → 1 and the
,does not terminate, so the descriptor runs to end of string:"((x), b.png 2x".descriptor_scorereturnsNoneand no candidate is left, socurrentSrcfalls back tofallback.png.
The remaining renderer tests (images/src.rs:1083-1135) do not cover nested parentheses. Every future html2md tweak to parse_srcset (it already carries a Markdown-only score) now changes browser behaviour too.
Fix. Keep the spec parser in the renderer, or move it to a neutral shared crate with its own WPT-derived tests. Either way, put that change in a renderer commit with a renderer title, not inside an html2md commit. If the parser is shared, use the spec's non-nesting paren state.
N2. Medium (#1336 fallback × #1337 adapter, missed in round 2): computed boundaries rewrite linked code
html_fallback::render computes code (html_fallback.rs:189) but applies has_block_layout and has_text_boundary (:190-197) whatever its value. Inside a <pre>/<code> rendered through the fallback (any linked or targeted code block, machine.rs:349-374), an inline-block span inserts a literal space before and after itself into the source. A display:block span or code (code is not in has_block_tag) inserts <div>…</div>.
- Input:
<pre><code>let v = <span style="display:inline-block">f</span>(<a href="/doc">x</a>);</code></pre> - Live:
<pre>\n<code>let v = f (<a href="/doc">x</a>);</code></pre>, which adds two characters that are not in the source. - Source-only, or the same block without the link (raw path
machine.rs:375-389, which never callsvisit):let v = f(x);
The text of a code block therefore depends on whether it happens to contain a link. Per-line highlighters that style lines as display:block/table-row (with gutters as table-cell/inline-block) produce <div> wrappers inside <pre> as well.
Fix. In render, skip both computed-boundary hooks while code is true, matching the raw path. If a CSS-only line break inside pre must be kept, emit a literal \n only for a block boundary, never a space.
N3. Low (#1334 html2md × #1337 adapter, missed in round 2): sanitized input defaults leak into the live dump
The adapter overrides value with element.input_value() for every HTML input (visibility.rs:138-143). input_value() returns the control's sanitized value, which is non-empty even when no value attribute exists: range gives the midpoint (moli-dom/src/forms/input_type.rs sanitize_range_value) and color gives #000000 (sanitize_color_value). form::input_text (form.rs:26-41) prefers any non-empty control_value over aria-label/title.
- Input:
<label>Volume <input type="range" aria-label="Volume level"></label> <input type="color" title="Theme"> - Live:
Volume\n50\n#000000(UA inputs are alsoinline-block; see Fix flex max-content text wrapping precision #3) - Source:
Volume Volume level Theme
A browser shows a slider and a swatch, not those strings.
Fix. In html2md, treat range and color like the other non-textual controls (accessible name only), or have the adapter supply control_value only for text-entry types or when input_value_dirty().
N4. Low (#1337, f04b0b151): the catch-all classifier adds <div>s inside fallback tables
The negative match at visibility.rs:209 now also classifies UA col (table-column) and colgroup (table-column-group) as blocks, and the old allow-list excluded both. Neither is in has_block_tag, so html_fallback.rs:191-193 wraps each one in <div>. A fallback table with <colgroup><col><col></colgroup> emits <table>\n<div><div></div><div></div></div>\n<thead>…. Browsers foster-parent those <div>s out of the table, but the output is no longer valid table HTML. -webkit-inline-box, if Stylo parses it, is likewise sent to blocks, because it does not start with inline.
Fix. Use an explicit allow-list again, adding the outer display types f04b0b151 meant to cover (flow-root list-item, inline list-item), or exclude table-column*.
Recommended merge gate
#1 and #2 are still output-correctness regressions on the default moli fetch --dump markdown path, so they should block #1337. N1 should be resolved before #1334 merges, because it changes browser behaviour from a PR titled as html2md-only. N2 and #3 can land together, since they share the boundary-mapping fix. The rest can follow.
Automated review by Claude Code.
f04b0b1 to
9272068
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Round-4 re-review of the updated version (92720683b)
Round-4 re-review (head 92720683b, base #1336 99e374401). This PR makes the renderer's Markdown export read live state (computed display/visibility/baseline-shift, Stylo own-visibility, live control values, base-resolved URLs) through the MarkdownDom adapter over moli-html2md. The new commit 92720683b and the #1336 commit 68cc9e546 beneath it fix both blocking stack findings. Live lists stay tight and <select> options stay on single lines, because content::owns_boundaries makes the converter ignore CSS boundaries on elements it already structures. A visibility:hidden cell keeps its <td>, so later columns no longer shift. The new markdown_live_structure_and_visibility_have_exact_outputs test fails on the round-3 code for each of these cases. I traced the code by hand and did not build or run anything.
Verdict: request changes, small. The two majors are gone. One minor regression came in with the hidden-cell fix: a hidden table that needs the HTML fallback now emits an empty <table> skeleton. Two minor/medium stack items are also still open: CSS boundaries inside fallback <pre>/<code> (#1336 layer) and loose lists from block-styled item children, which the new README sentence says cannot happen. Everything else is low or nit, or is stack hygiene that a squash merge removes.
Previous findings
| # | Finding | Status | Evidence at 92720683b |
|---|---|---|---|
| R3-1 / stack #1 | [major] UA display makes lists loose and splits <select> |
FIXED | machine.rs:216-219 and html_fallback.rs:192-198 skip both hooks when owns_boundaries(tag) holds (is_structural_block + select/optgroup/option; content.rs:140-142). <ul><li>a</li><li>b</li></ul> → - a\n- b; nested list and select/optgroup are exact-tested (markdown_dump.rs:477-479). No more <div><option>. A residual looseness case is in new #2. |
| R3-2 / stack #2 | [major] hidden element → NodeKind::Document shifts table columns |
FIXED (one regression, new #1) | visibility.rs:358-382 keeps table/list tags. Text under an invisible parent is Other (L383-390). …<td style=visibility:hidden>private</td>… → | A | | alice | (exact test L482). |
| R3-3 / stack #3 | [minor] UA inline-block controls → \n in Markdown, <br> in cells, space in fallback |
FIXED | The adapter skips native controls whose display is exactly inline-block (L213-218). html2md maps has_text_boundary to a space in both paths (Task::InlineSpace, machine.rs:80/221). Search q Go and | Alice Edit | are exact-tested. The Font Awesome link split is gone too, because text(" ") does not close styles. Design nit in new #6. |
| R3-4 | [minor] tabpanel inside closed <dialog>/[popover] exported |
FIXED | content_context rejects the dialog element and any popover attribute (L289-292). The exact test omits both panels. |
| R3-5 / stack #9 | [low] fragment-link predicate | FIXED | Requires a non-empty fragment (L57-62), matching anchors::referenced. Head links are out of scope now. |
| R3-6 / stack #6 | [minor] production adapter barely tested | FIXED | 8 exact live cases × strip on/off (markdown_dump.rs:471-492), plus an HTTP FixtureServer CLI test with base-resolved URLs (fetch_cli/markdown.rs:75-95). Pushback accepted on keeping the source-only unit helper. |
| R3-7 / stack #10 | [nit] duplicated tag list, unread source srcset, names |
FIXED | One (standard, lazy) match (media.rs:12-18). source advertises src and lazy inputs only. Renamed to tag. |
| R3-8 / stack #4 | [nit] README stale; R2-S5 export policy undocumented | FIXED (wrap nit remains) | L5/L20/L48 corrected, and the policy paragraph is added at L163-169. L5 (97 cols) and L20 (99 cols) are still unwrapped, and L169 overclaims (new #2). |
| R3-9 / stack #8 | [minor] many full passes per conversion | PARTIAL, pushback accepted for the rest | Styles are now collected for <html> + body subtree only (page_dump.rs:19-27). The 7 sets and per-panel ancestor walks remain. Accepted as not worth a redesign without a measured bottleneck. |
| R2-1 | [minor] Stylo pin on unmerged fork PR | merge-order item, unchanged | lexmount/stylo#2 still OPEN at e2baa14. |
| R2-S6 | [nit] minified tests | NOT FIXED | New test lines reach ~230 cols (markdown_dump.rs:475-485), and the CLI test args line is ~190. |
| R2-N1 / stack #11 | [minor] commits 1–4 don't build; churn | NOT FIXED (more churn) | Commits 1–4 are unchanged. New: 040269fd4 adds a blank line between a doc comment and fn needed (clippy empty_line_after_doc_comments) and a double blank in lib.rs, both removed by 92720683b. 3511c199a adds hacker-news-layout-computed.md and 92720683b deletes it. Moot with a squash merge. |
| R2-N2 | [nit] computed fixture in html2md; needed doc in #1337; #1592 missing from stack line |
PARTIAL | Fixture deleted, and html_fallback.rs is no longer in the net diff. The PR body stack line still ends at #1380. |
| stack #5 / #7 | html2md API and Stylo bump inside renderer commit | NOT FIXED (moot if squashed) | Both still in 040269fd4. |
| stack N2 | [medium] computed boundaries inside fallback <pre>/<code> |
NOT FIXED | html_fallback.rs:190-199 (#1336 file, touched by 68cc9e546 this round) still ignores code. See below. |
| stack N3 | [low] range/color sanitized defaults | NOT FIXED | See new #4. |
| stack N4 | [low] col/colgroup become <div>s in fallback tables |
NOT FIXED | See new #5. |
New / still-open findings
- [minor] Regression from the hidden-cell fix: a hidden table that takes the HTML fallback now emits an empty
<table>skeleton, andvisibility:collapserows become empty GFM rows (visibility.rs:358). - [minor] Lists are still loose when an item's child is block-styled (
li > a{display:block}, common in nav and dropdown menus). The README claim at L169 is false for this case. - [medium, #1336 × #1337, stack N2 still open] Live CSS boundaries change the text of linked code blocks (below, not anchorable in this diff).
- [low] Untouched range/color inputs print
50/#000000(visibility.rs:145). Stack N3. - [low] UA
table-column/table-column-groupare stillblocks, so fallback tables gain<div>s (visibility.rs:222). Stack N4. - [nit] The control exemption is a tag-and-string special case split across two layers (visibility.rs:217).
- [nit] README wrapping and placement (README.md:20).
3. (not anchorable: html_fallback.rs is not in this PR's diff) Computed boundaries rewrite linked code
html_fallback::render computes code (L190) but applies has_block_layout/has_text_boundary without checking it (L192-199). 68cc9e546 added the owns_boundaries guard on these exact lines but did not add !code. The adapter is the only producer of these hooks, so this only happens on the live path.
- Input:
<pre><code>let v = <span style="display:inline-block">f</span>(<a href="/doc">x</a>);</code></pre> - Live:
<pre>\n<code>let v = f (<a href="/doc">x</a>);</code></pre>, with two characters that are not in the source. Adisplay:blockline span produces<div>…</div>inside<pre>. - Source path, or the same block without a link (raw path, which never calls
visit):let v = f(x);.
Fix: skip both hooks whilecodeis true (if !code && let NodeKind::Element(tag) = …), and add one live case with a linked<pre>to the new exact-output test.
Other
- Merge state:
git merge-treeagainst #1336 (99e374401) and againstorigin/main(45be4e353) is clean. - CI: Format, Clippy, CDP smoke, harness tests, SecTrust and Windows storage pass. Nextest and both release builds are still queued, so the new exact-output and HTTP CLI tests have not run in CI yet.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
Stack-level re-review (#1334–#1337), round 4Scope. This round covers only cross-layer and combined-state problems. Refs: Changes since round 3. There are seven new commits: This round fixes the two round-3 blockers in the cases we wrote down:
Both fixes have exact-output live tests ( Status of the round-3 findings
New findings
N5. High: blockified computed
|
9272068 to
6d0a254
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Round-5 re-review of the updated version (6d0a2549a)
Round-5 re-review (head 6d0a2549a, base #1336 39ae6b32e). This PR makes the renderer's Markdown export read live state (computed display/visibility/baseline-shift, Stylo own-visibility, live control values, base-resolved URLs) through the MarkdownDom adapter over moli-html2md. The new commit 6d0a2549a and the #1336 changes beneath it fix the hidden-table skeleton, collapsed rows, single-block list items, col/colgroup and the control special case. Each fix has an exact live test that fails on the round-4 code. I traced the code by hand and did not build or run anything.
Verdict: request changes. The top stack item, N5 (high), is not fixed. The classifier still reads only the computed display (page_dump.rs still requests just display/visibility/baseline-shift). Stylo blockifies flex/grid items, floats and abspos elements (stylo e2baa14 style_adjuster.rs:221-256), so they still produce paragraph breaks. N6 is also open, and the range/color fix still leaks defaults when value="".
Previous findings
| # | Finding | Status | Evidence at 6d0a2549a |
|---|---|---|---|
| R4-1 | [minor] hidden fallback table emits an empty <table> skeleton; collapse rows become empty GFM rows |
FIXED | A reverse pre-order pass (visibility.rs:266-283) suppresses an invisible table/ul/ol/dl that has no visible descendant. visibility:collapse tr/thead/tbody/tfoot is suppressed (L199-214). Exact tests L486-487, plus a restored-descendant test at L500-511. Residual nit: col/colgroup in that list do nothing (new #5). |
| R4-2 | [minor] loose list from a block-styled item child; README claim | FIXED for the reported case | #1336 adds Writer::layout_boundary, which does not set has_blocks (writer.rs:276-283). flush_breaks sets it only when a break of 2 or more lands between content (writer.rs:504). Exact test L485. The README now claims only the single-block case (L119-121). Two flex items in an item are still loose (N5). |
| R4-3 / stack N3 | [low] range/color sanitized defaults | PARTIAL | Absent value is fixed and tested (L488-489, L505). value="" or an invalid authored value still prints 50/#000000 (new #3). |
| R4-4 / stack N4 | [low] col/colgroup become <div> |
FIXED (in #1336) | owns_boundaries now includes col/colgroup (#1336 machine.rs). Exact test L490. |
| R4-6 | [nit] control inline-block exemption |
FIXED | Branch deleted (L221-228). Controls get a collapsible space, and the existing exact control and cell tests are unchanged. |
| R4-7 | [nit] README wrap/placement | MOSTLY FIXED | Policy is under ## Content policy (L107) and the duplicate is gone. L20 and L113 are rewrapped. L6 is now 94 cols (new #6). |
| stack N2 | [medium] CSS boundaries inside fallback <pre>/<code> |
FIXED (in #1336) | html_fallback.rs now guards both hooks with !code. |
| stack N5 | [high] blockified flex/grid items, floats, abspos | NOT FIXED | See new #1. |
| stack N6 | [low] per-character inline-block spans split |
NOT FIXED | See new #2. |
| R2-1 | [minor] Stylo pin on unmerged fork PR | merge-order item, unchanged | lexmount/stylo#2 is still OPEN at e2baa14. |
| R3-9 / stack #8 | full passes per conversion | pushback accepted | This commit adds one more pass plus a node_order Vec. Still acceptable. |
| R2-S6 | [nit] minified tests | NOT FIXED | New lines reach 237 cols (L490) and 329 cols (L505). |
| R2-N1 / stack #5/#7/#11 | commit hygiene; html2md API and Stylo bump in the renderer commit | NOT FIXED (moot if squashed) | Commits 1-8 are unchanged (range-diff =). 6d0a2549a is titled fix(renderer) but mostly rewraps html2md README. |
| R2-N2 | PR body stack line omits #1592 | NOT FIXED | It still ends at #1380. |
New / still-open findings
- [high] N5 still open: blockified
displayturns flex children, floats and.sr-only/abspos spans into paragraph breaks (visibility.rs:222). - [low] N6 still open: every
inline-*box gets a space, so per-character spans split words (visibility.rs:223). - [low] range/color:
value=""or an invalid authored value still leaks the synthetic default. The type is re-parsed from the attribute (visibility.rs:142). - [low] The new range/color and hidden-structure test uses
containsassertions, not exact output (markdown_dump.rs:505). - [nit]
col/colgroupincollapsed_table_partare no-ops. Collapsed columns still export their cells, though the author reply says columns are removed (visibility.rs:202). - [nit] README L6 is 94 cols. "wholly invisible tables" also covers lists (README.md:6, :120).
Other
- #1592 is not rebased. Its head is still
86ed30b95on the round-3 basef04b0b151.git merge-tree --merge-base f04b0b151 6d0a2549a 86ed30b95conflicts inpage_dump.rsandpage_dump/visibility.rs. When it is rebased, it must apply body-only collection per frame document and run the newnode_orderreverse pass over each frame's styles. - Merge state: clean against #1336 (
39ae6b32e) and againstorigin/main. - CI: Format, Clippy, CDP smoke, harness, SecTrust and Windows storage pass. Nextest and both release builds are QUEUED, so none of the new exact tests have run in CI.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
Stack-level re-review (#1334–#1337), round 5Scope. This round covers only cross-layer and combined-state problems. Refs: merge-base Changes since round 4. Four new commits: Status of the round-4 findings
New findings
B1. High: nine commits do not compile on their own
Both problems were already present in round 4 ( Other per-commit checks found no further breaks. I resolved B2. Medium: collapsed rows shift
|
6d0a254 to
4e58ef8
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Final re-review (P0/P1 only) (4e58ef889)
Round-6 (final) re-review. Head is 4e58ef889 and base is #1336 5df2dd7f7. I traced the code by hand and did not build or run anything.
Verdict: request changes for one remaining P0, and only if this PR is rebase-merged. The new commit 4e58ef889 fixes N5 for all five reported inputs, and each has an exact live test. Column-flex and grid children still become blocks, as they should. N3 and N6 are fixed too. B1 has not changed: four commits do not compile on their own. The repository allows rebase merge (allow_rebase_merge: true), so if this PR is squash-merged, B1 does not matter and nothing at P0/P1 remains.
Previous P0/P1-level findings
| # | Finding | Status | Evidence at 4e58ef889 |
|---|---|---|---|
| N5 | [high] Stylo-blockified flex/grid items, floats and abspos elements produce paragraph breaks | FIXED | page_dump.rs:36-44 now also reads position, float and flex-direction, plus Stylo's original_display (computed.rs:288-290). visibility.rs:126-137 builds item_blocks from the parent's display: row flex/inline-flex and inline-grid children get a space boundary; column flex and grid children become blocks. Abspos/fixed elements, and floats that are not flex items, add no boundary (:244-271). Because set_adjusted_display(.., false) keeps original_display for floats and abspos (Stylo properties.mako.rs:1436-1446), a non-item inline-flex button keeps its inline outer type. Structural tags (div, li, td, form…) still own their boundaries (machine.rs:222, owns_boundaries), so floated or abspos divs are still paragraphs. Exact live tests at markdown_dump.rs:491-506 cover the search form (rust Search), inline-flex button, abspos .sr-only, flex li (tight list), flex cell (Alice Edit), floats, column flex (A\n\nB), grid and inline-grid. A CSSOM/DOM invariance test is at :536-547. |
| N6 | [low, user-visible] per-character inline-block spans split words |
FIXED | Plain atomic inlines get no space now. Only input/textarea/select/button and inline list-items do (visibility.rs:259-268). <h1> with per-letter spans gives # Hi (test :498). |
| N3 | range/color sanitized defaults leak when value="" or the value is invalid |
FIXED | Range/color use the live value only when dirty (visibility.rs:161-163, using element.input_type()). Tests: value='' gives Volume and invalid values give abc invalid (:500-501). A script-set value is asserted exactly (:519-532). |
| B1 | [P0 if rebase-merged] #1337 commits call HAS_OWN_VISIBILITY before the Stylo pin bump |
NOT FIXED | Commits 1-4 are unchanged (range-diff =). 5f4a1e855, f6342b5d8, aa9b30ced and 60a926f2e use ComputedValueFlags::HAS_OWN_VISIBILITY (computed.rs:312 at 5f4a1e855), while Cargo.toml/Cargo.lock pin Stylo 973988b9, which has no such constant. The bump arrives only in cc4aa24e1. Squash-merge, or move the bump into the first commit. |
| B2 | collapsed rows are removed, so a rowspan shifts cells |
NOT FIXED (P2 for this round) | visibility.rs:217-223 still suppresses a visibility:collapse tr/thead/tbody/tfoot. The author says this is intended. It needs a collapsed row with a rowspan from an earlier row reaching into it, which is rare on real pages, so I do not block on it. |
| R2-1 | Stylo pinned to an unmerged fork PR | merge-order item, unchanged | lexmount/stylo#2 is still OPEN at e2baa14. |
New P0/P1 findings
No new P0/P1 issues in 4e58ef889 or 85969dc48. Residual edge cases, below P1 and not blocking:
- The flex-item lookup uses the DOM parent, while Stylo uses the layout parent. A
display:contentswrapper between a row-flex container and its items still gives paragraph breaks, becauseoriginal_displayis blockified for items. - An abspos inline badge with no source whitespace now glues to its neighbour, for example
Cart<span style=position:absolute>3</span>→Cart3.
Other
- #1592 is rebased onto
4e58ef889(head8a423e752, merge-base = this head).git merge-treeis clean, and it now collects body-only styles per frame document. - CI: Format, Clippy (so the head compiles), CDP smoke, Node/Python harness, SecTrust and Windows storage pass. Nextest and both release builds are still pending, so the new exact tests have not run in CI yet.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
| pub(crate) fn has_own_visibility_value(&self) -> bool { | ||
| self.primary | ||
| .flags | ||
| .contains(ComputedValueFlags::HAS_OWN_VISIBILITY) |
There was a problem hiding this comment.
[P0 if rebase-merged] Still open (stack B1): the first four commits of this PR do not compile. 5f4a1e855, f6342b5d8, aa9b30ced and 60a926f2e reference ComputedValueFlags::HAS_OWN_VISIBILITY (here; computed.rs:312 at 5f4a1e855). In those commits Cargo.toml/Cargo.lock still pin Stylo 973988b9c…, whose computed_value_flags.rs has no such constant. It first appears at e2baa14, and the pin moves only in cc4aa24e1. The repo has allow_rebase_merge: true, so a rebase merge would put four non-building commits on main. Fix: squash-merge, or move the Cargo.toml/Cargo.lock Stylo bump into 5f4a1e855 (ideally after lexmount/stylo#2 lands).
4e58ef8 to
19bfc78
Compare
1 participant
问题
Markdown 导出需要读取当前 DOM、表单值和资源地址。序列化属性可能仍是初始状态；按透明度、颜色或框架属性猜测正文，也会误删有效内容。
可见性恢复还需要区分两种情况：页面继承的加载遮罩，以及元素自身明确设置的隐藏。两者混用时，内容面板的恢复路径会误删旁边的正文。
一图看懂
方案
display、visibility、baseline-shift和自身可见性来源，按 DOM 先序计算继承状态，并检查父节点先于子节点。display:none祖先参与披露路径的兄弟过滤。visibility单独处理，允许后代恢复可见；重复 ID 只选择第一个目标。pre保留折叠源文本，普通行内code仍遵守隐藏规则。脚本、样式、模板和代码操作控件不进入源文本。CSS 会把 flex/grid 子项、浮动和绝对定位元素的 display 调整为块级；这不表示作者设置了段落。导出从同一份 Stylo 观测中读取原始 display：横向控件保持一行，纵向布局保留分隔，定位文字不拆开原链接。普通逐字 span 不自动添空格，作者写出的空白仍保留。公开 CSSOM 值与原 DOM 均不改变。
验证
8798f93a…五个 CLI 测试覆盖六份布局开关快照及真实 HTTP 地址解析。当前 HEAD 的 CLI 复现绑定见下文。
真实页面的精确输出覆盖紧凑列表、嵌套列表、下拉分组、行内控件、隐藏中间单元格和关闭的 dialog/popover。隐藏单元格保留列位置，只去掉不可见文字；整张不可见的表格不生成空骨架，后代恢复可见时仍保留其内容，collapse 行从输出中移除；原生结构不因 UA 默认 CSS 变成额外段落。样式读取范围为正文子树与继承所需的根元素。
隐藏祖先下的独立 tabpanel 与 hidden=until-found 内容，先登记为内容根，再计算保留路径；路径中的隐藏兄弟与导航模板继续排除。计算样式的块边界覆盖多关键词 list-item，不依赖渲染布局。
未更改的 range/color 读取作者属性，空属性不变成 50 或黑色默认值；更改后的控件读取当前实时值。普通表单值属于授权 DOM 导出的内容；密码、隐藏和文件输入仍排除。HTML
with_frames沿用现有契约，保留 iframe 地址与嵌入元数据，不把 srcdoc 合并为主文档正文。已发布测量
历史完整运行绑定源码
721bd2e329f96c06573db6e6b257a149cf17a6fe和二进制fac987f5…。同一 545 页的 PR785 与该运行全文均值分别为 41.7243% 和 40.7252%；折叠明细丢失是其中一个已定位问题，HTML 结构增加也会改变字符串相似度。本 PR 的定向验证没有替换历史成绩。历史 WebMainBench 报告。
复现
以下命令验证完整 stack 的浏览器导出路径。
最小用例
横向表单保持一行，逐字样式包装不把 Hi 拆开；隐藏导航不进入输出。
实际输出：
rust Search # Hi真实 WebMainBench 页面
三个原始失败页
d80cf991…、72dd5ff7…、ccb6033c…均命中目标正文；8798f93a…保留全部 91 组明细。七个相关页面通过 browser-eval 的现有 runner 共用一次定向运行，7/7 正常导出，并核对上述内容；不是 545 页质量重测。查看冻结输入、实际输出与核对结果。
19bfc785；二进制 SHA-256：1c17bba8cb6b0a532144697411d13ec6a3872d1abad96d5872da0ded93d7b8b4。345ea093。0efaa4b49a45e320a27fe6e5a0b6aad5b57259fc3321ac3448519cacc74c537e，runner 会校验。保留原网址，代理返回冻结 HTML，外部资源禁止访问，无点击或 DOM 改写。环境准备与可复制命令
results.jsonl记录成功状态、错误和输入/输出哈希，execution.json记录二进制身份，artifacts/moli/保存实际 Markdown。定向运行只用于定位和回归，不进入正式榜单；报告中的历史成绩仍绑定原测量版本。依赖
直接基于 #1336。Stylo 固定
e2baa14adb66df48a56416a93511690d27bfd600，在 main 已使用的973988b9上仅增加自身可见性来源标记；对应 Stylo #2。已移除无消费者的动画接口和与导出无关的 font color 修改，保留 main 的其它依赖更新。#1334 → #1335 → #1336 → #1337 → #1380