Engine fixes for the weather cross-platform example - #60
Open
skyturkish wants to merge 4 commits into
Open
skyturkish wants to merge 4 commits into
skyturkish wants to merge 4 commits into
Conversation
- Tree::mount queues class styles for a root it mounts for the first time
(noteMountedRootStyle). Nothing in building the root recomputed its
styles, so `:root { --x: ... }` never applied and every var(--x) below
resolved to nothing: colours and paddings driven by custom properties
silently fell back to their defaults.
- A compiled length with var() inputs is no longer stored in the dynamic
length-expression cache. A node styled before it is parented resolved
the var to nothing, and that entry then answered every later
resolution, layout's included.
- pointer-events inherits, as in CSS: applyInheritedStyleDefaults copies
it, the parent snapshot and diff carry it so incremental recompute
reaches descendants, and it counts as a descendant-affecting property.
A `pointer-events: none` overlay now lets hits fall through its images.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Application::init publishes the viewport before the tree exists, clears the document, and only then runs the app's top level — so the style pass that actually matters is endStyleMountBatch, once the tree is built. That pass recomputes the nodes the mount marked pending, reduced to their top-most ancestors. The document root is never among them. Document creates it, not the app, so nothing marks it pending — and it is exactly what :root matches, which is where custom properties are declared. Descendants were therefore styled against a root that had no properties yet: every var() lookup missed, and a miss resolves to a hard 0, so `padding: var(--safe-top) var(--safe-x) var(--safe-bottom)` silently computed to 0 on all four sides. weather's content sat flush against both edges on every target. It looked like an iOS bug only because macOS and iOS call setViewportMetrics from their frame loops, which ends in recomputeAllClassStyles — and that walks from every parentless node, so it picks the root up and heals the miss. Android never calls it, and showed the bug plainly. Nothing was ever right on the first pass; two shells were just papering over it on the second. Pull the root in when it has never been styled, which is the first mount and nothing else: later incremental recomputes keep their narrow roots, so a theme switch does not become a whole-tree pass. Verified on iOS with the shells' second pass disabled, so the padding resolves on the mount pass alone: pad=(6,24,0,24) at dpr 1.44, i.e. the authored 4px and 17px. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A text run alone on its line box was widened to the whole content box so text-align had room to align inside it. `start` alignment does not use that room -- it draws at the left edge either way -- but the widened box becomes the run's layout.width, and that is what the parent shrink-wraps to. Any auto-width box holding one text run therefore claimed its container's full width instead of hugging its label. In the weather example that made each `<button>` in the forecast tab strip 358px wide inside a 358px row: the row measured 719px, `Days` was pushed off screen, and the sibling `Next hours` title was flex-shrunk to 1px. Restrict the widening to the alignments that consume the width (center / right). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The stylesheet compiler emits `font-weight` as a direct property rule, and makeDirectPropertyCssRule registered every such rule as Ignored. Its value is applied through the compiled value, so the declaration only decides which pass a rule belongs to -- and the font metrics are resolved first, by a pass that selects its rules by declaration (setsFontMetrics). Every later pass then drops font metrics as already settled, through setStyleValue's g_resolvedFontNode guard. A static `font-weight: 600` therefore reached no pass at all: every node kept 400, on every target, and the renderers painted weather's semibold labels in regular. font-size never hit this because it is emitted as a length rule, which carries its declaration. Give direct property rules the declaration of the font property they carry, so they join the font pass the way the length and family rules already do. A group rule joins it whole; its other properties are plain values, and the ordinary pass replays them in cascade order. Checked on the macOS and iOS simulator builds of weather: the chips, the topbar pills, the place name and the metric values now paint semibold, as in the browser. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks for the pull request. Before it can be merged, please read the GeaStack Contributor License Agreement and sign it by posting a comment here with exactly: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe changes update font-style rule resolution and dynamic length caching, add pointer-events to inherited style handling, recalculate styles when a mounted root changes, and restrict width expansion for sole text runs by alignment. ChangesStyle Resolution and Inheritance
Mounted-Root Style Recalculation
Text Layout
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The style and layout changes are mergeable after normal checks; no concrete unresolved issue is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The normal mount path validates the root and applies styles before presenting the styled view. No new security weakness is demonstrated, although unusual root switches and interrupted style batches remain less certain. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (1 skipped: 1 too large.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
This was referenced Sep 27, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Engine fixes found while taking the
weatherexample to Android, iOS, macOS and Windows. The renderer PRs depend on these, so this one should merge first.:rootcustom properties applied on the first mount. The document root was never styled by the mount pass, so everyvar()below it resolved to nothing and weather's.screenpadding came out as 0 on every target. Two complementary fixes:Tree::mountqueues the root for styling, and the mount batch pulls in a root that was never styled.var()lengths no longer cached as misses. A node styled before it was parented resolved the var to nothing, and that cache entry answered every later lookup.pointer-eventsinherits, as in CSS.font-weightrules take effect. They were registered with anIgnoreddeclaration, so the font pass skipped them and later passes dropped them: every node kept weight 400 on every target.Verified: weather built and compared against a Chrome rendering of the same CSS on macOS and the iOS simulator. The padding and the semibold labels now match.
Related PRs (merge core first):
🤖 Generated with Claude Code
Summary by CodeRabbit