Skip to content

avm2: Complete XML change notifications - #24131

Open
falpi wants to merge 1 commit into
ruffle-rs:masterfrom
falpi:xml-notifications-completion
Open

avm2: Complete XML change notifications#24131
falpi wants to merge 1 commit into
ruffle-rs:masterfrom
falpi:xml-notifications-completion

Conversation

@falpi

@falpi falpi commented Jul 5, 2026

Copy link
Copy Markdown

What

Ruffle currently emits E4X change notifications only for attribute writes and name changes (attributeAdded/attributeChanged from #23478, nameSet from #23582). Every other mutation — element text assignment, child insertion/replacement/removal, attribute deletion, namespace changes, normalize() — mutates the tree silently.

This PR implements the remaining commands of the (undocumented) XML.setNotification() protocol: textSet, nodeAdded, nodeChanged, nodeRemoved, attributeRemoved, namespaceAdded, namespaceRemoved, namespaceSet, covering every avmplus emission site, AS3_normalize included. setNotification is no longer marked as a stub.

Related to #23446, which was fixed for the attribute case only: the same COLLECTION_CHANGE machinery still doesn't fire for element mutations.

Why

The Flex framework builds its whole XML data-binding pipeline on this protocol: mx.utils.XMLNotifier installs a notification function on every item of an XML data provider, and mx.collections.XMLListAdapter.xmlNotification turns each notification into itemUpdated()COLLECTION_CHANGE, which refreshes list/grid renderers and re-runs dependent bindings.

With attribute-only notifications, attribute-driven UIs work but element-driven ones silently don't: a two-way binding writing item.FIELD = text (editable grid cells, "enable the button when the value differs" forms) updates the XML but never triggers the collection event, so the UI stays stale until something else happens to re-assign renderer data.

How

Semantics are transcribed from the avmplus sources (XMLObject.cpp, E4XNode.cpp, XMLListObject.cpp):

  • textSet is issued from within [[Replace]] (E4XNode::replace, text branch), with the new text node as target and the prior value as detail — mirroring ElementE4XNode::_replace. One emission site covers primitive property assignment, XMLList index puts, XML.replace() and setChildren(), and matches the target.parent().parent() === currentTarget branch XMLListAdapter.xmlNotification has for it.
  • [[Replace]] takes avmplus's pastValue argument: the primitive [[Put]] deletes all properties of x[i] before replacing, so the prior content would be gone by the time textSet fires. Like avmplus setMultinameProperty, the old first child is captured (only when a watcher is reachable) and passed through, so the notification detail still reports the replaced content.
  • Structural commands are issued by the operations through a new XmlObject::trigger_child_changes helper replicating avmplus childChanges: it only fires when the value is an XML/XMLList object and passes the replaced node, if any, as detail. Call sites: set_property_local (duplicate collapse → nodeRemoved, replace/append → nodeChanged/nodeAdded), delete_property_local, XMLList numeric put/delete, prependChild/insertChildBefore/insertChildAfter, XML.replace(), and the namespace methods.
  • normalize() is reworked from one borrow held across its whole loop to short per-iteration borrows with re-validated indices, so it can emit like avmplus AS3_normalize does: nodeRemoved for every merged-away or whitespace-dropped text node, textSet from the surviving node when merging changed its value. The index re-validation also makes the loop robust against callbacks that mutate the tree mid-walk. XMLList.prototype.normalize gets the same treatment.
  • E4XNode::notify_needed() replicates the avmplus notifyNeeded fast path, so unwatched trees pay nothing beyond a parent-chain walk.
  • remove_matching_children/remove_matching_attribute now also return the removed nodes, so duplicate collapses and deletions can be reported.
  • XMLList index puts follow avmplus setUintProperty: creating the slot emits nothing, the replace path notifies nodeAdded, and XMLList values notify nodeChanged/nodeAdded per inserted child — so appendChild yields exactly one nodeAdded, matching the avmplus test's ground truth.
  • Notifications invoke arbitrary ActionScript synchronously: emission sites are placed after the mutation completes and never while a kind_mut/children_mut borrow is live. Three pre-existing call sites were re-scoped accordingly, as they would otherwise panic with a RefCell double borrow once their inner [[Replace]] started emitting textSet.

Tests

  • tests/swfs/from_avmplus/e4x/XML/setNotification — the ported avmplus coverage test (attribute add/change/remove, node add/change/remove, textSet, nameSet, namespaceSet, plus the TypeError #1034 check) was known_failure and now passes; the flag is removed.
  • tests/swfs/avm2/xml_notification_bubbling — pins exact traces for the attribute/nameSet paths across a multi-level watch; those paths are untouched and the test stays green.
  • Verified end-to-end against the Flex 4.6 data-binding stack (XMLNotifier/XMLListAdapter) in a production Flex application running under Ruffle: element-text edits now propagate to COLLECTION_CHANGE exactly as under Flash Player.

Notification test harness (attached)

The attached application.zip contains a small self-contained Flex application (MXML source + compiled SWF) that exercises every use case of the notification protocol — 25 checks in all:
application.zip

  • attribute add / change / remove, with value/detail payload verification (old value reporting);
  • element text assignment on existing and new properties (the textSet + pastValue paths, including target.parent() identification as used by XMLListAdapter);
  • node add / change / remove through both the XML and XMLList mutation paths (appendChild, prependChild, insertChildBefore/After, numeric puts and deletes, replace(), setChildren());
  • nameSet and the three namespace commands;
  • the three normalize() scenarios (adjacent-text merge, whitespace-only removal, merge-collapsing-to-whitespace);
  • notification bubbling from a grandchild to the watched root, and un-watching via setNotification(null).

Each case installs a watcher, performs one mutation, and prints the observed command sequence with a PASS/FAIL verdict against the expected one. Since it's a plain SWF, it runs unchanged under Flash Player too, giving a side-by-side ground-truth comparison — handy for reviewing this PR and for catching regressions in the future. All 22 checks pass under Ruffle with this PR applied.


The second commit adds a self-contained design/reference document (docs/xml-notifications.html); happy to drop it from the PR if it doesn't belong in-tree.

Tests

  • tests/swfs/from_avmplus/e4x/XML/setNotification — the ported avmplus coverage test (attribute add/change/remove, node add/change/remove, textSet, nameSet, namespaceSet, plus the TypeError #1034 check) was known_failure and now passes; the flag is removed.
  • tests/swfs/avm2/xml_notification_bubbling — pins exact traces for the attribute/nameSet paths across a multi-level watch; those paths are untouched and the test stays green.
  • Verified end-to-end against the Flex 4.6 data-binding stack (XMLNotifier/XMLListAdapter) in a production Flex application running under Ruffle, plus a dedicated 22-case harness (attributes, text, structure, namespaces, normalize, bubbling, unwatch) cross-checked against Flash Player.

Checklist

  • I, a human, have self-reviewed this PR and fully understand the changes within.
  • I have made or updated tests where possible.
  • All of my commits are properly scoped, compile successfully, and pass all tests.
  • This PR does not make sense to split up into smaller PRs.
  • An LLM was involved in the authoring of this code.

@kjarosh

kjarosh commented Jul 5, 2026

Copy link
Copy Markdown
Member

Does this contain MPL-licensed code?

@falpi

falpi commented Jul 5, 2026

Copy link
Copy Markdown
Author

Does this contain MPL-licensed code?

No. The Rust implementation is original code written for this PR — nothing was copied or mechanically translated from avmplus. The MPL-licensed avmplus sources (XMLObject.cpp, E4XNode.cpp, XMLListObject.cpp) were used as a behavioral reference only, to determine which notification command fires on which operation and with which target/value/detail arguments — the same way the existing AVM2 code references avmplus semantics throughout (including the pre-existing "avmplus does X" notes in the very files this PR touches). The command names and the callback signature are interface facts required for compatibility.

The only MPL-covered file the PR touches is tests/swfs/from_avmplus/e4x/XML/setNotification/Test.as, which was already ported in-tree with the rest of the from_avmplus corpus and keeps its MPL header; this PR only removes the known_failure flag from its test.toml.

If the "semantics are transcribed" wording in the description raised the concern, I'm happy to rephrase it to "derived by studying the avmplus behavior".

@falpi
falpi force-pushed the xml-notifications-completion branch 3 times, most recently from 1da829a to 9dba71c Compare July 7, 2026 21:01
Upstream only triggered notifications for attribute changes and name
changes (attributeAdded, attributeChanged, nameSet). Implement the
remaining commands emitted by avmplus: textSet, nodeAdded, nodeChanged,
nodeRemoved, attributeRemoved, namespaceAdded, namespaceRemoved and
namespaceSet.

Semantics are transcribed from avmplus (XMLObject.cpp, E4XNode.cpp,
XMLListObject.cpp):

- textSet is issued from within [[Replace]] (E4XNode::replace) with the
  new text node as target, covering primitive property assignment,
  XMLList index puts, XML.replace() and setChildren().
- [[Replace]] takes avmplus's pastValue argument: the primitive [[Put]]
  clears x[i] before replacing, so the old first child is captured up
  front and still reported as the textSet detail.
- Structural commands are issued by the callers through the new
  XmlObject::trigger_child_changes helper (avmplus childChanges: only
  fires for XML/XMLList values, prior node as detail).
- normalize() is reworked from one borrow held across its whole loop to
  short per-iteration borrows with re-validated indices, so it can emit
  like avmplus AS3_normalize: nodeRemoved for every merged-away or
  whitespace-dropped text node, textSet from the surviving node when
  merging changed its value. XMLList.prototype.normalize gets the same
  treatment.
- E4XNode::notify_needed() replicates the avmplus notifyNeeded fast
  path, so unwatched trees pay nothing.
- remove_matching_children/attribute also return the removed nodes so
  duplicate collapses can be notified.
- Notifications run arbitrary AS3 synchronously: emission sites are
  placed after the mutation completes and never while a kind_mut /
  children_mut borrow is live; three pre-existing call sites were
  rescoped accordingly.
- XMLList index puts follow avmplus setUintProperty: creating the slot
  emits nothing, the replace path notifies nodeAdded, and XMLList
  values notify nodeChanged/nodeAdded per inserted child - so
  appendChild yields exactly one nodeAdded.
- setNotification is no longer a stub.

This unblocks the Flex XML data-binding chain for element mutations
(XMLNotifier -> XMLListAdapter -> itemUpdated -> COLLECTION_CHANGE),
which previously reacted to attribute writes only.

Un-mark from_avmplus/e4x/XML/setNotification as known_failure and
remove its now-stale output.ruffle.txt.
@falpi
falpi force-pushed the xml-notifications-completion branch from 9dba71c to c75fed8 Compare July 7, 2026 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants