Files
rust_browser/_bmad-output/implementation-artifacts/1-13-css-inheritance-completeness.md
Zachary D. Rowitsch 85db5e559f Implement CSS 2.1 inheritance completeness with code review fixes (§6.2, §6.2.1, §6.2.4)
Add CssValue::Unset variant with proper cascade semantics: acts as inherit
for inherited properties, initial for non-inherited. Fix unset/inherit/initial
handling in font, list-style, and text-decoration shorthand expansion. Add
unset handling to both pseudo-element code paths. Promote WPT test
wpt-css-css2-normal-flow-inlines-002 (now passes with font shorthand fix).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-14 12:19:51 -04:00

343 lines
21 KiB
Markdown

# Story 1.13: CSS Inheritance Completeness
Status: done
<!-- Note: Validation is optional. Run validate-create-story for quality check before dev-story. -->
## Story
As a web user,
I want CSS property values to correctly inherit, reset, and cascade,
So that styling behaves predictably across nested elements.
## Acceptance Criteria
1. **Given** an element with a property set to `inherit`
**When** the page is rendered
**Then** the element uses its parent's computed value for that property per CSS 2.1 §6.2.1
2. **Given** an element with a property set to `initial`
**When** the page is rendered
**Then** the element uses the property's initial value per its specification
3. **Given** an element with a property set to `unset`
**When** the page is rendered
**Then** the property behaves as `inherit` if it's an inherited property, or `initial` if it's not
4. **Given** all CSS 2.1 properties
**When** checking inheritance behavior
**Then** each property correctly defaults to inherited or non-inherited per its CSS 2.1 specification
5. Golden tests cover inherit/initial/unset for both inherited and non-inherited properties, checklist is updated, and `just ci` passes.
## Tasks / Subtasks
- [x] Task 1: Add `CssValue::Unset` variant and proper handling (AC: #3)
- [x] 1.1 Add `Unset` variant to `CssValue` enum in `crates/css/src/types.rs` (~line 199)
- [x] 1.2 Add parser recognition: in `crates/css/src/parser/mod.rs` (~line 1161, `parse_generic_value` and ~line 1296, `parse_single_value`), add `"unset" => Some(CssValue::Unset)`
- [x] 1.3 Remove incorrect `"unset" | "revert" => Some(CssValue::Inherit)` workarounds in `crates/css/src/parser/shorthands/background.rs` (~line 47) and `typography.rs`
- [x] 1.4 In `crates/style/src/context.rs` (~line 587), add `CssValue::Unset` handling: check `PropertyId::is_inherited()` — if inherited, behave like `inherit` (copy from parent); if not inherited, behave like `initial` (reset to initial value)
- [x] 1.5 In `crates/style/src/types/computed.rs` `apply_declaration()` (~line 500), add early return for `CssValue::Unset` that delegates to the cascade engine (or handle inline if parent context is available)
- [x] 1.6 Add unit tests: `unset` on inherited property (e.g., `color: unset`) behaves like `inherit`; `unset` on non-inherited property (e.g., `margin: unset`) behaves like `initial`
- [x] Task 2: Fix `text-indent` auto-inheritance (AC: #4)
- [x] 2.1 Add `PropertyId::TextIndent` to `is_inherited()` match in `crates/css/src/types.rs` (~line 778) — `text-indent` is inherited per CSS 2.1 §16.1
- [x] 2.2 Add `text_indent` to `inherit_from()` in `crates/style/src/types/computed.rs` (~line 1864) — currently missing despite being supported in `copy_property_from_parent()`
- [x] 2.3 Add unit test: child element inherits `text-indent` from parent without explicit `inherit` keyword
- [x] Task 3: Audit and complete CSS 2.1 inherited property list (AC: #4)
- [x] 3.1 Cross-reference CSS 2.1 spec against `is_inherited()` in `crates/css/src/types.rs` (~line 778). The complete list of CSS 2.1 inherited properties is:
- `azimuth`, `border-collapse`, `border-spacing`, `caption-side`, `color`, `cursor`, `direction`, `elevation`, `empty-cells`, `font-family`, `font-size`, `font-style`, `font-variant`, `font-weight`, `font`, `letter-spacing`, `line-height`, `list-style-image`, `list-style-position`, `list-style-type`, `list-style`, `orphans`, `pitch-range`, `pitch`, `quotes`, `richness`, `speak-header`, `speak-numeral`, `speak-punctuation`, `speak`, `speech-rate`, `stress`, `text-align`, `text-indent`, `text-transform`, `visibility`, `voice-family`, `volume`, `white-space`, `widows`, `word-spacing`
- [x] 3.2 For properties already in ComputedStyles but missing from `is_inherited()`: add them (e.g., `TextIndent`)
- [x] 3.3 For properties NOT yet in ComputedStyles (e.g., `word-spacing`, `cursor`, `direction`, `font-variant`, `empty-cells`, `caption-side`, `list-style-position`, `list-style-image`, `quotes`, `orphans`, `widows`): skip aural/speech properties (out of scope), but note which visual properties are missing for future stories
- [x] 3.4 Ensure `inherit_from()` and `is_inherited()` are kept in sync — every property in `is_inherited()` must be copied in `inherit_from()`
- [x] Task 4: Verify `inherit` keyword works for all properties (AC: #1)
- [x] 4.1 Verify `copy_property_from_parent()` in `crates/style/src/types/computed.rs` (~line 1636) covers all PropertyId variants currently in ComputedStyles
- [x] 4.2 Scan for any PropertyId variants missing from the match statement — these would silently fail when `inherit` is used
- [x] 4.3 Add any missing property arms to `copy_property_from_parent()`
- [x] 4.4 Add unit tests for `inherit` on non-inherited properties (e.g., `border: inherit`, `display: inherit`)
- [x] Task 5: Verify `initial` keyword works for all properties (AC: #2)
- [x] 5.1 Verify `reset_to_initial()` in `crates/style/src/types/computed.rs` (~line 1512) covers all PropertyId variants
- [x] 5.2 Cross-check initial values against CSS 2.1 spec — verify `css_initial_values()` (~line 1758) returns correct values
- [x] 5.3 Add any missing property arms to `reset_to_initial()`
- [x] 5.4 Add unit test for `initial` on inherited properties (e.g., `color: initial` → black, `font-size: initial` → 16px)
- [x] Task 6: Golden tests and documentation (AC: #5)
- [x] 6.1 Create golden test: `inherit` keyword on inherited property (e.g., nested div with `color: inherit`)
- [x] 6.2 Create golden test: `inherit` keyword on non-inherited property (e.g., `border: inherit`)
- [x] 6.3 Create golden test: `initial` keyword resetting inherited property (e.g., `color: initial` overrides parent color)
- [x] 6.4 Create golden test: `unset` on inherited vs non-inherited properties
- [x] 6.5 Create golden test: auto-inheritance of text properties (text-indent, letter-spacing flowing to children without explicit inherit)
- [x] 6.6 Update `docs/CSS2.1_Implementation_Checklist.md` — correct status entries for inheritance items and mark as complete
- [x] 6.7 Run `just ci 2>&1 | tee /tmp/ci-output.txt` and fix any issues
## Dev Notes
### CSS 2.1 Spec References
- **§6.2** Value processing: specified → computed → used → actual values
- **§6.2.1** Specified values: if cascade yields `inherit`, use parent's computed value. If cascade yields no value, use inherited value (for inherited properties) or initial value (for non-inherited).
- **§6.2.4** `inherit` keyword: forces inheritance for any property
- **CSS3 Cascading §3.6** `unset` keyword: acts as `inherit` for inherited properties, `initial` for non-inherited
- **CSS3 Cascading §3.5** `initial` keyword: resets property to its spec-defined initial value
### Current Inheritance Pipeline State
**Already working (DO NOT reimplement):**
- `CssValue::Inherit` and `CssValue::Initial` enum variants in `crates/css/src/types.rs` (~line 207-209)
- Parser recognition of `"inherit"` and `"initial"` keywords in `crates/css/src/parser/mod.rs` (~line 1161, 1296)
- Cascade-level `inherit` handling in `crates/style/src/context.rs` (~line 587) — copies from parent via `copy_property_from_parent()`
- `apply_declaration()` early return for `CssValue::Initial` → calls `reset_to_initial()` in `crates/style/src/types/computed.rs` (~line 500)
- `reset_to_initial()` (~line 1512) — resets any property to its CSS 2.1 initial value
- `css_initial_values()` (~line 1758) — complete struct with all spec-correct initial values
- `copy_property_from_parent()` (~line 1636) — handles ~50 properties for explicit `inherit`
- `inherit_from()` (~line 1864) — auto-inherits 15 properties for normal cascade
- `PropertyId::is_inherited()` (~line 778) — 15 properties marked as inherited
- `!important` handling with correct cascade weight ordering
- `CascadeOrigin::UserAgent` and `CascadeOrigin::Author` with proper priority
**Must be added:**
- `CssValue::Unset` variant + parser recognition + cascade handling
- `text-indent` in `is_inherited()` and `inherit_from()`
- Audit of all CSS 2.1 inherited properties
- Remove incorrect `"unset" => CssValue::Inherit` workarounds in shorthands
- Golden tests for inherit/initial/unset
### Key Code Locations
| Component | File | What to modify |
|---|---|---|
| CssValue enum | `crates/css/src/types.rs` (~line 199) | Add `Unset` variant |
| is_inherited() | `crates/css/src/types.rs` (~line 778) | Add `TextIndent`, audit list |
| Parser keywords | `crates/css/src/parser/mod.rs` (~line 1161, 1296) | Add `"unset"` recognition |
| Background shorthand | `crates/css/src/parser/shorthands/background.rs` (~line 47) | Remove `"unset" => Inherit` workaround |
| Typography shorthand | `crates/css/src/parser/shorthands/typography.rs` (~line 73) | Remove `"unset"` workaround |
| Cascade inherit handler | `crates/style/src/context.rs` (~line 587) | Add `CssValue::Unset` handling |
| apply_declaration | `crates/style/src/types/computed.rs` (~line 500) | Add `Unset` early return |
| reset_to_initial | `crates/style/src/types/computed.rs` (~line 1512) | Verify all properties covered |
| copy_property_from_parent | `crates/style/src/types/computed.rs` (~line 1636) | Verify all properties covered |
| inherit_from | `crates/style/src/types/computed.rs` (~line 1864) | Add `text_indent` |
| css_initial_values | `crates/style/src/types/computed.rs` (~line 1758) | Verify correctness |
| Golden fixtures | `tests/goldens/fixtures/` | New inherit/initial/unset fixtures |
| Checklist | `docs/CSS2.1_Implementation_Checklist.md` (~line 65) | Update inheritance items |
### Architecture Constraints
- **No unsafe code** — all changes in Layer 1 crates only (css, style)
- **Cascade engine design** — `inherit` and `unset` must be handled in the cascade engine (`context.rs`) because that's where parent context is available. `apply_declaration()` in `computed.rs` handles `initial` because it doesn't need parent context.
- **Phase-based pipeline** — inheritance runs during style computation phase only
### Implementation Pattern (Follow Exactly)
1. **CssValue** in `css/types.rs` — add `Unset` variant
2. **Parser** in `css/parser/mod.rs` — add `"unset"` keyword recognition
3. **Remove workarounds** in `css/parser/shorthands/` — delete incorrect `"unset" => Inherit` mappings
4. **is_inherited()** in `css/types.rs` — add `TextIndent`, audit full list
5. **inherit_from()** in `style/types/computed.rs` — add `text_indent`, keep in sync with `is_inherited()`
6. **Cascade engine** in `style/context.rs` — add `CssValue::Unset` handling (check `is_inherited()`, delegate to inherit or initial)
7. **Verify** `copy_property_from_parent()` and `reset_to_initial()` cover all properties
8. **Golden tests** in `tests/goldens/` — inherit/initial/unset fixtures
9. **Checklist update**`docs/CSS2.1_Implementation_Checklist.md`
10. **CI validation**`just ci 2>&1 | tee /tmp/ci-output.txt`
### `unset` Implementation Strategy
The `unset` keyword semantics (CSS Cascading §3.6):
- If the property is an **inherited property** → behave like `inherit` (copy from parent)
- If the property is a **non-inherited property** → behave like `initial` (reset to initial value)
Implementation in `context.rs` cascade handling:
```rust
if matches!(m.declaration.value, CssValue::Unset) {
if m.declaration.property.is_inherited() {
// Behave like inherit
if let Some(parent_id) = doc.parent(node_id) {
if let Some(parent) = styles.get(&parent_id) {
computed.copy_property_from_parent(&m.declaration.property, parent);
}
}
} else {
// Behave like initial
computed.reset_to_initial(&m.declaration.property);
}
continue;
}
```
### CSS 2.1 Inherited Properties Reference
Complete list of CSS 2.1 **visual** inherited properties (excluding aural/speech):
| Property | In is_inherited()? | In inherit_from()? | In ComputedStyles? |
|---|---|---|---|
| `color` | ✅ | ✅ | ✅ |
| `font-family` | ✅ | ✅ | ✅ |
| `font-size` | ✅ | ✅ | ✅ |
| `font-style` | ✅ | ✅ | ✅ |
| `font-weight` | ✅ | ✅ | ✅ |
| `font-variant` | ❌ | ❌ | ❌ (Story 1-10 scope) |
| `letter-spacing` | ✅ | ✅ | ✅ |
| `line-height` | ✅ | ✅ | ✅ |
| `list-style-type` | ✅ | ✅ | ✅ |
| `list-style-position` | ❌ | ❌ | ❌ |
| `list-style-image` | ❌ | ❌ | ❌ |
| `text-align` | ✅ | ✅ | ✅ |
| `text-indent` | ❌ MISSING | ❌ MISSING | ✅ |
| `text-transform` | ✅ | ✅ | ✅ |
| `visibility` | ✅ | ✅ | ✅ |
| `white-space` | ✅ | ✅ | ✅ |
| `word-spacing` | ❌ | ❌ | ❌ |
| `border-collapse` | ✅ | ✅ | ✅ |
| `border-spacing` | ✅ | ✅ | ✅ |
| `caption-side` | ❌ | ❌ | ❌ |
| `empty-cells` | ❌ | ❌ | ❌ |
| `cursor` | ❌ | ❌ | ❌ |
| `direction` | ❌ | ❌ | ❌ |
| `quotes` | ❌ | ❌ | ❌ |
| `orphans` | ❌ | ❌ | ❌ |
| `widows` | ❌ | ❌ | ❌ |
**Action for this story:**
- Fix `text-indent` (exists in ComputedStyles but missing from inheritance lists)
- For properties not in ComputedStyles at all: document as future work, don't add empty fields
- `font-variant` is being added in Story 1-10 — if already merged, add to inheritance lists
### Previous Story Learnings
From stories 1-11 (Overflow) and 1-12 (Media Rules):
- Type changes in `crates/css/src/types.rs` must not break any existing match statements — adding an enum variant requires updating all `match` expressions
- Golden test fixtures: `tests/goldens/fixtures/NNN-descriptive-name.html` (sequential numbering)
- Regen goldens: `cargo test -p rust_browser --test regen_goldens -- --nocapture`
- `just ci` must pass clean at the end
### Git Intelligence
Recent relevant commits:
- `7c8d2b8` — :link/:visited pseudo-class (style computation pattern)
- Previous milestone work established the inheritance infrastructure
### Testing Strategy
1. **Unit tests** for `CssValue::Unset` parsing (CSS parser tests)
2. **Unit tests** for `unset` on inherited property → inherits from parent
3. **Unit tests** for `unset` on non-inherited property → resets to initial
4. **Unit tests** for `text-indent` auto-inheritance
5. **Unit tests** for `inherit` on non-inherited properties (e.g., `display: inherit`)
6. **Unit tests** for `initial` on inherited properties (e.g., `color: initial`)
7. **Golden tests** — minimum 4 fixtures:
- `inherit` keyword on inherited + non-inherited properties
- `initial` keyword overriding inherited value
- `unset` on inherited property (acts as inherit)
- `unset` on non-inherited property (acts as initial)
8. **Regression** — all existing style computation tests pass unchanged
### Scope Boundaries
**In scope:**
- `CssValue::Unset` variant + proper handling
- Fix `text-indent` auto-inheritance
- Audit `is_inherited()` / `inherit_from()` for existing properties
- Verify `copy_property_from_parent()` / `reset_to_initial()` completeness
- Golden tests, checklist updates
**Out of scope (future stories):**
- `revert` keyword (CSS Cascading Level 4 — not CSS 2.1)
- `all` shorthand property (CSS3 — not CSS 2.1)
- Adding new properties to ComputedStyles (e.g., `word-spacing`, `cursor`, `direction`)
- User stylesheet origin
### Project Structure Notes
- All changes span Layer 1 crates only — `css` and `style`
- No new external dependencies needed
- Adding `CssValue::Unset` will require updating all `match` arms on `CssValue` — search for existing matches and add the new variant
### References
- [Source: CSS 2.1 §6.2 — Value processing (specified/computed/used/actual)]
- [Source: CSS 2.1 §6.2.1 — Specified values and inheritance]
- [Source: CSS 2.1 §6.2.4 — inherit keyword]
- [Source: CSS Cascading Level 3 §3.5-§3.6 — initial and unset keywords]
- [Source: crates/css/src/types.rs — CssValue enum (~line 199), is_inherited() (~line 778)]
- [Source: crates/css/src/parser/mod.rs — keyword parsing (~line 1161, 1296)]
- [Source: crates/style/src/context.rs — cascade inherit handling (~line 587)]
- [Source: crates/style/src/types/computed.rs — apply_declaration (~line 500), reset_to_initial (~line 1512), copy_property_from_parent (~line 1636), inherit_from (~line 1864)]
- [Source: crates/css/src/parser/shorthands/background.rs — incorrect unset workaround (~line 47)]
- [Source: docs/CSS2.1_Implementation_Checklist.md — inheritance status (~line 65)]
- [Source: _bmad-output/project-context.md — Full project rules and patterns]
## Dev Agent Record
### Agent Model Used
Claude Opus 4.6 (1M context)
### Debug Log References
None — implementation was straightforward.
### Completion Notes List
- **Task 1 (CssValue::Unset):** Added `Unset` variant to `CssValue` enum, parser recognition in `values.rs`, `property_dispatch.rs`, and `content.rs`. Removed incorrect `"unset" => CssValue::Inherit` workaround in `background.rs`. Added `Unset` handling in cascade engine (`context.rs`) using `is_inherited()` to decide between inherit/initial semantics. Added early return in `apply_declaration()`.
- **Tasks 2-3 (text-indent & property audit):** Already complete in the codebase — `text-indent`, `word-spacing`, `direction`, `font-variant`, `list-style-position`, `list-style-image`, `caption-side`, `empty-cells` all present in `is_inherited()` and `inherit_from()`. No changes needed.
- **Task 4 (inherit keyword):** Verified `copy_property_from_parent()` covers all PropertyId variants. No gaps found.
- **Task 5 (initial keyword):** Verified `reset_to_initial()` covers all PropertyId variants. Made method `pub(crate)` for use by `unset` handling in `context.rs`.
- **Task 6 (golden tests & docs):** Created 5 golden test fixtures (270-274) covering inherit/initial/unset/auto-inheritance. Updated CSS2.1 Implementation Checklist. `just ci` passes.
### Change Log
- 2026-03-14: Implemented CSS inheritance completeness — added `CssValue::Unset`, verified inherit/initial/auto-inheritance coverage, 12 unit tests + 5 golden tests added.
- 2026-03-14: Code review fixes — added `CssValue::Unset` handling to both pseudo-element code paths in `context.rs`; fixed `font: unset`, `list-style: unset`, and `text-decoration: unset` shorthand expansion; promoted WPT test `wpt-css-css2-normal-flow-inlines-002` (now passes due to `font: inherit` fix); added 5 shorthand expansion tests.
### File List
- `crates/css/src/types.rs` — Added `CssValue::Unset` variant
- `crates/css/src/parser/values.rs` — Added `"unset"` keyword recognition in all value parsers
- `crates/css/src/parser/property_dispatch.rs` — Added `"unset"` keyword recognition in 3 parse functions
- `crates/css/src/parser/content.rs` — Added `"unset"` to CSS-wide keyword check
- `crates/css/src/parser/shorthands/background.rs` — Fixed `"unset"` to use `CssValue::Unset` (was incorrectly mapped to `CssValue::Inherit`)
- `crates/style/src/context.rs` — Added `CssValue::Unset` cascade handling (inherit for inherited props, initial for non-inherited)
- `crates/style/src/types/computed.rs` — Added `CssValue::Unset` early return in `apply_declaration()`, made `reset_to_initial` pub(crate)
- `crates/style/src/tests/mod.rs` — Added `inheritance` test module
- `crates/style/src/tests/inheritance.rs` — New: 12 unit tests for unset/inherit/initial/auto-inheritance
- `tests/goldens/fixtures/270-inherit-inherited-property.html` — New golden fixture
- `tests/goldens/fixtures/271-inherit-non-inherited-property.html` — New golden fixture
- `tests/goldens/fixtures/272-initial-overrides-inherited.html` — New golden fixture
- `tests/goldens/fixtures/273-unset-inherited-vs-non-inherited.html` — New golden fixture
- `tests/goldens/fixtures/274-auto-inheritance-text-properties.html` — New golden fixture
- `tests/goldens/expected/270-*.txt` — Generated expected outputs (layout + display list)
- `tests/goldens/expected/271-*.txt` — Generated expected outputs
- `tests/goldens/expected/272-*.txt` — Generated expected outputs
- `tests/goldens/expected/273-*.txt` — Generated expected outputs
- `tests/goldens/expected/274-*.txt` — Generated expected outputs
- `docs/CSS2.1_Implementation_Checklist.md` — Updated inheritance items to complete
- `crates/css/src/parser/shorthands/typography.rs` — Fixed `font: unset`, `list-style: unset`, `text-decoration: unset` shorthand expansion
- `tests/external/wpt/wpt_manifest.toml` — Promoted `wpt-css-css2-normal-flow-inlines-002` from known_fail to pass
- `_bmad-output/implementation-artifacts/sprint-status.yaml` — Status: in-progress → done
## Senior Developer Review (AI)
**Review Date:** 2026-03-14
**Reviewer:** Claude Opus 4.6 (code review workflow)
**Outcome:** Approve (after fixes)
### Findings Summary
| # | Severity | Description | Status |
|---|----------|-------------|--------|
| H1 | HIGH | `CssValue::Unset` not handled in pseudo-element styling (2 code paths) | Fixed |
| M1 | MEDIUM | `list-style: unset` shorthand not handled | Fixed |
| M2 | MEDIUM | `text-decoration: unset` shorthand silently drops | Fixed |
| M3 | MEDIUM | `font: unset` silently drops (also fixed `font: inherit`/`initial`) | Fixed |
| L1 | LOW | No test for `unset` through shorthand expansion | Fixed |
### Action Items
- [x] H1: Add `CssValue::Unset` handling to both `compute_pseudo_styles` code paths in `context.rs`
- [x] M1: Add CSS-wide keyword handling (`inherit`/`initial`/`unset`) to `list-style` shorthand
- [x] M2: Add CSS-wide keyword handling to `text-decoration` shorthand
- [x] M3: Expand `font: inherit/initial/unset` to all 6 sub-properties instead of returning empty
- [x] L1: Add 5 shorthand expansion tests for `unset`