feat(grida_wpt): new crate for rendering-test harness + WPT plugin#690
feat(grida_wpt): new crate for rendering-test harness + WPT plugin#690softmarshmallow merged 6 commits intomainfrom
Conversation
Promote the HTML/CSS golden producer out of `cg --example golden_htmlcss` into a dedicated `grida_wpt` crate. Lean deps (no winit/GL) so external runners — the in-tree refbrowser pipeline today, upstream wptrunner next — can invoke the binary cheaply. Three modes on the `render` subcommand: - `--suite <json>` — matches the existing refbrowser L0 pipeline; 64/64 fixtures byte-identical to the prior `cg --example` output. - `--fixture`/`--dir` — ad-hoc local renders. Directory scan accepts .html/.htm/.xht/.xhtml (shared HTML_EXTENSIONS const). - `--url <http://...>` — fetches a page and renders it; consumed by the Grida WPT fork's wptrunner plugin. Also extends `.xht`/`.xhtml` stem support in `refbrowser_render.ts` so WPT-style filenames round-trip through the oracle. WPT plugin setup and fork links: docs/contributing/wpt.md. The fork itself: https://github.com/gridaco/wpt
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
WalkthroughThe PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as grida_wpt CLI
participant Suite as Suite Loader
participant Render as Render Module
participant CSS as CSS Cascade
participant Canvas as Canvas/Skia
participant FS as File System
CLI->>Suite: load(suite_path)
Suite->>FS: read JSON
FS-->>Suite: suite config
Suite-->>CLI: SuiteFile with fixtures
loop for each fixture
CLI->>Render: render_to_png(html, width, height, name, out_dir)
Render->>Render: load HTML + extra CSS
Render->>CSS: compile HTML with stylesheets
CSS-->>Render: styled tree
Render->>Canvas: create Skia surface & draw
Canvas-->>Render: rasterized picture
Render->>FS: write {name}.png
FS-->>Render: ✓ written
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Taffy has no inline formatting context, so a block container holding `display: inline-block` siblings previously either stacked them vertically (when preserved as elements) or dropped their width/height (when flattened into an InlineGroup). Both broke the WPT ref pattern where two or more explicit-size inline-blocks should sit side-by-side. - collect.rs: preserve inline-block as an element when it has an explicit width or height instead of flattening into the inline path. - layout.rs: at Taffy-style time, detect containers whose element children are all inline-block (>=2, no text or other block children) and switch the container to Flex + flex-wrap:wrap + align-items:start. Lands layout-display-inline-block.html in L0.exact (65 fixtures, all 100.00%). Unlocks WPT refs that rely on inline-block for horizontal layout (37% of css-flexbox, 30% of css-position refs).
feat(htmlcss): inline-block siblings via Taffy flex-wrap emulation
servo/stylo#273 has shipped, so we can drop the `gridaco/stylo` fork pin that existed solely to get that fix ahead of release. API adaptations required by the 0.9 → 0.16 bump: - `selectors` fork replaced with crates.io `selectors 0.37`. - `TElement` now requires `AttributeProvider`; implemented for both the real adapter and the standalone reference example. - `ensure_data`/`borrow_data`/`mutate_data` return the new `ElementDataRef/Mut` types; per-element storage migrated from `AtomicRefCell<Option<ElementData>>` to `OnceLock<ElementDataWrapper>`. `clear_data` is a no-op (matches Gecko's reuse-allocation semantics). - `Stylist::flush` dropped its root/snapshot arguments. - Module moves: `style::servo::media_queries::{Device,FontMetricsProvider}` → `style::device::{Device, servo::FontMetricsProvider}`; `QueryFontMetricsFlags` moved from `specified::font` to `computed::font`. - `BorderSideWidth` is now a newtype over `Au`; unwrap via `.0.to_f32_px()` and gate by border-style so `none/hidden` still zero the used width (the fork did this implicitly). - `OverflowClipMargin` became a struct; access via `.offset`. - `GenericShapeRadius::Length` keeps its `NonNegative` wrapper. - `outline_has_nonzero_width()` removed; replaced by explicit outline-style + width checks. - Grid pref plumbing moved from `stylo_config` to `stylo_static_prefs::set_pref!`, which is what stylo 0.16 reads. Verified: - `cargo check` / `cargo test` pass across cg, csscascade, grida-dev, grida-canvas-wasm (771 unit tests on cg). - WASM debug build + Node smoke test pass. - Refbrowser L0.exact: 65/65 at 100.00%. L0.coverage: avg 99.70%, no new regressions (sub-100 fixtures are all in the already-documented divergence surfaces: gradients, dotted borders, blur shadows, outline AA).
chore(stylo): drop fork, upgrade to stylo 0.16 from crates.io
Wraps the `cargo build` + `./wpt run` dance into a single command with sensible defaults (whole `css/` suite, structured report to `target/wpt-report.json`). Doc updated to lead with the recipe; raw `./wpt run` invocation relegated to an "under the hood" section for contributors who need custom flags.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b43c78efe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let width = vp.width.unwrap_or(DEFAULT_WIDTH); | ||
| let height = vp.height.unwrap_or(DEFAULT_HEIGHT); |
There was a problem hiding this comment.
resolve_entry now falls back directly to DEFAULT_WIDTH/DEFAULT_HEIGHT after selecting entry.viewport, so a fixture that sets only one dimension (e.g. just height) silently drops the suite-level default for the other axis. That breaks the documented inheritance contract and changes render sizes for partial viewport overrides, which can skew image diffs and baseline comparisons.
Useful? React with 👍 / 👎.
| right now is to make the plumbing reliable, not to ship a pass rate. | ||
|
|
||
| [wpt]: https://web-platform-tests.org/ | ||
| [cg-reftest]: ../../.agents/skills/cg-reftest/SKILL.md |
There was a problem hiding this comment.
This page links to ../../.agents/..., which violates the docs/AGENTS.md rule that docs markdown must not link outside /docs. In the published Docusaurus site those paths do not exist, so readers get broken links and navigation regressions; convert these to inline code references or valid in-doc/external URLs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (13)
crates/grida-canvas/src/html/mod.rs (1)
1138-1152: Pre-existing nit (not from this PR): color probe ignores per-side visibility.The color-picker at Lines 1140–1143 iterates
top → right → bottom → leftwithout checking whether the corresponding side has non-zero width after the newwidth_forzeroing. Patterns likeborder: 1px solid red; border-top-style: none;will still pickborder_top_coloreven though the top side is invisible. Not introduced by this change, but now that side-visibility is explicit it would be a small, natural follow-up to gate the probe ontop_w > 0.0/right_w > 0.0/ etc. Feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/html/mod.rs` around lines 1138 - 1152, The color probe that builds border_color should skip a side if that side's computed width is zero; update the selection logic in the block using css_color_to_cg(&border.border_top_color) / _right_color / _bottom_color / _left_color to first check the corresponding width (e.g., top_w/right_w/bottom_w/left_w as produced by width_for or however widths are named) and only call css_color_to_cg for sides with width > 0.0 so invisible sides (width zero or style none) are not considered when choosing the primary stroke color.Cargo.toml (1)
8-8: Optional: crate name style deviates from the rest of the workspace.Every other workspace member uses hyphens (
grida-canvas,grida-canvas-wasm,grida-dev,csscascade,math2), but this new crate uses an underscore (grida_wpt). Cargo normalizes both forms to the same crate identifier, so the hyphen form (grida-wpt) would still yieldgrida_wptas the library/binary target and keep the workspace convention consistent. Not blocking — flagging only because the binary incrates/grida_wpt/Cargo.tomlsetsname = "grida_wpt"explicitly, so a hyphenated package name would not affect the CLI invocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` at line 8, The workspace lists the new crate as "grida_wpt" which deviates from the hyphenated naming used elsewhere; update the workspace member entry from "crates/grida_wpt" to the hyphenated form "crates/grida-wpt" and also update the crate's own Cargo.toml package.name (currently "grida_wpt") to "grida-wpt" so naming is consistent across the workspace while Cargo still normalizes the crate identifier to grida_wpt at compile time; ensure any explicit binary `name = "grida_wpt"` entries are changed to `name = "grida-wpt"` as well..agents/skills/cg-reftest/scripts/refbrowser_render.ts (1)
152-152: Extension strip is correct; tiny readability nit.The regex works: with the
$anchor, JavaScript's alternation backtracks from axhtpartial to the fullxhtmlmatch, so the order inside the group doesn't change behavior. Still, ordering by descending specificity (xhtmlbeforexht) avoids a double-take for readers cross-checking against the Rust side (crates/grida_wpt/src/fetch.rs:stem_from_url, which iterates[".xhtml", ".html", ".htm", ".xht"]):♻️ Optional readability tweak
- const stem = path.basename(entry.path).replace(/\.(html?|xht|xhtml)$/i, ""); + const stem = path.basename(entry.path).replace(/\.(xhtml|html?|xht)$/i, "");- const stem = path.basename(htmlPath).replace(/\.(html?|xht|xhtml)$/i, ""); + const stem = path.basename(htmlPath).replace(/\.(xhtml|html?|xht)$/i, "");Also applies to: 261-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/cg-reftest/scripts/refbrowser_render.ts at line 152, The regex used to strip extensions in the stem calculation is correct but reorder alternation for readability: change the replace call on path.basename(entry.path) inside the const stem assignment so the alternation lists "xhtml" before "xht" (e.g. use an alternation like xhtml|html?|xht) and make the same change at the other occurrence noted (the second replace around line 261) so both uses read consistently and match the Rust ordering.justfile (1)
45-53: Recipe composes correctly; one small resilience suggestion.
{{justfile_directory()}}resolves to absolute paths beforecd ../wpt, so--binaryand--log-wptreportwon't break after thecd. That's the right pattern here.Optional: a preflight check for the sibling checkout would turn a cryptic
./wpt: No such file or directoryinto something actionable, e.g.:♻️ Optional preflight
wpt target="css/": cargo build -p grida_wpt --release + test -x ../wpt/wpt || { echo "error: expected gridaco/wpt fork at ../wpt (see docs/contributing/wpt.md)"; exit 1; } cd ../wpt && ./wpt run \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 45 - 53, The wpt recipe should fail fast with a clear message when the sibling wpt checkout or the wpt runner is missing: add a preflight check in the wpt recipe (before cd ../wpt) that verifies the ../wpt directory exists and that the wpt runner is present and executable (the ./wpt used after cd ../wpt), and if not, print an actionable error and exit; keep the existing use of {{justfile_directory()}} for absolute paths but ensure the check references the sibling ../wpt and its ./wpt binary so users see a clear “missing checkout or non-executable wpt” message instead of “./wpt: No such file or directory”.crates/grida-canvas/src/htmlcss/layout.rs (2)
129-140: Flex-wrap emulation is a reasonable approximation; document the limits.Forcing
Flex + Wrap + align_items: Startreproduces left-to-right inline-block flow for the homogeneous case the gate accepts, and the explicit Start override correctly prevents the defaultstretchfrom blowing up zero-height inline-blocks to container-line height.Two caveats worth a comment (inline) so future contributors don't get surprised:
- Sibling
vertical-align: baseline | middle | bottomvariance among inline-blocks won't render faithfully under this emulation —align_itemsapplies uniformly, whereas real inline-block baseline alignment is per-item against the line baseline. The new fixture sidesteps this by usingvertical-align: topon all boxes.- The container's own
justify_content(written unconditionally inelement_to_taffy_styleat line 464) now becomes active for what was previously a block container. For defaultStartit's a no-op; if any codepath introduces non-Startdefaults for block elements, alignment could shift. Consider pinningstyle.justify_content = Some(taffy::JustifyContent::FlexStart)here alongside thealign_items = Startoverride to be explicit about the emulation's intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 129 - 140, The inline-block emulation change (inside the block gated by should_emulate_inline_block_container) needs an inline comment documenting two limitations and an explicit justify_content pin: note that using style.display = taffy::Display::Flex with style.flex_wrap and style.align_items = Start will not reproduce per-item vertical-align (baseline/middle/bottom) semantics and that callers should prefer vertical-align: top to match the fixture; also explicitly set style.justify_content = Some(taffy::JustifyContent::FlexStart) here (and mention element_to_taffy_style as the place that otherwise writes justify_content unconditionally) so the emulation does not accidentally inherit non-Start justification.
192-210: Gate relies on upstream whitespace filtering—consider adding a note for clarity.The gate function does indeed return
falsefor anyStyledNode::TextorStyledNode::InlineGroup, which could be brittle if inter-element whitespace survived tree construction. However, verification shows thatcollect.rs(lines 952–962) explicitly filters out whitespace-only inline groups during tree construction with the comment:Skip whitespace-only groups — these are inter-element whitespace (e.g. "\n " between
and) that should not create a block.
This means pure-whitespace text between inline-block siblings never reaches the gate function, so the code works correctly. Still, adding a one-line comment in
should_emulate_inline_block_containerdocumenting this invariant—or citing the filtering incollect.rs—would help future maintainers understand why the gate doesn't need explicit whitespace tolerance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/layout.rs` around lines 192 - 210, Add a brief comment in should_emulate_inline_block_container explaining that whitespace-only text/inline groups are filtered earlier during tree construction (see collect.rs), so the function can safely treat any StyledNode::Text or StyledNode::InlineGroup as a fatal case; reference the function name should_emulate_inline_block_container and the upstream filter in collect.rs in the comment so future maintainers understand the invariant.crates/grida_wpt/src/fetch.rs (1)
32-48: Minor: percent-encoded path segments leak into the stem.
path_segments()returns raw (percent-encoded) segments, so a URL like.../hello%20world.htmlbecomes stemhello%20world. For canonical WPT paths this never matters, but if you ever want round-trippable filenames you may want to percent-decode the last segment viapercent_encoding::percent_decode_str(last).decode_utf8_lossy()before stripping the extension.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida_wpt/src/fetch.rs` around lines 32 - 48, The stem_from_url function currently uses raw percent-encoded path segments (variable last); decode that segment before lowercasing and extension stripping to avoid percent-encoding leaking into stems. Update stem_from_url to percent-decode last (e.g., via percent_encoding::percent_decode_str(last).decode_utf8_lossy()), then operate on the decoded string for to_ascii_lowercase(), strip_suffix checks, and final return (ensure you still return the original-cased decoded segment trimmed to the stem length when a suffix is removed).crates/grida_wpt/src/main.rs (1)
129-139: Minor:--outis not honored in--fixturemode.
render_onegoes straight torender_with_extras, which writes<out_dir>/<file_stem>.png. So--fixture foo.html --out /tmp/bar.pnglands at<default_out_dir>/foo.png, not/tmp/bar.png. Either route fixture mode throughresolve_outtoo, or document that--outis URL-mode-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida_wpt/src/main.rs` around lines 129 - 139, The fixture-mode render_one currently ignores RenderArgs.out and calls render::render_with_extras directly, producing files under the default out dir; update render_one to compute the final output path using the existing resolve_out logic (or honor args.out) before calling render_with_extras so the produced PNG goes to the user-specified location; locate the function render_one and replace the direct use of out_dir with the resolved path (still passing fonts and the cache) so --out is respected in fixture mode.docs/contributing/wpt.md (2)
55-58: Inline TODO links will keep firing MD052 and leaving dangling references.Two TODO placeholder references (
[inventory]on line 55 and the "adoption plan" link on line 154) both point at generic issues pages with trailing<!-- TODO -->comments.markdownlintis already complaining (MD052at line 55). Either land the real links or replace the reference links with inline italics/text until they're ready, so the docs don't ship with broken references.Also applies to: 154-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/contributing/wpt.md` around lines 55 - 58, The markdown contains placeholder reference links ([inventory] and the "adoption plan" reference) with trailing <!-- TODO --> comments causing markdownlint MD052; update the references in docs/contributing/wpt.md by either replacing the reference-style links ([inventory], the adoption-plan reference) with inline italicized/plain text placeholders or by removing the HTML TODO comment and inserting the final target URLs so the reference labels are valid; ensure you remove or fix the dangling reference labels rather than leaving them commented, so markdownlint no longer reports MD052.
1-2: Consider addingformat: mdfrontmatter.The doc uses raw angle brackets inside code spans (
<meta name="fuzzy">,<link rel="stylesheet">— the latter even crossing a line break at 115-116). Docusaurus MDX parsing can get flaky with cross-line code spans containing JSX-ish tokens. Addingformat: mdis the guideline-recommended hedge.📝 Suggested frontmatter
+--- +format: md +--- + # Contributing to Grida | Web Platform TestsAs per coding guidelines: "When using angle brackets in Docusaurus markdown (e.g., comparisons, URLs, generics), prefer
format: mdfrontmatter to opt out of MDX parsing".Also applies to: 92-93, 115-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/contributing/wpt.md` around lines 1 - 2, Add a top-matter entry setting format: md to this markdown file to opt out of MDX parsing; specifically, insert a frontmatter block at the very top of "Contributing to Grida | Web Platform Tests" that includes format: md so raw angle-bracket code spans like `<meta name="fuzzy">` and `<link rel="stylesheet">` (and the occurrences around lines noted: the cross-line span at 115-116 and the ones at 92-93, 115-117) are treated as plain markdown rather than MDX.crates/csscascade/examples/exp_impl_telement.rs (1)
1476-1503: LGTM — example kept in lockstep with theadapter.rs/dom.rsrefactor.Note: this example duplicates a lot of
src/adapter.rsandsrc/dom.rswholesale. If it's meant as a long-lived playground, consider trimming it to only the parts that differ from the library, or gating it behind an#[cfg(example)]-like feature to prevent silent drift.Also applies to: 1608-1619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/csscascade/examples/exp_impl_telement.rs` around lines 1476 - 1503, The example duplicates the library DOM/adapter implementations (the TElement methods ensure_data, clear_data, has_data, borrow_data, mutate_data) causing maintenance drift; fix by removing the duplicated method bodies in exp_impl_telement.rs and either (A) delegate those methods to the library implementations (call into the crate::adapter / crate::dom equivalents) so the example re-uses the real code, or (B) gate the example behind a dedicated feature (e.g., #[cfg(feature = "example_playground")]) and document enabling it; update the TElement impl to call the shared functions (or conditionally compile the whole example) so the example no longer silently drifts from adapter.rs/dom.rs.crates/grida_wpt/src/render.rs (1)
143-154: Shallow directory scan — document the depth-1 semantics.
read_diris non-recursive, so fixtures nested in subdirectories (typical WPT layout:reftest/box/foo.html) are silently skipped. That's defensible for--dirbut worth calling out in the doc comment onrender_directoryso users don't assume recursive discovery when pointing at a WPT subtree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida_wpt/src/render.rs` around lines 143 - 154, The function render_directory currently uses std::fs::read_dir (non-recursive) to collect HTML files into entries, so nested fixtures under subdirectories are intentionally skipped; update the doc comment on render_directory to explicitly state this depth-1 (non-recursive) semantics, mention that it only scans the immediate directory entries (not recursive traversal), and suggest alternatives (e.g., use a recursive walker like walkdir::WalkDir or call render_directory for each subtree) so callers know how to handle typical WPT layouts with nested files such as reftest/box/*.html.crates/grida-canvas/src/htmlcss/collect.rs (1)
1024-1024:overflow-clip-marginreference-box is dropped.
clone_overflow_clip_margin()in stylo 0.16 carries both a reference box (padding-box/content-box/border-box) and an offset length. Only.offset.px()is kept here, so the reference-box is silently coerced to whatever paint code assumes (likelypadding-box). That matches the CSS default and is fine for current fixtures, but will bite when WPT starts exercisingoverflow-clip-margin: content-box 10pxstyle tests. Consider threading the reference-box keyword through to the paint path as a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/htmlcss/collect.rs` at line 1024, The current assignment drops the reference-box from clone_overflow_clip_margin() by only using .offset.px(); capture the full result of style.get_margin().clone_overflow_clip_margin() into a local (preserving both the reference-box keyword and the offset) and update the element representation and paint-path to carry that reference-box alongside the offset instead of coercing it away; specifically, adjust the usage around get_margin(), clone_overflow_clip_margin(), and the el.overflow_clip_margin assignment so the reference-box keyword is threaded through (or add a new field on el to hold the reference-box) and ensure downstream paint code consumes that new value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/csscascade/Cargo.toml`:
- Around line 22-26: The Cargo.toml pins for stylo-related crates are incorrect;
update the dependency versions to published crates (set stylo = "0.15.0",
stylo_dom = "0.13.0", stylo_atoms = "0.14.0") or revert those three back to
their git dependency entries until 0.16 is published; modify the entries for the
symbols "stylo", "stylo_dom", and "stylo_atoms" accordingly while keeping
"style_traits" (stylo_traits) and "selectors" as-is to ensure the crate
resolves.
In `@crates/grida_wpt/src/main.rs`:
- Around line 37-76: RenderArgs' width/height are ignored when running with a
suite because render::render_suite computes per-fixture viewports; update
RenderArgs so the CLI clearly prevents or forwards those flags: either add
conflicts_with = "suite" to the width and height args (or to the suite arg) so
clap rejects --suite + --width/--height, or change the width/height help text to
state "Used only with --fixture/--dir/--url" and document that
render::render_suite ignores them; apply the same change to the duplicate
RenderArgs block around the other occurrence (lines ~85-112) so behavior/help is
consistent.
In `@crates/grida_wpt/src/render.rs`:
- Around line 91-94: The current fallback sets name to "unknown" when
html_path.file_stem() is None, which can cause collisions; update the logic that
defines the variable name (the let name = ... block using html_path) to produce
a unique fallback instead of a constant "unknown" — for example, compute a short
hash (e.g., SHA256 or xxhash) of the full html_path (or its display_string) and
use that hash as the stem when file_stem() is None, or alternatively return an
Err from the surrounding function (render) to surface the problem; ensure you
modify the code that constructs name so it uses
html_path.file_stem().map(...).unwrap_or_else(|| fallback_from_hash(html_path))
and keep variable name and surrounding error-handling consistent with the
function's return type.
- Around line 29-40: The current ensure_css_cached function leaves the HashMap
entry vacant on read error so every caller retries and re-warns; update
ensure_css_cached (the cache: &mut HashMap<PathBuf, String> function) to insert
a sentinel value into the vacated slot on Err (e.g., empty string or specific
marker) after logging the eprintln, so Entry::Vacant is no longer present and
subsequent calls will not re-read or re-emit the warning; keep the existing
success path (slot.insert(s)) and the error log using abs.display().
- Around line 43-71: Change render_to_png (and likewise render_with_extras) to
return Result<(), String> instead of panicking: propagate errors from
htmlcss::render, surfaces::raster_n32_premul, image.encode, and std::fs::write
by mapping them into Err with context that includes the fixture identifier (name
and/or html_path), e.g., "render failed for {name}: {err}". Remove all
expect/unwrap/panic calls in render_to_png and return descriptive Err strings;
update callers in render_suite / render_directory to log eprintln! on Err and
continue with the next fixture.
In `@crates/grida_wpt/src/suite.rs`:
- Around line 74-91: The current resolve_entry replaces the entire viewport when
entry.viewport is Some, losing per-field defaults; change resolution to merge
per-field: read entry.viewport as Option<&Viewport> and compute width =
entry.viewport.as_ref().and_then(|v|
v.width).or(defaults.viewport.width).unwrap_or(DEFAULT_WIDTH) and similarly
height = entry.viewport.as_ref().and_then(|v|
v.height).or(defaults.viewport.height).unwrap_or(DEFAULT_HEIGHT), then use those
width/height values in ResolvedFixture; update resolve_entry (and references to
entry.viewport, defaults.viewport, DEFAULT_WIDTH, DEFAULT_HEIGHT) accordingly so
each axis falls back first to fixture, then suite defaults, then built-in
constants.
In `@docs/contributing/wpt.md`:
- Around line 13-14: The docs use reference-style links that point outside /docs
which violates the guideline; locate the reference labels [cg-reftest],
[crates/grida_wpt/], and [grida_wpt crate README] and replace any uses of those
reference-style links with inline code containing the external path (e.g.,
`../../.agents/skills/cg-reftest/SKILL.md`, `../../crates/grida_wpt/`,
`../../crates/grida_wpt/README.md`) and remove the corresponding reference
definitions so no link definitions point outside /docs.
- Around line 31-35: The fenced code block showing the directory layout is
missing a language tag; update the triple-backtick fence that precedes the
"Apps/grida/" directory tree to include a language identifier (use "text") so
the block becomes ```text, ensuring markdownlint MD040 is satisfied; no other
content changes needed.
- Around line 92-93: The typos checker is falsely flagging the substring "PN" in
"PNGs" as a typo; update the typos allowlist by adding the plural "PNGs" to the
[default.extend-words] section of .typos.toml so the term used in
docs/contributing/wpt.md (e.g., the sentence referencing RefTestImplementation)
is ignored by the checker; add the entry PNGs = "PNGs" under
[default.extend-words] to whitelist the correct plural form.
In `@fixtures/test-html/README.md`:
- Around line 117-120: The README currently uses two invocation forms for the
grida_wpt CLI ("grida_wpt render --suite" and "cargo run -p grida_wpt -- render
--suite"); standardize these so contributors without the binary on PATH won't
get "command not found": either replace the bare "grida_wpt render --suite"
occurrences with "cargo run -p grida_wpt -- render --suite" or add a one-line
note before both instructions explaining how to build/install the binary (so the
bare form works); update the references to "grida_wpt render --suite" and the
longer "cargo run -p grida_wpt -- render --suite" accordingly (also apply the
same change to the other occurrence around lines 159-163).
---
Nitpick comments:
In @.agents/skills/cg-reftest/scripts/refbrowser_render.ts:
- Line 152: The regex used to strip extensions in the stem calculation is
correct but reorder alternation for readability: change the replace call on
path.basename(entry.path) inside the const stem assignment so the alternation
lists "xhtml" before "xht" (e.g. use an alternation like xhtml|html?|xht) and
make the same change at the other occurrence noted (the second replace around
line 261) so both uses read consistently and match the Rust ordering.
In `@Cargo.toml`:
- Line 8: The workspace lists the new crate as "grida_wpt" which deviates from
the hyphenated naming used elsewhere; update the workspace member entry from
"crates/grida_wpt" to the hyphenated form "crates/grida-wpt" and also update the
crate's own Cargo.toml package.name (currently "grida_wpt") to "grida-wpt" so
naming is consistent across the workspace while Cargo still normalizes the crate
identifier to grida_wpt at compile time; ensure any explicit binary `name =
"grida_wpt"` entries are changed to `name = "grida-wpt"` as well.
In `@crates/csscascade/examples/exp_impl_telement.rs`:
- Around line 1476-1503: The example duplicates the library DOM/adapter
implementations (the TElement methods ensure_data, clear_data, has_data,
borrow_data, mutate_data) causing maintenance drift; fix by removing the
duplicated method bodies in exp_impl_telement.rs and either (A) delegate those
methods to the library implementations (call into the crate::adapter /
crate::dom equivalents) so the example re-uses the real code, or (B) gate the
example behind a dedicated feature (e.g., #[cfg(feature =
"example_playground")]) and document enabling it; update the TElement impl to
call the shared functions (or conditionally compile the whole example) so the
example no longer silently drifts from adapter.rs/dom.rs.
In `@crates/grida_wpt/src/fetch.rs`:
- Around line 32-48: The stem_from_url function currently uses raw
percent-encoded path segments (variable last); decode that segment before
lowercasing and extension stripping to avoid percent-encoding leaking into
stems. Update stem_from_url to percent-decode last (e.g., via
percent_encoding::percent_decode_str(last).decode_utf8_lossy()), then operate on
the decoded string for to_ascii_lowercase(), strip_suffix checks, and final
return (ensure you still return the original-cased decoded segment trimmed to
the stem length when a suffix is removed).
In `@crates/grida_wpt/src/main.rs`:
- Around line 129-139: The fixture-mode render_one currently ignores
RenderArgs.out and calls render::render_with_extras directly, producing files
under the default out dir; update render_one to compute the final output path
using the existing resolve_out logic (or honor args.out) before calling
render_with_extras so the produced PNG goes to the user-specified location;
locate the function render_one and replace the direct use of out_dir with the
resolved path (still passing fonts and the cache) so --out is respected in
fixture mode.
In `@crates/grida_wpt/src/render.rs`:
- Around line 143-154: The function render_directory currently uses
std::fs::read_dir (non-recursive) to collect HTML files into entries, so nested
fixtures under subdirectories are intentionally skipped; update the doc comment
on render_directory to explicitly state this depth-1 (non-recursive) semantics,
mention that it only scans the immediate directory entries (not recursive
traversal), and suggest alternatives (e.g., use a recursive walker like
walkdir::WalkDir or call render_directory for each subtree) so callers know how
to handle typical WPT layouts with nested files such as reftest/box/*.html.
In `@crates/grida-canvas/src/html/mod.rs`:
- Around line 1138-1152: The color probe that builds border_color should skip a
side if that side's computed width is zero; update the selection logic in the
block using css_color_to_cg(&border.border_top_color) / _right_color /
_bottom_color / _left_color to first check the corresponding width (e.g.,
top_w/right_w/bottom_w/left_w as produced by width_for or however widths are
named) and only call css_color_to_cg for sides with width > 0.0 so invisible
sides (width zero or style none) are not considered when choosing the primary
stroke color.
In `@crates/grida-canvas/src/htmlcss/collect.rs`:
- Line 1024: The current assignment drops the reference-box from
clone_overflow_clip_margin() by only using .offset.px(); capture the full result
of style.get_margin().clone_overflow_clip_margin() into a local (preserving both
the reference-box keyword and the offset) and update the element representation
and paint-path to carry that reference-box alongside the offset instead of
coercing it away; specifically, adjust the usage around get_margin(),
clone_overflow_clip_margin(), and the el.overflow_clip_margin assignment so the
reference-box keyword is threaded through (or add a new field on el to hold the
reference-box) and ensure downstream paint code consumes that new value.
In `@crates/grida-canvas/src/htmlcss/layout.rs`:
- Around line 129-140: The inline-block emulation change (inside the block gated
by should_emulate_inline_block_container) needs an inline comment documenting
two limitations and an explicit justify_content pin: note that using
style.display = taffy::Display::Flex with style.flex_wrap and style.align_items
= Start will not reproduce per-item vertical-align (baseline/middle/bottom)
semantics and that callers should prefer vertical-align: top to match the
fixture; also explicitly set style.justify_content =
Some(taffy::JustifyContent::FlexStart) here (and mention element_to_taffy_style
as the place that otherwise writes justify_content unconditionally) so the
emulation does not accidentally inherit non-Start justification.
- Around line 192-210: Add a brief comment in
should_emulate_inline_block_container explaining that whitespace-only
text/inline groups are filtered earlier during tree construction (see
collect.rs), so the function can safely treat any StyledNode::Text or
StyledNode::InlineGroup as a fatal case; reference the function name
should_emulate_inline_block_container and the upstream filter in collect.rs in
the comment so future maintainers understand the invariant.
In `@docs/contributing/wpt.md`:
- Around line 55-58: The markdown contains placeholder reference links
([inventory] and the "adoption plan" reference) with trailing <!-- TODO -->
comments causing markdownlint MD052; update the references in
docs/contributing/wpt.md by either replacing the reference-style links
([inventory], the adoption-plan reference) with inline italicized/plain text
placeholders or by removing the HTML TODO comment and inserting the final target
URLs so the reference labels are valid; ensure you remove or fix the dangling
reference labels rather than leaving them commented, so markdownlint no longer
reports MD052.
- Around line 1-2: Add a top-matter entry setting format: md to this markdown
file to opt out of MDX parsing; specifically, insert a frontmatter block at the
very top of "Contributing to Grida | Web Platform Tests" that includes format:
md so raw angle-bracket code spans like `<meta name="fuzzy">` and `<link
rel="stylesheet">` (and the occurrences around lines noted: the cross-line span
at 115-116 and the ones at 92-93, 115-117) are treated as plain markdown rather
than MDX.
In `@justfile`:
- Around line 45-53: The wpt recipe should fail fast with a clear message when
the sibling wpt checkout or the wpt runner is missing: add a preflight check in
the wpt recipe (before cd ../wpt) that verifies the ../wpt directory exists and
that the wpt runner is present and executable (the ./wpt used after cd ../wpt),
and if not, print an actionable error and exit; keep the existing use of
{{justfile_directory()}} for absolute paths but ensure the check references the
sibling ../wpt and its ./wpt binary so users see a clear “missing checkout or
non-executable wpt” message instead of “./wpt: No such file or directory”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 934b99a8-50e0-4b39-bfa8-b01af3b22fa9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.agents/skills/cg-reftest/SKILL.md.agents/skills/cg-reftest/scripts/refbrowser_render.ts.agents/skills/dev-cg-htmlcss-feature/SKILL.mdCargo.tomlcrates/csscascade/Cargo.tomlcrates/csscascade/examples/exp_impl_telement.rscrates/csscascade/src/adapter.rscrates/csscascade/src/cascade.rscrates/csscascade/src/dom.rscrates/csscascade/src/tree/mod.rscrates/grida-canvas/Cargo.tomlcrates/grida-canvas/examples/golden_htmlcss.rscrates/grida-canvas/src/html/mod.rscrates/grida-canvas/src/htmlcss/collect.rscrates/grida-canvas/src/htmlcss/layout.rscrates/grida_wpt/Cargo.tomlcrates/grida_wpt/README.mdcrates/grida_wpt/src/fetch.rscrates/grida_wpt/src/lib.rscrates/grida_wpt/src/main.rscrates/grida_wpt/src/render.rscrates/grida_wpt/src/suite.rscrates/grida_wpt/src/wpt/mod.rsdocs/contributing/wpt.mdfixtures/test-html/L0/layout-display-inline-block.htmlfixtures/test-html/README.mdfixtures/test-html/suites/L0.exact.jsonjustfile
💤 Files with no reviewable changes (1)
- crates/grida-canvas/examples/golden_htmlcss.rs
| #[derive(Debug, clap::Args)] | ||
| struct RenderArgs { | ||
| /// Suite JSON file. Fixtures and `extra_css` resolve relative to it. | ||
| #[arg(long, value_name = "PATH", conflicts_with_all = ["fixture", "dir", "url"])] | ||
| suite: Option<PathBuf>, | ||
|
|
||
| /// Single HTML fixture to render (ad-hoc, no suite context). | ||
| #[arg(long, value_name = "PATH", conflicts_with_all = ["dir", "url"])] | ||
| fixture: Option<PathBuf>, | ||
|
|
||
| /// Directory of HTML fixtures to render at default viewport. | ||
| /// Accepts .html, .htm, .xht, and .xhtml files. | ||
| #[arg(long, value_name = "DIR", conflicts_with = "url")] | ||
| dir: Option<PathBuf>, | ||
|
|
||
| /// URL to fetch and render (WPT serves tests over localhost). The | ||
| /// HTML body is fetched via blocking HTTP and rendered at the | ||
| /// default viewport. External assets referenced by the page | ||
| /// (stylesheets, images) are NOT resolved — see adoption plan P4. | ||
| #[arg(long, value_name = "URL")] | ||
| url: Option<String>, | ||
|
|
||
| /// Explicit output file path. Overrides `--out-dir` when set; | ||
| /// intended for `wptrunner` which names PNGs deterministically. | ||
| #[arg(long, value_name = "FILE", conflicts_with = "out_dir")] | ||
| out: Option<PathBuf>, | ||
|
|
||
| /// Output directory for rendered PNGs. Default: | ||
| /// `${TMPDIR}/grida-htmlcss-goldens`. | ||
| #[arg(long, value_name = "DIR")] | ||
| out_dir: Option<PathBuf>, | ||
|
|
||
| /// Viewport width override (px). Default from suite / built-in. | ||
| #[arg(long, value_name = "PX")] | ||
| width: Option<f32>, | ||
|
|
||
| /// Viewport height override (px). Default from suite / built-in. | ||
| #[arg(long, value_name = "PX")] | ||
| height: Option<f32>, | ||
| } |
There was a problem hiding this comment.
Minor: --width/--height are silently ignored with --suite.
The help text says "Default from suite / built-in", but render::render_suite resolves the viewport per fixture from the suite JSON and never sees args.width/args.height. A user passing --suite ... --width 800 will get the suite defaults with no error or warning.
Either thread overrides into render_suite, mark the options conflicts_with = "suite", or adjust the help text to "Used only with --fixture/--dir/--url".
🧱 Suggested fix (help text + clap conflicts)
- /// Viewport width override (px). Default from suite / built-in.
- #[arg(long, value_name = "PX")]
+ /// Viewport width override (px). Used only with --fixture / --dir / --url;
+ /// ignored for --suite (the suite JSON sets per-fixture viewports).
+ #[arg(long, value_name = "PX", conflicts_with = "suite")]
width: Option<f32>,
- /// Viewport height override (px). Default from suite / built-in.
- #[arg(long, value_name = "PX")]
+ /// Viewport height override (px). Used only with --fixture / --dir / --url;
+ /// ignored for --suite.
+ #[arg(long, value_name = "PX", conflicts_with = "suite")]
height: Option<f32>,Also applies to: 85-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida_wpt/src/main.rs` around lines 37 - 76, RenderArgs' width/height
are ignored when running with a suite because render::render_suite computes
per-fixture viewports; update RenderArgs so the CLI clearly prevents or forwards
those flags: either add conflicts_with = "suite" to the width and height args
(or to the suite arg) so clap rejects --suite + --width/--height, or change the
width/height help text to state "Used only with --fixture/--dir/--url" and
document that render::render_suite ignores them; apply the same change to the
duplicate RenderArgs block around the other occurrence (lines ~85-112) so
behavior/help is consistent.
| /// Missing files warn once to stderr and contribute nothing to | ||
| /// injection (callers skip absent keys silently). | ||
| fn ensure_css_cached(cache: &mut HashMap<PathBuf, String>, abs: &Path) { | ||
| if let Entry::Vacant(slot) = cache.entry(abs.to_path_buf()) { | ||
| match std::fs::read_to_string(abs) { | ||
| Ok(s) => { | ||
| slot.insert(s); | ||
| } | ||
| Err(e) => eprintln!(" warn: failed to read {}: {e}", abs.display()), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
warn once claim doesn't match runtime behavior — re-warns per fixture.
On I/O error the function doesn't insert anything, so the entry stays Vacant and every subsequent fixture re-reads the missing file and re-emits the warning. For a 64-fixture suite pointing at a missing extra_css, you'll get 64 failed reads and 64 warnings. Poison the slot with an empty string (or a sentinel) so the attempt is actually made only once.
🛠️ Proposed fix
fn ensure_css_cached(cache: &mut HashMap<PathBuf, String>, abs: &Path) {
if let Entry::Vacant(slot) = cache.entry(abs.to_path_buf()) {
match std::fs::read_to_string(abs) {
Ok(s) => {
slot.insert(s);
}
- Err(e) => eprintln!(" warn: failed to read {}: {e}", abs.display()),
+ Err(e) => {
+ eprintln!(" warn: failed to read {}: {e}", abs.display());
+ // Poison the slot so we don't retry/re-warn per fixture.
+ slot.insert(String::new());
+ }
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida_wpt/src/render.rs` around lines 29 - 40, The current
ensure_css_cached function leaves the HashMap entry vacant on read error so
every caller retries and re-warns; update ensure_css_cached (the cache: &mut
HashMap<PathBuf, String> function) to insert a sentinel value into the vacated
slot on Err (e.g., empty string or specific marker) after logging the eprintln,
so Entry::Vacant is no longer present and subsequent calls will not re-read or
re-emit the warning; keep the existing success path (slot.insert(s)) and the
error log using abs.display().
| pub fn render_to_png( | ||
| html: &str, | ||
| width: f32, | ||
| height: f32, | ||
| name: &str, | ||
| out_dir: &Path, | ||
| fonts: &FontRepository, | ||
| ) { | ||
| let picture = | ||
| htmlcss::render(html, width, height, fonts, &htmlcss::NoImages).expect("render failed"); | ||
| // Full-viewport dims match Chromium's fullPage footprint; transparent clear | ||
| // lets PNG alpha double as the reftest content mask. | ||
| let w = width.max(1.0) as i32; | ||
| let h = height.max(1.0) as i32; | ||
|
|
||
| let mut surface = surfaces::raster_n32_premul((w, h)).expect("surface"); | ||
| let canvas = surface.canvas(); | ||
| canvas.clear(Color::TRANSPARENT); | ||
| canvas.draw_picture(&picture, None, None); | ||
|
|
||
| let image = surface.image_snapshot(); | ||
| let data = image | ||
| .encode(None, skia_safe::EncodedImageFormat::PNG, None) | ||
| .unwrap(); | ||
| let path = out_dir.join(format!("{name}.png")); | ||
| std::fs::write(&path, data.as_bytes()) | ||
| .unwrap_or_else(|e| panic!("failed to write {}: {e}", path.display())); | ||
| eprintln!(" {name}: {w}x{h} → {}", path.display()); | ||
| } |
There was a problem hiding this comment.
A single broken fixture aborts the whole suite run.
Every step here (render, raster_n32_premul, encode, fs::write) panics on error. In render_suite / render_directory, one pathological fixture takes the whole batch down, which is exactly the scenario a WPT-style runner has to survive. Consider returning Result<(), String> from render_to_png (and render_with_extras) so the suite loop can log and continue; at the very least, interpolate name/html_path into the panic messages so failures in a 64+ fixture run are diagnosable without rerunning.
🧰 Sketch — Result-returning API and per-fixture continuation
-pub fn render_to_png(
+pub fn render_to_png(
html: &str,
width: f32,
height: f32,
name: &str,
out_dir: &Path,
fonts: &FontRepository,
-) {
+) -> Result<(), String> {
let picture =
- htmlcss::render(html, width, height, fonts, &htmlcss::NoImages).expect("render failed");
+ htmlcss::render(html, width, height, fonts, &htmlcss::NoImages)
+ .map_err(|e| format!("{name}: render failed: {e}"))?;
let w = width.max(1.0) as i32;
let h = height.max(1.0) as i32;
- let mut surface = surfaces::raster_n32_premul((w, h)).expect("surface");
+ let mut surface = surfaces::raster_n32_premul((w, h))
+ .ok_or_else(|| format!("{name}: failed to allocate {w}x{h} surface"))?;
let canvas = surface.canvas();
canvas.clear(Color::TRANSPARENT);
canvas.draw_picture(&picture, None, None);
let image = surface.image_snapshot();
let data = image
.encode(None, skia_safe::EncodedImageFormat::PNG, None)
- .unwrap();
+ .ok_or_else(|| format!("{name}: PNG encode failed"))?;
let path = out_dir.join(format!("{name}.png"));
std::fs::write(&path, data.as_bytes())
- .unwrap_or_else(|e| panic!("failed to write {}: {e}", path.display()));
+ .map_err(|e| format!("failed to write {}: {e}", path.display()))?;
eprintln!(" {name}: {w}x{h} → {}", path.display());
+ Ok(())
}Callers in render_suite / render_directory can then swallow per-fixture Err with eprintln! and continue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida_wpt/src/render.rs` around lines 43 - 71, Change render_to_png
(and likewise render_with_extras) to return Result<(), String> instead of
panicking: propagate errors from htmlcss::render, surfaces::raster_n32_premul,
image.encode, and std::fs::write by mapping them into Err with context that
includes the fixture identifier (name and/or html_path), e.g., "render failed
for {name}: {err}". Remove all expect/unwrap/panic calls in render_to_png and
return descriptive Err strings; update callers in render_suite /
render_directory to log eprintln! on Err and continue with the next fixture.
| let name = html_path | ||
| .file_stem() | ||
| .map(|s| s.to_string_lossy().to_string()) | ||
| .unwrap_or_else(|| "unknown".to_string()); |
There was a problem hiding this comment.
unknown.png fallback can silently collide in batch mode.
If two fixtures ever resolve to paths without a file_stem, both render to the same unknown.png — the second silently overwrites the first. Very unlikely in practice, but in a test-oracle context I'd rather surface it. Consider deriving a unique stem (e.g. hash of full path) or returning an error instead of swallowing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida_wpt/src/render.rs` around lines 91 - 94, The current fallback
sets name to "unknown" when html_path.file_stem() is None, which can cause
collisions; update the logic that defines the variable name (the let name = ...
block using html_path) to produce a unique fallback instead of a constant
"unknown" — for example, compute a short hash (e.g., SHA256 or xxhash) of the
full html_path (or its display_string) and use that hash as the stem when
file_stem() is None, or alternatively return an Err from the surrounding
function (render) to surface the problem; ensure you modify the code that
constructs name so it uses html_path.file_stem().map(...).unwrap_or_else(||
fallback_from_hash(html_path)) and keep variable name and surrounding
error-handling consistent with the function's return type.
| pub fn resolve_entry( | ||
| entry: &SuiteEntry, | ||
| defaults: &FixtureDefaults, | ||
| suite_dir: &Path, | ||
| ) -> ResolvedFixture { | ||
| let html = suite_dir.join(&entry.path); | ||
| let css_rel: &[String] = entry.extra_css.as_deref().unwrap_or(&defaults.extra_css); | ||
| let extra_css: Vec<PathBuf> = css_rel.iter().map(|r| suite_dir.join(r)).collect(); | ||
| let vp = entry.viewport.unwrap_or(defaults.viewport); | ||
| let width = vp.width.unwrap_or(DEFAULT_WIDTH); | ||
| let height = vp.height.unwrap_or(DEFAULT_HEIGHT); | ||
| ResolvedFixture { | ||
| html, | ||
| extra_css, | ||
| width, | ||
| height, | ||
| } | ||
| } |
There was a problem hiding this comment.
Viewport inheritance is all-or-nothing; doc claims per-field inherit.
entry.viewport.unwrap_or(defaults.viewport) replaces the whole viewport wholesale, so a fixture that only overrides one axis silently loses the other axis's default:
// suite defaults: { viewport: { width: 600, height: 800 } }
// fixture: { viewport: { width: 800 } }
// result today: width=800, height=600 (DEFAULT_HEIGHT) ← not 800The doc comment says "Viewport width/height inherit from defaults and fall back to the built-in defaults", which reads like per-field inheritance. Either tighten the doc or merge per-field.
🛠️ Proposed fix — per-field fallback chain
- let vp = entry.viewport.unwrap_or(defaults.viewport);
- let width = vp.width.unwrap_or(DEFAULT_WIDTH);
- let height = vp.height.unwrap_or(DEFAULT_HEIGHT);
+ let entry_vp = entry.viewport.unwrap_or_default();
+ let width = entry_vp
+ .width
+ .or(defaults.viewport.width)
+ .unwrap_or(DEFAULT_WIDTH);
+ let height = entry_vp
+ .height
+ .or(defaults.viewport.height)
+ .unwrap_or(DEFAULT_HEIGHT);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida_wpt/src/suite.rs` around lines 74 - 91, The current
resolve_entry replaces the entire viewport when entry.viewport is Some, losing
per-field defaults; change resolution to merge per-field: read entry.viewport as
Option<&Viewport> and compute width = entry.viewport.as_ref().and_then(|v|
v.width).or(defaults.viewport.width).unwrap_or(DEFAULT_WIDTH) and similarly
height = entry.viewport.as_ref().and_then(|v|
v.height).or(defaults.viewport.height).unwrap_or(DEFAULT_HEIGHT), then use those
width/height values in ResolvedFixture; update resolve_entry (and references to
entry.viewport, defaults.viewport, DEFAULT_WIDTH, DEFAULT_HEIGHT) accordingly so
each axis falls back first to fixture, then suite defaults, then built-in
constants.
| [wpt]: https://web-platform-tests.org/ | ||
| [cg-reftest]: ../../.agents/skills/cg-reftest/SKILL.md |
There was a problem hiding this comment.
Links outside /docs violate the docs coding guideline — convert to inline code.
The guideline forbids linking outside /docs from docs markdown files; external paths must be referenced as inline code instead. The following links point into .agents/ and crates/ and need to be rewritten:
- Line 14:
[cg-reftest]: ../../.agents/skills/cg-reftest/SKILL.md - Lines 97-98:
[crates/grida_wpt/](../../crates/grida_wpt/) - Lines 151-153:
[grida_wpt crate README](../../crates/grida_wpt/README.md)
📝 Suggested rewrites
-refbrowser pipeline (see [cg-reftest skill][cg-reftest]).
+refbrowser pipeline (see the `.agents/skills/cg-reftest/SKILL.md` skill).
@@
-[cg-reftest]: ../../.agents/skills/cg-reftest/SKILL.md-- **Rust side:** [`crates/grida_wpt/`](../../crates/grida_wpt/) in
- this repo. The `render` subcommand handles `--url` mode.
+- **Rust side:** `crates/grida_wpt/` in this repo. The `render`
+ subcommand handles `--url` mode.-- [grida_wpt crate README](../../crates/grida_wpt/README.md) — Rust
- side usage (including the refbrowser-pipeline use of `grida_wpt
-render --suite`).
+- `crates/grida_wpt/README.md` — Rust side usage (including the
+ refbrowser-pipeline use of `grida_wpt render --suite`).As per coding guidelines: "Never link outside /docs from docs markdown files; instead reference external paths as inline code".
Also applies to: 96-101, 149-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contributing/wpt.md` around lines 13 - 14, The docs use reference-style
links that point outside /docs which violates the guideline; locate the
reference labels [cg-reftest], [crates/grida_wpt/], and [grida_wpt crate README]
and replace any uses of those reference-style links with inline code containing
the external path (e.g., `../../.agents/skills/cg-reftest/SKILL.md`,
`../../crates/grida_wpt/`, `../../crates/grida_wpt/README.md`) and remove the
corresponding reference definitions so no link definitions point outside /docs.
| ``` | ||
| Apps/grida/ | ||
| ├── grida/ # this repo | ||
| └── wpt/ # the fork | ||
| ``` |
There was a problem hiding this comment.
Missing fenced-code language (markdownlint MD040).
📝 Add a language tag to the directory-layout fence
-```
+```text
Apps/grida/
├── grida/ # this repo
└── wpt/ # the fork</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/contributing/wpt.md around lines 31 - 35, The fenced code block showing
the directory layout is missing a language tag; update the triple-backtick fence
that precedes the "Apps/grida/" directory tree to include a language identifier
(use "text") so the block becomes ```text, ensuring markdownlint MD040 is
satisfied; no other content changes needed.
</details>
<!-- fingerprinting:phantom:medusa:nectarine:d1dc1682-c547-4821-9507-5cb4eb3d1414 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| `RefTestImplementation` compares the two PNGs, honoring `<meta | ||
| name="fuzzy">` tolerances. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Re-run typos exactly on the changed doc and print the offending lines with context.
fd -t f 'wpt.md$' docs/contributing || true
typos --config typos.toml docs/contributing/wpt.md || typos docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 190
🏁 Script executed:
# Read the file and show lines around 92-93 with context
wc -l docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 86
🏁 Script executed:
# Show lines 85-100 to get full context around the flagged area
sed -n '85,100p' docs/contributing/wpt.md | cat -nRepository: gridaco/grida
Length of output: 793
🏁 Script executed:
# Search for common typo patterns: PN and ON occurrences
rg '(PN|ON)' docs/contributing/wpt.md -nRepository: gridaco/grida
Length of output: 123
🏁 Script executed:
# Read more of the file to find potential typos
cat -n docs/contributing/wpt.md | head -100Repository: gridaco/grida
Length of output: 3972
🏁 Script executed:
# Search for patterns that might trigger typos check
rg 'PN|ON' docs/contributing/wpt.md -B2 -A2 -nRepository: gridaco/grida
Length of output: 282
🏁 Script executed:
# Look for common misspellings and suspicious words
rg '\bur\b|\bor\b|\bud\b|\bte\b|\bof\b|\bin\b' docs/contributing/wpt.md | head -20Repository: gridaco/grida
Length of output: 1130
🏁 Script executed:
# Look for potential typos: common misspellings
rg '\b(recieve|occured|seperate|neccessary|definately|occassion|wich|thier|begining)\b' docs/contributing/wpt.md -iRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Search for PN or ON as complete tokens/words to see context
rg '\b(PN|ON)\b' docs/contributing/wpt.md -n -B2 -A2Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look at lines 88-100 more carefully for all text
sed -n '88,100p' docs/contributing/wpt.md | cat -nRepository: gridaco/grida
Length of output: 703
🏁 Script executed:
# Read the entire file to spot obvious typos
cat docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 5370
🏁 Script executed:
# Search for words that might be commonly misspelled (longer view)
rg -i 'occured|recieve|seperate|neccessary|definately|thier|wich|begining|adress|bussiness|occassion|untill|dissapear|goverment|enviroment|exeption|existance|freind|independant|knowlege|licesce|maintance|occassion|ocassion|persue|postion|preform|recomend|refrence|reccommend|sheduled|statment|strengh|succeded|suficient|thoughout|tommorow|untill|usuall|withold' docs/contributing/wpt.md -nRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look for potential typos by checking for odd character patterns
rg '[a-z]{2,}[A-Z][a-z]' docs/contributing/wpt.md -nRepository: gridaco/grida
Length of output: 123
🏁 Script executed:
# Check the beginning of the file for frontmatter
head -20 docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 853
🏁 Script executed:
# Search for any potential typo patterns more carefully
rg 'upon|upon' docs/contributing/wpt.md -nRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look for anything that could be flagged as PN/ON related
rg '\bpon\b|\bpon' docs/contributing/wpt.md -i -nRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look for typos configuration
find . -name "typos.toml" -o -name ".typos.toml" | head -5Repository: gridaco/grida
Length of output: 71
🏁 Script executed:
# Search for any obvious typos in the specific line range with extended context
sed -n '90,95p' docs/contributing/wpt.md | od -c | head -50Repository: gridaco/grida
Length of output: 98
🏁 Script executed:
# Check if the issue could be in surrounding context (lines 85-98)
sed -n '85,98p' docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 543
🏁 Script executed:
# Check the typos configuration
cat .typos.tomlRepository: gridaco/grida
Length of output: 1742
🏁 Script executed:
# Look for any special characters or encoding issues around line 92
hexdump -C docs/contributing/wpt.md | sed -n '2920,3000p'Repository: gridaco/grida
Length of output: 103
🏁 Script executed:
# Check git status to see what files are changed
git status docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 123
🏁 Script executed:
# Check for git diff to see what was actually changed
git diff docs/contributing/wpt.md | head -100Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look for common typo patterns that typos checker looks for
# Check around the reported line for obvious misspellings
rg 'teh|hte|recieve|occured|seperate|neccessary' docs/contributing/wpt.md -i -nRepository: gridaco/grida
Length of output: 203
🏁 Script executed:
# Try to install typos and run it
pip install typos 2>/dev/null || npm install -g typos 2>/dev/null || echo "Cannot install typos in sandbox"Repository: gridaco/grida
Length of output: 397
🏁 Script executed:
# Look for patterns that might trigger substring matching issues
# Check if "upon" is in the file (substring of "upon" could match "PN" -> "ON")
grep -n 'upon' docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look at raw bytes around line 92 to spot encoding issues
python3 << 'EOF'
with open('docs/contributing/wpt.md', 'rb') as f:
content = f.read()
# Find line 92
lines = content.split(b'\n')
for i in range(88, min(95, len(lines))):
print(f"Line {i+1}: {lines[i]}")
print(f" Hex: {lines[i].hex()}")
EOFRepository: gridaco/grida
Length of output: 1050
🏁 Script executed:
# Run typos on the specific file
typos docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 103
🏁 Script executed:
# Run with verbose output to see the exact offending token
typos --dump-config docs/contributing/wpt.md 2>&1 | head -50Repository: gridaco/grida
Length of output: 101
🏁 Script executed:
# Find where typos was installed
python3 -m pip show typosRepository: gridaco/grida
Length of output: 258
🏁 Script executed:
# Try running it as a python module
python3 -m typos docs/contributing/wpt.mdRepository: gridaco/grida
Length of output: 97
🏁 Script executed:
# Or try the full path
~/.local/bin/typos docs/contributing/wpt.md 2>&1 || python3 -c "from typos import typos_cli; typos_cli.main()" docs/contributing/wpt.md 2>&1Repository: gridaco/grida
Length of output: 362
🏁 Script executed:
# Confirm the exact error and check the typos.toml allowlist
~/.local/bin/typos docs/contributing/wpt.md 2>&1 || echo "---"
grep -i "png" .typos.tomlRepository: gridaco/grida
Length of output: 247
🏁 Script executed:
# Check what the typos checker considers valid for this pattern
grep -A 5 -B 5 "PN\|ON" .typos.toml || echo "Pattern not found in typos.toml"Repository: gridaco/grida
Length of output: 89
Add PNGs to the typos allowlist in .typos.toml.
The typos checker correctly flags line 92, column 42: the substring PN within PNGs is being matched as a typo (should be ON). This is a false positive — "PNGs" is the standard plural of PNG (Portable Network Graphics). Add the following to .typos.toml under [default.extend-words]:
PNGs = "PNGs"
🧰 Tools
🪛 GitHub Actions: typos
[error] 92-92: typos: PN should be ON
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contributing/wpt.md` around lines 92 - 93, The typos checker is falsely
flagging the substring "PN" in "PNGs" as a typo; update the typos allowlist by
adding the plural "PNGs" to the [default.extend-words] section of .typos.toml so
the term used in docs/contributing/wpt.md (e.g., the sentence referencing
RefTestImplementation) is ignored by the checker; add the entry PNGs = "PNGs"
under [default.extend-words] to whitelist the correct plural form.
| To find the right height, render the fixture once with | ||
| `golden_htmlcss --suite` and read the reported `WxH`. Update the | ||
| `grida_wpt render --suite` and read the reported `WxH`. Update the | ||
| suite entry's `viewport.height` to match. Re-render refbrowser; both | ||
| sides should now be at identical dimensions. |
There was a problem hiding this comment.
Minor: inconsistent invocation form for grida_wpt.
Line 118 uses the bare grida_wpt render --suite form, while line 159 uses cargo run -p grida_wpt -- render --suite …. Contributors without the binary on PATH will hit a "command not found" error on the first instruction. Either standardize both on cargo run -p grida_wpt -- render --suite (safer for a fresh checkout) or add a one-line note pointing to the build step that makes grida_wpt available on PATH.
Also applies to: 159-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fixtures/test-html/README.md` around lines 117 - 120, The README currently
uses two invocation forms for the grida_wpt CLI ("grida_wpt render --suite" and
"cargo run -p grida_wpt -- render --suite"); standardize these so contributors
without the binary on PATH won't get "command not found": either replace the
bare "grida_wpt render --suite" occurrences with "cargo run -p grida_wpt --
render --suite" or add a one-line note before both instructions explaining how
to build/install the binary (so the bare form works); update the references to
"grida_wpt render --suite" and the longer "cargo run -p grida_wpt -- render
--suite" accordingly (also apply the same change to the other occurrence around
lines 159-163).
1 participant
Summary
Promotes the HTML/CSS golden producer out of a
cg --exampleinto a lean, dedicated crate and wires it up to the upstream Web Platform Tests via a wptrunner product plugin (living in thegridaco/wptfork).crates/grida_wptwith clap CLI (rendersubcommand;--suite/--fixture/--dir/--urlmodes). Lean deps — no winit/GL — so external runners can invoke it cheaply.gridaco/wpt@d9b9582. Setup steps indocs/contributing/wpt.md.cg --example golden_htmlcssoutput..xht/.xhtmlstem support inrefbrowser_render.tsso WPT-style filenames round-trip.Why
After ~70 iterations of
/dev-cg-htmlcss-featurethe in-tree L0 corpus hit diminishing returns. WPT is the industry-standard CSS spec-conformance oracle (used by Chromium/Gecko/WebKit/Servo/Ladybird). This PR lands the plumbing end-to-end; most tests will fail until tier-0 primitives (inline SVG, external stylesheets, Ahem font) land — tracked in~/Desktop/grida-wpt/todo.md.Test plan
cargo test -p grida_wpt— 4/4 pass (URL stem parsing).cargo clippy -p grida_wpt --no-deps— clean.css/css-transforms/2d-rotate-001.html—SUITE_START→TEST_START→TEST_END: FAIL(expected; ref.html uses inline SVG which cg doesn't paint yet).Status
Draft — opening for visibility while tier-0 primitive work starts. Reftest-only; testharness/crashtest executors not wired.
See also
README-GRIDA.mdand the plugin files)~/Desktop/grida-wpt/{grida-wpt-adoption-plan,grida-wpt-peer-research,ref-primitives-inventory}.mdSummary by CodeRabbit
Release Notes
New Features
grida_wpt rendercommand for rendering HTML/CSS fixtures to PNGBug Fixes
noneorhiddenstyles as zero-width.xhtand.xhtmlformatsDocumentation
Chores