Skip to content

Commit d0fa8fa

Browse files
author
DevBot
committed
fix(ui): remove openPropsTokenSheet from theme toggle adoptedStyles + finalize plan
Theme toggle: remove openPropsTokenSheet from static styles to work around Safari's adoptedStyleSheets bug. Token CSS is already injected as page-level <style> by vite.config.ts — custom properties cascade through shadow DOM naturally. The component retains only its own component-specific sheet. Plan update: add architecture review (4 rendering paths, VNode vs VDOM), redundancy report (double signal binding, signal→DOM duplication, adoptedSheet duplication), problems-encountered table, and long-term Safari theme solutions. Verification: - deno task test: 894 passed, 0 failed - e2e chromium: 7/7 theme, 25/25 all - e2e webkit: 6/7 theme (surface colors test — known Safari bug) - autoflow:push: 5/5 PASS
1 parent 2482699 commit d0fa8fa

2 files changed

Lines changed: 79 additions & 2 deletions

File tree

docs/release/v0.41.0-alpha1-plan.md

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,3 +215,76 @@ After the initial cleanup, 6 CI gates failed on the remote build:
215215
inside the panel div. `www/app/islands/open-search.tsx`.
216216
- [x] E2E verified across Chromium (25/25), Firefox (23/25), WebKit (23/28,
217217
2 CSS color assertion failures, 1 SPA locale switch timeout — all cosmetic).
218+
219+
### v0.41.0-alpha1 - Framework Architecture Review
220+
221+
Comprehensive review of the rendering pipeline and signal-to-DOM binding:
222+
223+
**Rendering paths — 4 exist, all necessary:**
224+
225+
| Path | Mechanism | Purpose |
226+
| ---------------------- | -------------------------------------------------------------------------- | ------------------------------------------- |
227+
| A. Fine-grained effect | `effect(() => el.setAttribute())` | Signal prop → DOM attr update, no re-render |
228+
| B. Signal → TextNode | `effect(() => textNode.textContent = sig.value)` | JSX signal children expression |
229+
| C. Full re-render | `render()``while(root.firstChild) root.removeChild()``renderToDom()` | Initial mount + `this.update()` |
230+
| D. DSD hydration | `querySelectorAll('[data-signal]')``effect()` | SSR → client signal binding |
231+
232+
**Framework confirms user understanding:** VNode is an intermediate representation
233+
(5 frozen fields: `tag`, `props`, `children`, `key`, `ref`), NOT a virtual DOM.
234+
No diff algorithm, no reconciliation, no keyed child matching. Signal changes drive
235+
DOM updates directly via `effect()` callbacks — no VNode involvement.
236+
237+
**Identified redundancies:**
238+
239+
- **Double signal binding**`class={this.#signal}` + `data-signal-attr` on the same
240+
element cause two subscription paths that collide: signal change triggers full
241+
`render()` which replaces the DOM element, while `data-signal-attr` tries to update
242+
the attribute in-place. Result: `onClick` handler lost on re-render.
243+
Fix in `open-search.tsx`: use static `class='overlay'` + only `data-signal-attr`.
244+
- **Signal→DOM binding logic duplicated** — Path A (`applyProps` + `effect` in
245+
`jsx-render-dom.ts`) and Path D (`hydrateSignals` in `open-element-hydration.ts`)
246+
both bind signals to DOM but from different starting points (VNode vs SSR markers).
247+
Could be unified by making Path D call `renderToDom` instead of manual `effect()`.
248+
- **`adoptedStyleSheets` for global tokens**`openPropsTokenSheet` and
249+
`daisyClassSheet` are adopted into every component's shadow root, but the same
250+
CSS is already injected as page-level `<style>` by `vite.config.ts`. The
251+
duplicate adoption causes Safari's adoptedStyleSheets bug (`:host([data-theme])`
252+
does not recompute when `data-theme` attribute changes).
253+
254+
### v0.41.0-alpha1 - Progress on AdoptedStyleSheets Safari Bug
255+
256+
Safari/WebKit bug: `adoptedStyleSheets` containing `:host([data-theme])` selectors
257+
do not recompute when the host element's `data-theme` attribute changes via JS.
258+
This affects themed colors (`--bg-canvas`, `--surface-*` etc.) inside shadow DOM.
259+
260+
Attempted workarounds (all failed for the surface colors test):
261+
262+
- JS style.setProperty() on documentElement — Safari still uses stale adoptedSheet values
263+
- Forcing style recalculation via offsetHeight read — no effect
264+
- Re-assigning `adoptedStyleSheets = [...sheets]` — does not trigger recompute
265+
266+
**Current fix applied:**
267+
268+
- [x] `open-theme-toggle`: removed `openPropsTokenSheet` from `static styles`.
269+
The token CSS custom properties are already injected as page-level `<style>`
270+
by `vite.config.ts` and cascade through shadow DOM naturally. The component
271+
only adopts its own component-specific sheet.
272+
273+
**Long-term solutions (tracked for future):**
274+
275+
| Solution | Effort | Effect |
276+
| ------------------------------------------------------------------- | -------------------- | --------------------------------------------------------------------------------- |
277+
| Remove token sheets from all component `static styles` | Medium | All components inherit theme tokens from page-level CSS, bypassing the Safari bug |
278+
| Switch affected components to `renderMode = 'light'` | Small per component | No adoptedStyleSheets at all, but loses style encapsulation |
279+
| Use CSS `light-dark()` function instead of `[data-theme]` selectors | Large (CSS refactor) | Modern standard, works in all browsers, eliminates the selector dependency |
280+
| Unify signal binding Paths A and D (make D call `renderToDom`) | Medium | Reduces code duplication, simplifies maintenance |
281+
282+
### v0.41.0-alpha1 - Problems Encountered and Resolved
283+
284+
| Problem | Root Cause | Resolution | Commit |
285+
| ----------------------------------------------------- | ------------------------------------------------------------------------------------- | ------------------------------------------------------------ | ---------- |
286+
| Search overlay stuck open on backdrop click | `class={#signal}` + `data-signal-attr` double binding → full re-render → onClick lost | Static `class='overlay'` + `data-signal-attr` only | `2482699b` |
287+
| Search input click closes overlay in Firefox | Shadow DOM event retargeting, `e.target !== e.currentTarget` | `composedPath()` traversal | `a4b69863` |
288+
| Client bundle contains `Deno.readDirSync` | `manifest.ts` top-level execution of `buildManifest()` | Proxy + lazy init | `0f7e44d6` |
289+
| CI 6 gates fail (version anchors, PWA, arch contract) | Rebase + v0.41.0 cleanup fallout | Updated version refs across 14 files, updated arch contracts | `5f169d76` |
290+
| `sanitize-html` + `flexsearch` missing from deno.json | Rebase introduced deps not declared | Added to imports maps | `92215208` |

packages/ui/src/open-theme-toggle.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020

2121
import { OpenElement } from '@openelement/element';
2222
import { StyleSheet, type StyleSheetLike } from '@openelement/core/style-sheet';
23-
import { openPropsTokenSheet } from './open-props-tokens.js';
2423
import { signal } from '@openelement/signal';
2524
export const tagName = 'open-theme-toggle';
2625

@@ -72,7 +71,12 @@ sheet.replaceSync(`
7271
`);
7372

7473
export class OpenThemeToggle extends OpenElement {
75-
static override styles = [openPropsTokenSheet, sheet];
74+
// ponytail: Safari does not recompute adoptedStyleSheets when
75+
// :host([data-theme]) changes. The token sheets (openPropsTokenSheet,
76+
// daisyClassSheet) are already injected as page-level <style> by
77+
// vite.config.ts — CSS custom properties cascade from :root naturally.
78+
// Only adopt the component-specific sheet.
79+
static override styles = [sheet];
7680
static override delegatesFocus = true;
7781
static override observedAttributes = ['theme'];
7882

0 commit comments

Comments
 (0)