Repository navigation
Allow DOM event and style modifiers on SVG content - #119
Conversation
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.
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>
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>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@Vithanco thanks for the PR! I had it on the list to eventually fix this - so thank you for starting the work. the only thing I am not sold on yet is weather the "full duplication" is really a good thing. I think there may be a cleaner option of using public return types (instead of then there may be a way to implement these extensions on MarkdownContent somehow only once. wdyt? |
|
Spiked it — works. Snag: One exception to "only once": |
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>
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>
|
I think this is the right direction, but the "public" blast radius is too big for my taste. I was hoping we can get some clever I am not 100% sure if this can be done with current Swift, but I have not found the time to explore a shape for this. Do you know where I am trying to go? |
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>
|
ModifiedContent<some Modifier, Self> is exactly it, and DOMEffectView already was that type. Only the return types changed:
Named wrapper, so the conditional conformances still pick the namespace. Opaque modifier, so no concrete modifier appears in a public signature — and an internal type needs no public witnesses, which is what keeps CSSOpacity/CSSTransform/CSSFilter, the payload structs and DOMEventHandlers internal. What do you think now? |
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>
|
Also underscored what stays public: |
|
I think this is approaching the right shape now, thanks. can you please rebase onto latest main so we can run CI on it? |
Local pnpm runs rewrote libc fields in three lockfiles; unrelated to this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
done |
|
@Vithanco thank you for your patience, and my apologies for being annoying - but after a bit of back an forth I think my preference is the original shape: explicit copies of the API, with "clean and simple" it feels the bit of duplication is something very easily dealt with nowadays - in contrast to the somewhat messy exposure of underscored types + more complicated type gymnastics for these extensions. so I think the price of a tiny bit more maintenance burden is justified by the added simplicity for package consumers / documentation readers. wdyt? do you have a preference? also, I vaguely remember that svg elements have a different geometry behavior with transformation (eg: not "center" as anchor by default) - I have not tested it, but maybe we run into trouble by omitting the anchor for ".center" in the transforms for SVG? |
|
To be frank, I don't mind too much. Both have advantages and disadvantages.
The impact for choosing one is little as the using code shouldn't change with either implementation. So, we can chose whatever you think is better. Chose your vice ;-)
/Klaus
--
Vithanco / Klaus Kneupner
See for yourself how you can combine your brain power with structured visuals to help you think: https://vithanco.com/newsletter.
The home of Structured Visual Thinking: www.vithanco.com <http://www.vithanco.com>.
27 Aug 2026, 13:15 by ***@***.***:
… sliemeobn> left a comment > (elementary-swift/elementary-ui#119) <#119 (comment)>
@Vithanco <https://github.com/Vithanco>> thank you for your patience, and my apologies for being annoying - but after a bit of back an forth I think my preference is the original shape: explicit copies of the API, with "clean and simple" > some View> / > some SVGView> return types - both returning the same internal type under the hood.
it feels the bit of duplication is something very easily dealt with nowadays - in contrast to the somewhat messy exposure of underscored types + more complicated type gymnastics for these extensions. so I think the price of a tiny bit more maintenance burden is justified by the simplicity of package consumers.
wdyt? do you have a preference?
also, I vaguely remember that svg elements have a different geometry behavior with transformation (eg: not "center" as anchor by default) - I have not tested it, but maybe we run into trouble by omitting the anchor for ".center" in the transforms for SVG?
—
Reply to this email directly, > view it on GitHub <#119?email_source=notifications&email_token=ABFFOMB37ICMP7KVXNFCSDL5MAJ5DA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBTHAYTOOJRGEY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5438179111>> , or > unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFFOMFVIMZI2K2RZT3VHOD5MAJ5DAVCNFSNUABFKJSXA33TNF2G64TZHM4DMNBZGU3DKNRZHNEXG43VMU5TKMJTGE2TMMBUGY3KC5QC>> .
You are receiving this because you were mentioned.> Message ID: > <elementary-swift/elementary-ui/pull/119/c5438179111> @> github> .> com>
|
|
I think taking on a little more code duplication for a simpler API and cleaner docs is worth it, so if I could ask you to finalize the PR with this shape (and maybe double-check that SVG rotations/scaling actually works as expected) we can finally get this thing merged 🙏 |
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>
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>
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>
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>
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>
|
Done — mirrored shape, You were right about the transforms: Also "answered" your |
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>
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>
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>
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>
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>
SVG content composes but cannot respond to anything.
Group,ForEach,Optional,_ConditionalContentand the tuples all carryextension …: SVGView where …, so you can build any SVG tree — and then nothing in it takes a handler or a style:One constraint causes it.
DOMEffectViewis declaredWrapped: View, but its body uses onlyWrapped.Tagand the_Mountablerequirements.ViewmeansHTML & _Mountable, so asking for mountability also charges for HTML-ness.The fix is what
_KeyedViewalready does inKeyedView.swift— generic overMarkupContent & _Mountable, conditional conformance, modifier declared once per namespace:DOMEffectViewwas the one wrapper of its kind not yet converted. The mirrored declarations are the same duplicationkey(_:)pays:some View<Tag>andsome SVGView<Tag>are distinct opaque types.