Repository navigation
macos: Add configurable materials and glass tint compositing - #1008
Conversation
huacnlee
left a comment
There was a problem hiding this comment.
This PR completes macOS material composition: it applies the existing blend modes, adds native material configuration and live glass tint, and offers a transparent window for custom Rust hosts. Keeping AppKit roles in a macOS-only prop group and product colors outside NSH is reasonable; this is a new public API, so the host integration scope should be explicit before accepting it.
Two changes are needed:
- Keep the shipped skill guide synchronized (
website/docs/coding-guides.md:988,skills/nsh/references/coding-guides.md:988). Addingmacos={{…}}only to the website guide breakswebsite/tests/skills.test.ts:13, which requires verbatim copies. I reproducedbun test website/tests/skills.test.ts: 4 pass, 1 fail. The copies are equal at this commit's parent. Copy the updated guide into the bundled skill; this explains the Website build failure. - Make the new public configuration extensible (
crates/nsh-core/src/effects.rs:607).MacOsBackdropderivesDefaultbut is missing#[non_exhaustive], required byAGENTS.md's Public Rust structs rule. Add it now so adding material options later does not break exhaustive downstream literals. The external literals incrates/nsh-engine/tests/engine/platform.rs:194andcrates/nsh-platform-macos/src/visual.rs:1327must then initialize fromMacOsBackdrop::default()and assign fields; documentation should demonstrate that construction pattern too.
API scope to resolve/document: AppOptions::transparent can only be set by a custom Rust host calling run_app/constructing AppView. The normal nsh-macos-host CLI and nsh-macos-app::setup leave it false, including generated Rust runners and packaged TSX apps. Thus blending: "behind-window" cannot produce a desktop-transparent standard packaged app through the new TSX API alone. This may be intentional for embedding, but should not be presented as general packaged-app support.
Validation on Linux at 99aa22d8cb5aa9a78079f9959f7f41c80925f3eb:
cargo test -p nsh-core --lib: 1,133 passed, 9 ignored.cargo test -p nsh-js --lib: 198 passed, 1 ignored.cargo test -p nsh-engine --test engine macos_props_apply_on_macos_through_the_mount: passed (including the non-macOS control).cargo clippy -p nsh-core -p nsh-js -p nsh-engine --all-targets -- -D warnings: passed.cargo +nightly fmt --all --check: fails only in seven files unchanged by this PR; no formatting failure was reported in its changed files.- Website guide parity fails as above. Required Compat also fails at the new material case (
no prop macos.backdrop); the PR declares paired checker dependency longbridge/nsh-compat#37. The failing Web Shift+F10 case is the separate existing issue tracked by the fleet, not a demonstrated material-composition regression.
I could not independently verify AppKit rendering on this Linux host. Before merging, a macOS run should capture WindowServer main/PR comparisons (the in-process screenshot API omits compositor effects), light/dark and active/inactive materials, live tint/change/clear, effect-kind transitions, pointer passthrough, transparent custom host, and unchanged embedded host window configuration. On macOS before 26, verify glass fallback without referencing the absent class. No native visual result is claimed here.
|
Mac verification follow-up at WindowServer main/PR composition, blend/reset, material activity, live tint/clear, effect transitions, pointer passthrough, transparent standalone hosts and embedded-window preservation remains unverified. No compositor screenshot, native application test or live settings-change result is claimed. Independent detached worktrees are prepared for resuming in an accessible desktop session; no application/server was started and no user settings were changed. The previous review findings remain unresolved. |
e448d45 to
335ee39
Compare
|
Addressed the review. Current head:
Post-rebase local validation: core 1143 passed / 9 existing ignored; menu suite 16 passed; macOS backend 87 passed; original headless context-menu file all 3 passed; real Chromium keyboard and ordinary-click cases both passed; core/base/engine/macOS all-target Clippy The browser host drives a desktop mouse, so the newly added Chrome CI commands select the keyboard and ordinary-click cases. The held-finger test remains unchanged in the full headless run; mouse long press is not substituted for touch input. Extra browser-module checks report 88 pass / 2 skips / 1 existing bidi-link fixture failure, also reproduced on the fixed baseline; no failure was suppressed. Native evidence and reproduction sources: https://github.com/honsunrise/nsh/tree/0ba1ca241385264382b1970f496e6358ab01e1a7/verification/pr1008 Limits: no execution on macOS before 26, and no real active/key-window transition could be obtained in this session. The evidence records these as unverified, not passed. Paired longbridge/nsh-compat#37 is also rebased and tested locally. Its upstream CI still needs maintainer approval (the approval endpoint returned 403 for my read-only upstream account). Required gates have not been bypassed; neither PR has been merged. The framework Actions run is being followed separately, and local results are not presented as remote CI success. Remote CI follow-up: watched the final head for one hour. Website build, CI paths, and Rust application (windows-latest) passed; Website deploy was skipped. The other nine checks remained queued. The Core job API reports |
335ee39 to
de735c1
Compare
|
API-focused review at de735c1 (base 0d0338a). The purpose is sound: expose native material composition that application markup cannot implement, apply the already-existing blend API, and allow a custom Rust host to opt into a transparent window. I would not accept the public API unchanged. The useful surface is three capabilities, not dozens of independent features; typed AppKit roles are appropriate and should not be replaced with an untyped string bag. Standards
Design judgment: keep glassTintColor alongside the existing glassEffect style, but implement its tint/reset contract on iOS 26 too before treating it as a coherent Apple glass capability. UIKit already exposes optional UIGlassEffect.tintColor: https://developer.apple.com/documentation/uikit/uiglasseffect/tintcolor . The PR explicitly documents the iOS no-op (style.rs:1597), so this is a completeness/design concern, not an undisclosed runtime regression. If this PR must remain macOS-only, defer the shared tint addition for a focused follow-up rather than create another parallel tint vocabulary. SpecThe live-tint-clear requirement has the Rust ergonomics gap above. Static inspection found no confirmed defect in material > glass > blur precedence, same-kind updates, native effect removal, or legacy fallback preservation. AppOptions::transparent is now correctly documented as custom-Rust-host-only; it does not make normal packaged TSX apps transparent. The browser context-menu keyboard change (web/nsh.js:6687 and rust-app.yml:205) solves a separate baseline problem, as the author's comment explains. Move it out of the material PR; it is not needed to define these APIs. Reference assessment
Validation and limitsRead the full API diff, core/style/host decoder, macro generation and macOS effect lifecycle, existing reviews and author response. Independently ran bun test website/tests/skills.test.ts: 5 pass. The previous missing non_exhaustive and guide-sync findings are fixed. No Rust suite rerun or native visual execution is claimed in this API-focused review; the existing T-O0005 native follow-up remains separate. Remote Core, Linux, Windows, application and Website checks are green, but required Compat is FAILED: run 42153881052, checkout_apps_run step 157 reports no prop macos.backdrop. Its paired checker dependency #37 remains relevant. CI is not all green; this is not a merge recommendation. Standards: 3 concrete API findings plus an iOS completeness judgment; most significant is the public reset contract. Spec: 2 concerns (shared tint-reset concern and unrelated browser scope); no confirmed backend behavior defect from static review. |
de735c1 to
18d2a3c
Compare
huacnlee
left a comment
There was a problem hiding this comment.
Incremental review at 18d2a3c (previous API audit at de735c1). The branch was rebased onto main 084926b; per git range-diff, the three PR commits differ from de735c1 only by rebase context (AppOptions::notifications, Application.quit() docs). No code from the API audit has changed yet.
Resolved earlier (still holds)
MacOsBackdropis#[non_exhaustive]and built viaDefault+ field assignment (crates/nsh-core/src/effects.rs:606-610).- Custom-Rust-host-only transparency is documented (
crates/nsh-platform-macos/src/app.rs:207-210,docs/dev/macos.md).
Still open from the API audit (P2, design)
- Redundant public converters:
BackdropMaterial::name/parse(crates/nsh-core/src/effects.rs:571,582) andMacOsBackdropMaterial/Blending/Statename()/parse()(effects.rs:675,695,731…) duplicate the mappingPropEnumalready generates. Drop them and decode throughfield::<Enum, _>inhost_values.rs. - Native prop values use kebab case (
#[prop(rename = "hud-window")]effects.rs:654,"behind-window"effects.rs:725,follows-window-active-state).docs/dev/api-naming.mdand the neighbouring iOSultraThinuse camelCase:hudWindow,behindWindow,followsWindowActiveState. AppKit (NSVisualEffectView.Material.hudWindow) and SwiftUI also spell them this way; kebab case belongs to CSS keywords such asmixBlendMode. - Rust/TS parity for tint reset:
glass_tint_color(impl IntoReactive<AdaptiveColor>)(crates/nsh-core/src/element/style.rs:2930) cannot bind color -> native default -> color the way TSnullcan. Accept an optional reactive color, like SwiftUIGlass.tint(_: Color?)and UIKitUIGlassEffect.tintColor, and add a reset test. - Scope: the iOS 26 glass tint is still documented as a no-op. Either implement it via
UIGlassEffect.tintColor, or moveglassTintColorinto a follow-up so one Apple glass vocabulary covers both platforms. - Commit
18d2a3c(Chromium context-menu keys,crates/nsh-platform-web/web/nsh.js:6536-6690,.github/workflows/rust-app.yml:204-210) is unrelated to materials. Please split it into its own PR. Main has had its own Shift+F10 / held-finger menu fixes (#1011, #1033) since then, so it needs separate review anyway.
API verdict: keep the shape (one typed macos.backdrop object mirroring NSVisualEffectView's material/blendingMode/state, plus AppOptions::transparent for custom hosts). Before accepting it, fix items 1–3 and decide on item 4. Fallbacks are fine: non-macOS platforms ignore macos.backdrop, and CSS backdropFilter is unchanged.
Ran here (Linux): cargo clippy -p nsh-core -p nsh-engine --all-targets -D warnings is clean at 18d2a3c. I could not run the AppKit runtime or the compositor on this host. CI for this head is still pending.
18d2a3c to
481f4a6
Compare
|
Addressed the API review in
One scope correction to the incremental review: the older iOS Reproduction sources, WindowServer captures and source/PID-bound native observations: https://github.com/honsunrise/nsh/tree/bc4e1f6037e6e37b8d20df619f041024d265e5c8/verification/pr1008 Final-head isolated verification is complete: core 1154 passed / 9 existing ignored; engine platform selection 23 passed; macOS backend 87 passed; UIKit lifecycle selection 2 passed; core/engine strict Clippy passed; paired checker all 32 passed, including checkout applications; bundled guides 5 passed; changed Rust formatting clean. The fixture runs and captures are tied to this final head. Remote gate: all 12 framework checks remain queued. The Core job API reports an empty runner name and no steps, so there is no execution failure log to fix. Paired checker workflow 42196961620 requires maintainer approval. Local verification does not override these gates. The local watcher was stopped; no remote Actions run was cancelled. Limits: Catalyst's existing standalone |
|
Closing out the independent macOS verification follow-up at observed head Still not independently verified: WindowServer baseline/PR composition, blend/reset, material active/inactive transitions, glass tint update/clear, effect switching, pointer passthrough, transparent custom-host behavior and preservation of embedded-window configuration. The last execution-session probe found zero accessible active displays and no screen-capture access. No reviewer compositor media was produced. The author's latest native evidence and API-fix response are recorded above; they are not independent execution by this reviewer. This follow-up is complete with those limitations, rather than awaiting another desktop retry. No application/service was left running, user preference changed, or contributor branch pushed or merged by this task. |
481f4a6 to
8ddbc7e
Compare
|
Rebased onto the fetched The rebase exposed an interaction with the new upstream window API: a custom transparent host could switch its window opaque but keep a clear background (alpha 0), even after redraw. Fresh local checks: core 1207 passed / 9 existing ignored; engine platform 32 passed; macOS backend 100 passed; core/engine/macOS all-target Clippy The old Website failure was the 4,102,471-byte Brotli output against a 4,100,000-byte limit. Upstream now makes that metric reporting-only with a 5,000,000-byte budget; this PR does not change the budget. The old macOS lint was fixed upstream by #1112, and the new macOS application CI check has passed. Other new checks are still running/queued; no all-green claim is made. The maintainer's independent native-verification closure remains recorded with its limitations. This author-side rebase smoke is not represented as independent reviewer execution. No PR was merged by this update. |
huacnlee
left a comment
There was a problem hiding this comment.
Incremental review of 8ddbc7e against the previously reviewed 18d2a3c (git range-diff: two reviewed commits rebased, the unrelated web context-menu commit dropped, new 7873dd3 and 8ddbc7e).
All points from the earlier API audit are addressed:
- The redundant public
name()/parse()onMacOsBackdropMaterial/Blending/Stateare removed. Decoding now goes through the sharedPropEnumfield::<…>path. - Native values are camelCase (
hudWindow,behindWindow,followsWindowActiveState), matching AppKit names and the existing iOSbackdropMaterialvalues (ultraThin). The compat case and script are updated. - Rust
glass_tint_colortakesimpl IntoReactive<Option<AdaptiveColor>>.Noneresets to the system tint, and static colors still pass directly. TSglassTintColor: nullbehaves the same way, so Rust and TS now match. - iOS 26
UIGlassEffectnow applies the tint. A tint change updates the existing effect view instead of rebuilding it, and transparent counts as a real tint, not a reset. - The web context-menu change is no longer in this PR (it is #1098).
8ddbc7emakesAppOptions::transparentonly an initial default: explicit window attributes andWindowHandle::setwin, and abehindWindowmaterial never makes the window transparent. That precedence is right.
Public API verdict. The current surface is macos={{ backdrop: { material, blending, state } }}, glassTintColor / glass_tint_color(Option<…>) and AppOptions::transparent for custom hosts. Each name is self-explanatory, follows AppKit/UIKit vocabulary, has Rust/TS parity and a reset path. No further API changes requested. Accepting a new macOS-only prop group is still the maintainer's call.
Ran locally (Linux):
cargo test -p nsh-corefiltered to tint/backdrop/material/glass andjs::tests: all passcargo test -p nsh-engine --test engine platform: 32 passedcargo clippy -p nsh-core -p nsh-engine --all-targets -- -D warnings: clean;cargo fmt --check: clean
CI: not green yet.
gpui-galleryfails only onform.test.tsx › Shift with a key…, the same failure as main's latest Linux run (42347811185), so this is main drift.compatfails incheckout_apps_runat the desktop case's first step (tap id:case-window-open: nothing to press). This diff doesn't touch that case, and other branches on the same main base pass compat, so it looks like a flake. It is red only on this branch, though: please re-run it, and confirm it passes before merging.checkandwebare pending.
Not verified here: WindowServer and iOS 26 rendering of the material, the tint and transparency (no macOS/iOS host; see the earlier close-out comment for the remaining native gaps).
8ddbc7e to
47e7de8
Compare
|
CI follow-up: head @huacnlee Compat's independent root cause is fixed in longbridge/nsh-compat#40: Final CI status on |
## Scope Extracted from #1008 as requested in the material API review. This PR only handles desktop browser context-menu keyboard dispatch and its CI coverage; it contains no material/tint APIs. ## Changes - Handle Shift+F10 and the Menu key after app shortcuts and editable-field handling. - Mark keyboard requests as pointer-free, preserving the existing Chromium `button === -1` classification. - Prevent the native default only when a custom context menu handled the request, avoiding duplicate openings. - Add real-Chrome keyboard and ordinary-click coverage. The full existing headless file still runs its held-finger case; the Chrome host drives a desktop mouse, not touch. ## Earlier verification at 4414d25 - Original headless context-menu file: 3 passed. - Real Chromium/macOS Shift+F10: passed (the unmodified browser implementation failed to open the menu). - Browser input/runtime module tests: 66 passed, no skips. - Branch is based directly on current main, with one commit and no dependency on #1008. ## Rebase refresh Head `bd30b584cd58a30157da194d1d8fc89e001054db` is rebased onto `origin/main` at `beeb9fc7707e0d4a195a4f7274f77b30e134c500`. `git range-diff` reports the same patch as approved `4414d25f`. After rebase: JS syntax check and both mouse-module tests passed; the original headless file passed all 3 cases; real Chromium Shift+F10 and ordinary-click cases both passed. Windows Menu-key key-up behavior was not exercised locally.
…1227) ## Description `crates/nsh-platform-web/web/browser/bootstrap.js` is generated, and the Website workflow checks it is current (`cargo run -p nsh-bundler --example browser_bootstrap -- --check`). #1008 (the `macos` platform prop group) and #1216 (`setTheme`) changed its sources without regenerating it, so that check now fails on `main` and on every pull request that runs Website (e.g. #1121). This commit is the generator's output, unchanged: `macos` joins the browser's `PLATFORM_PROPS`, and the theme gains `setTheme` (op 406), as the QuickJS runtime already has them. ## Public API None: the browser runtime catches up with what `main` already declares. ## How to Test - `cargo run -p nsh-bundler --example browser_bootstrap -- --check` passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Scope
Expose native material composition without prescribing an application theme:
mixBlendModeAPI on macOS, including text, images and clipped children;normalremoves the filter.macos.backdropconfiguration for AppKit material, blending and activity state.glassTintColorsupport tint updates and native-default restoration on both iOS 26+ and macOS 26+.run_apphost to opt into a transparent window. Standard hosts default to opaque windows but can use upstream window attributes to request transparency; an embeddedAppViewnever changes its host window.The unrelated browser keyboard patch is now a separate PR: #1098. This branch contains no context-menu keyboard or CI-selection changes.
API
MacOsBackdropis#[non_exhaustive]: construct it withDefaultand assign fields. Its existing fallback defaults remainPopover,WithinWindow, andActive.The three new native enums use
PropEnum's generatedjs_nameandHostValue::decodepaths, with camelCase values. The redundant handwrittenname/parsemethods and kebab-case overrides introduced by this PR are removed. Existing CSSmixBlendModevalues are unchanged.Styled::glass_tint_coloracceptsIntoReactive<Option<AdaptiveColor>>. StaticAdaptiveColor/Hsla/Rgba/Colorvalues retain direct-call ergonomics; optional signals and closures can set and clear the tint..glass_tint_color(None)also directly restores the default. TSnulland removing the style property have the same reset meaning. Transparent color remains an explicit tint.Native behavior
Explicit macOS material takes precedence over glass, then the existing blur fallback. Effect-kind changes remove the old native view. Tint-only changes retain the glass view, and material views remain noninteractive.
UIKit uses runtime
UIGlassEffectlookup, updatestintColoron its effect and assigns the effect back to the sameUIVisualEffectView. Blur effects are not sent glass-only selectors. Earlier-system class-unavailable fallback is retained.AppOptions::transparentdefaults to false and now feeds the upstream main-window attributes as an initial default. Explicit launch attributes and later window updates take precedence; the standalone surface no longer keeps a competing transparency flag. No CLI flags,nsh.jsonkeys, per-material utility classes, application color recipes or terminal behavior are added.Verification scope
Earlier Apple tint/reset verification at
481f4a6ewas exercised through real AppKit mouse events and UIKit accessibility activation in a publicAppViewembedding host on Mac Catalyst. Light/dark native appearance, red/blue, explicit transparent color, native-default restoration and stable glass-view identity passed. UIKit blur also ignored tint updates without replacing its view. WindowServer captures, not in-process screenshots, record the material rendering.Earlier validation at
481f4a6eused an isolated build directory (not a rerun of the current rebase):-D warningsPrevious rebase verification at 8ddbc7e
Both open PRs are rebased onto
origin/mainatbeeb9fc7707e0d4a195a4f7274f77b30e134c500, without merge commits.The new upstream window APIs exposed a native interaction regression: after starting with
AppOptions::transparent=true, setting the main window opaque left its background alpha at 0, including after a redraw. The integration fix uses the upstream live window state; the same real AppKit scenario now restores alpha 1, survives redraw, and can switch back to alpha 0. Explicit opaque launch attributes win over the host default, and normal hosts remain opaque by default. English/Chinese docs and screenshot-fallback descriptions follow the new upstream contracts.Fresh local verification after rebase: core 1207 passed / 9 existing ignored; engine platform selection 32 passed / 322 filtered; macOS backend 100 passed; core/engine/macOS all-target strict Clippy passed; bundled guides 5 passed; CLI built; upstream checker all 32 passed. Changed Rust formatting passed. The native window scenario verified both runtime toggling and explicit/default launch precedence. Earlier Website CI exceeded the then-hard Brotli budget by 2,471 bytes. Current upstream has already changed that check to a reporting-only 5,000,000-byte budget; this PR does not modify the budget. The old macOS lint failure was fixed upstream in #1112.
The maintainer closed the independent native-verification follow-up with its environment limitations, not a pass. Earlier author-side native captures remain historical evidence; no new independent verification or older-OS coverage is claimed here. Current local runtime verification targets the rebased window-state integration.
History and merge order
Framework head:
47e7de842b5a585a194a519067ddb74d9ec3b892, linearly rebased onto4ca2672d30659a63b6358f2942383c6248b2772f; no merge commits.Companion longbridge/nsh-compat#37 is now merged. Validation uses the checker’s upstream
origin/main; no rewrite or push of the merged PR was performed.Native evidence and reproduction: https://github.com/honsunrise/nsh/tree/bc4e1f6037e6e37b8d20df619f041024d265e5c8/verification/pr1008
Latest CI root causes and correction
All checks on
8ddbc7e3finished: only Compat and GPUI Gallery failed. GPUI has since been fixed upstream in #1205; the current rebase includes it, with all four material commits unchanged byrange-diff. Fresh engine platform tests (32), macOS backend tests (100), and CLI build passed. The original shifted-key assertion also passed on the upstream GPUI implementation in a focused macOS run; a preceding full local run had three setup timeouts, so it is not claimed as 9/9.Compat is a checker race, not a material regression: the case and example both named
desktopshared output and staging directories. It can mix the example bundle with the case script or silently run the same app twice. Earlier green checkout runs do not establish that both desktop apps were covered. The root fix is longbridge/nsh-compat#40: distinct case/example identities and private staging, with no skipped scripts or assertions. All 33 tests passed locally against8ddbc7e3, and all 17 distinct app outputs were inspected, including both desktop IDs and scripts. That fix has passed its upstream CI and still needs maintainer review/merge; a lucky rerun with the old checker is not proof the race is gone.The duplicate GPUI fix #1211 was closed in favor of merged #1205. No unrelated GPUI patch was added to this PR. All executed checks on
47e7de84have now passed, including Linux GPUI Gallery and Web; deployment is skipped by the PR workflow. The green Compat job still checked out old checkerf1e4188, so it does not establish that the directory race is eliminated. Merge checker #40 and rerun Compat to verify the corrected harness.