Repository navigation
fixed crasher in 6.3 Embedded - #121
Merged
Merged
Conversation
Vithanco
added a commit
to Vithanco/elementary-ui
that referenced
this pull request
Aug 29, 2026
The revert to the mirrored shape checked this file out of the pre-merge commit along with the ones the design change actually touched, which undid the attribute storage rework from "maybe fixed embedded crasher" (elementary-swift#121). This PR never had a reason to change _ElementNode at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sliemeobn
pushed a commit
that referenced
this pull request
Sep 1, 2026
* Allow DOM modifiers on SVG content DOMEffectView was constrained to Wrapped: View, but nothing in its body uses anything HTML-specific — only Wrapped.Tag and the three _Mountable requirements. The modifier stack it drives attaches to DOM nodes, which are namespace-agnostic, so the constraint was stricter than the implementation. Relaxing it to Wrapped: MarkupContent & _Mountable and conditionally conforming lets the same wrapper carry a modifier over HTML and over SVG alike. Only SVG.svg conforms to View; everything nested inside it is SVGView, so before this no inner SVG element could take a handler at all. MarkupContent is stated once and unconditionally, because a type may conform to a protocol only once and both HTML and SVGContent inherit it. Tag is the wrapped tag, Body is Never, and the _render default on MarkupContent supplies _Renderable, so it costs nothing. The event modifiers are mirrored onto SVGView as a first slice. The duplicated declarations are forced by the return type: some View<Tag> and some SVGView<Tag> are different opaque types, so a single shared extension on MarkupContent & _Mountable cannot serve both without making DOMEffectView public API. Happy to go the other way if you would rather expose it. Tests cover a handler on a nested element, the no-argument form, stacked modifiers, that the wrapper leaves the wrapped element's ops untouched, and a recursive SVGView carrying one handler per element. Remaining if this direction is right: ~15 more modifiers across styles, focus, binding, lifecycle, onChange, keyed views, transitions and global events. * trim comments to the ones that earn their place The blocks above DOMEffectView and in SVGView+DOMEvents largely restated the signatures they sat on. What is left is what the code cannot say: that the loose Wrapped constraint is deliberate because DOM nodes are namespace-agnostic, that MarkupContent must be unconditional since a type may conform only once, and why the mirrored SVGView declarations cannot be shared. The two per-test doc comments repeated their own test names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Mirror the remaining DOM event and style modifiers onto SVGView Completes the two modifier families that `DOMEffectView` drives, so SVG content is no longer interactively inert: - events: onMouseDown, onMouseMove, onMouseUp, onKeyDown - styles: opacity, rotationEffect, offset, scaleEffect (x2), blur, saturation, brightness `onInput` is left out (input events do not fire on SVG elements), as is the deprecated `Float` overload of `offset`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * trim the new comments to the ones that earn their place Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * match the repo's comment style: no backticks in implementation comments Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * share the DOM event modifiers across HTML and SVG Return the concrete DOMEffectView instead of some View<Tag> / some SVGView<Tag>, so its conditional conformances pick the namespace and one declaration serves both. SVGView+DOMEvents.swift is no longer needed. Naming DOMEffectView in a public signature means DOMElementModifier has to be public, which forces its mount requirement public too - and that names DOM.Node, which is @_spi(Benchmarking). Marking just that one requirement @_spi(Benchmarking) keeps the two in the same visibility tier. Every conformer is in-module, so nothing outside can be affected. onInput stays on View: input events do not fire on SVG elements. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * share the style modifiers across HTML and SVG Same treatment as the event modifiers: return the concrete DOMEffectView so the conditional conformances pick the namespace, and drop SVGView+Styles.swift. View+Styles.swift becomes MarkupContent+Styles.swift; its documentation moves with it, reworded from "view" to "content" now that it covers both. The offset(x:Float,y:Float) overload moves too. Splitting it from the Double one would change overload resolution for integer literals on HTML. This costs more public surface than the events did: naming the style modifiers in a return type makes their Value typealiases public, which pulls in CSSOpacity, CSSTransform and CSSFilter along with their nested function and payload types. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * hide the concrete modifiers behind an opaque return type Naming the modifier in the return type was what forced the public surface open: a public DOMEffectView<OpacityModifier, Self> makes OpacityModifier public, which makes its Value typealias public, which pulls in CSSOpacity, CSSTransform, CSSFilter and every nested function and payload type. Returning DOMEffectView<some DOMElementModifier, Self> keeps the wrapper named, so the conditional conformances still pick the namespace, while the modifier itself stays hidden. Every concrete modifier, the CSS value vocabulary and DOMEventHandlers go back to internal: 108 new public declarations become 29, and the new public types are DOMEffectView, DOMElementModifier, DOMElementModifiers and AnyUnmountable. BindingModifier and FocusModifier had been given public witnesses they never needed - an internal conformer does not require them. some rather than any: any DOMElementModifier cannot satisfy the constraint at all, and Embedded Swift allows only class-bound existentials. Opaque types monomorphise, so embedded output is unchanged across the five size targets and memory is identical on the performance suite. Adds a test that only compiles if the modifier really is declared once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * underscore the types that are public only for the compiler None of these four are meant to be used: with the modifier opaque, _DOMEffectView cannot even be named outside the module, since its first generic argument is deliberately unspellable. The underscore matches _Mountable, _AttributeModifier and the rest of the reconciler surface, and keeps them out of generated docs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * drop accidental pnpm-lock churn Local pnpm runs rewrote libc fields in three lockfiles; unrelated to this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * go back to explicit per-namespace declarations Reverts the shared MarkupContent extensions and everything they required: the opaque return types, the public and underscored DOMEffectView surface, and the @_spi on the mount requirement. DOMEffectView is internal again, and View and SVGView each declare their own modifiers returning some View<Tag> and some SVGView<Tag>. A little duplication in exchange for no exposed internals and simpler signatures for package consumers. The cross-namespace test goes with it: with two declarations there is no longer a single one to pin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * leave the SVG transform modifiers out for now rotationEffect and scaleEffect do not work on SVG: transform-origin resolves against the viewBox rather than the element, so rotate(45deg) on a rect at (20,20)-(60,60) lands its centre at (0,56.6) instead of (40,40), and a topLeading anchor puts it off-canvas at (-100,98) - the percentage translate the anchor uses assumes an origin already at the element's centre. Both need transform-box: fill-box plus an explicit transform-origin, which is worth doing on its own rather than smuggling into this PR. offset stays: it translates by absolute pixels and is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * record why the duplication is a choice, not a limit The old note said one extension cannot serve both namespaces. It can - the PR history proves it - just not while returning some View<Tag> / some SVGView<Tag>, which is what keeps the wrapper internal. Saying "cannot" invites the next reader to rediscover that the hard way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * cut the comments that document the PR rather than the code The notes about rotationEffect, scaleEffect and the Float offset overload explained absences that only exist relative to this branch's history - on main those functions were never in the file. The note above DOMEffectView restated what the four conformances below it say precisely, and tightening the constraint back would fail to compile anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * answer the embedded TODO on _onEvent The note asked whether this was still an issue in 6.3. It is: making _onEvent internal fails the embedded build of Counter in _FunctionNode.swift with "cannot use metatype of type 'some View<HTMLTag.button>'" and "a protocol type cannot contain a generic method 'unmount'". Public builds clean on the same toolchain, so the trigger is the opaque return type reaching the function-node machinery, not the declaration. Recheck target moved to 7.0. The SVG copy keeps pointing at this note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * restore main's _ElementNode - it was reverted by mistake The revert to the mirrored shape checked this file out of the pre-merge commit along with the ones the design change actually touched, which undid the attribute storage rework from "maybe fixed embedded crasher" (#121). This PR never had a reason to change _ElementNode at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * cover the SVG modifier paths that only mounting exercised The suites only ever mounted, so _patchNode had no SVG coverage at all - half of DOMEffectView, and the half a reconciler change is most likely to break. Adds patching for opacity, offset and filters, patching an attribute through a wrapping modifier, growing a recursive SVGView, and unmounting a modified element. Three assertions matched any value for transform and filter, so a wrong CSS string would have passed; they now pin the exact string. saturation and brightness were untested, as was modifier layering - stacking two filters or two transforms composes through upstream/layerNumber and had no test in either namespace. Also covers styling an SVG container rather than a leaf. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test the animation behaviour the docs promise Every SVG style modifier's doc comment ends with "Changes to X are automatically animated when done in an animated transaction", which nothing checked. It holds: opacity and transform changes inside withAnimation start an animation, and a plain change outside one emits a setStyle op instead. The suite has to be serialized for that, the way TransitionMountRootTests already is - withAnimation leaks into tests running in parallel, which made three unrelated patch tests see their ops swallowed by an animation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * back out the animation tests - they make the suite flaky Transaction._current is a process-global, not task-local, which is fine for the single-threaded browser runtime but not for swift-testing's parallel suites: a withAnimation in one test leaks into whatever runs beside it, and tests asserting setStyle ops see them swallowed by an animation instead. @suite(.serialized) is not enough - it only orders tests within a suite, and the leak crosses suites. Measured: 2 failures in 5 runs with these tests, 0 in 6 without, and the failures move around. The documented "automatically animated in an animated transaction" behaviour is still worth covering; it needs the leak fixed first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix the formatting left behind by removing the transform modifiers swift format lint is part of CI and this failed it: two runs of stray blank lines with trailing whitespace, where rotationEffect and scaleEffect used to be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
fixes #120
there are still a few weird issues with Embedded on Swift 6.3 going on, but this seems to address a few.
6.4 appears to be a lot more robust