Fix XmlSerializer consuming sibling elements when deserializing empty XmlElement member

[Copilot]

Description

When deserializing a class with an XmlElement member followed by other members, a self-closing/empty element (e.g., <Description/>) caused the serializer to consume the next sibling element as the XmlElement's content, leaving subsequent members null.

public class Root {
    public XmlElement Description { get; set; }
    public string Name { get; set; }
}

// Bug: obj.Description = <Name>Test</Name>, obj.Name = null
var obj = serializer.Deserialize(new StringReader(
    @"<Root><Description/><Name>Test</Name></Root>"));

// Expected: obj.Description = null, obj.Name = "Test"

Root Cause

ReadXmlNode(bool wrapped) called _r.ReadStartElement() without first checking IsEmptyElement. For an empty element like <Description/>, ReadStartElement() advances the reader past the empty element to the next sibling. The subsequent MoveToContent() + NodeType != EndElement check then incorrectly read that sibling as child content.

Fix

• XmlSerializationReader.ReadXmlNode: Add IsEmptyElement guard before ReadStartElement() — matching the pattern already used in ReadNull(). If empty, Skip() and return null.

if (_r.IsEmptyElement)
{
    _r.Skip();
    return null;
}
_r.ReadStartElement();

• Test: Added TypeWithXmlElementMemberAndSibling to SerializationTypes.cs and a [Theory] regression test (Xml_XmlElementMember_EmptyElement_SiblingNotConsumed) covering non-empty, space-before-slash, and compact empty element forms.

[Copilot]

Pull request overview

Fixes an XmlSerializer deserialization bug where a self-closing/empty element mapped to an XmlElement/XmlNode member could incorrectly consume the next sibling element as its content, causing subsequent members to deserialize as null.

Changes:

• Update XmlSerializationReader.ReadXmlNode(bool wrapped) to treat wrapped empty elements as having no content (skip and return null) rather than advancing into the next sibling.
• Add a regression test in XmlSerializerTests.RuntimeOnly.cs covering non-empty and empty element forms.
• Add a new test type TypeWithXmlElementMemberAndSibling.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/libraries/System.Runtime.Serialization.Xml/tests/SerializationTypes.cs	Adds TypeWithXmlElementMemberAndSibling test type (but see comment about compilation inclusion).
src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.RuntimeOnly.cs	Adds regression coverage for empty XmlElement member not consuming a sibling.
src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs	Fixes wrapped empty-element handling in ReadXmlNode to avoid consuming siblings.

[rhuijben]

@copilot is this a regression or a long standing bug?

[rhuijben]

And shouldn't it still read the empty element in the XmlElement property in this case?
It was there after all..

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[github-actions]

🤖 Copilot Code Review — PR #126769

Note

This review was generated by Copilot and should be validated by a human reviewer.

Holistic Assessment

Motivation: The bug is real and well-understood. When XmlSerializer deserializes a type with an XmlElement member and the XML has a self-closing element (e.g., <Description/>), ReadStartElement() advances past the entire element, and the subsequent MoveToContent() + ReadNode logic consumes the next sibling element as child content. This silently corrupts deserialization. The <Description></Description> case (non-self-closing empty) was already handled correctly by the existing code path.

Approach: The fix adds an IsEmptyElement guard before ReadStartElement() — matching the exact same pattern already used in ReadNull() at line 827–831 of the same file. This is the right fix at the right layer (root cause in ReadXmlNode, not a workaround). An AppContext switch is provided for compat. ReadXmlDocument delegates to ReadXmlNode, so it's automatically covered.

Summary: ⚠️ Needs Human Review. The code fix itself is correct and well-targeted, and the tests are good. However, there are a few items that warrant human judgment: (1) whether returning null is the right semantic for an empty XmlElement (as @rhuijben noted in comments), (2) the behavioral change has no linked tracking issue, and (3) the default for the compat switch should be confirmed.

---

Detailed Findings

✅ Correctness — Fix is sound and addresses root cause

The fix correctly identifies the root cause: ReadStartElement() on a self-closing element advances the reader past the entire element, leaving it positioned on the next sibling. The new IsEmptyElement guard + Skip() pattern is identical to the existing pattern in ReadNull() (line 827–831), which handles the same IsEmptyElement concern for xsi:nil elements. This consistency strongly supports correctness.

The ReadXmlDocument path is automatically covered since it delegates to ReadXmlNode. The IL-emitted serializer code path (line 4852) also calls ReadXmlNode/ReadXmlDocument on the base class, so it gets the fix too.

⚠️ Behavioral semantics — Should empty element return null or an empty XmlElement?

@rhuijben raised this in the comments and it deserves attention: <Description/> is a valid XML element. Returning null means "the element was not present," but it was present — just empty. An alternative would be to return an empty XmlElement node.

However, I verified that <Description></Description> (the non-self-closing equivalent) already returned null before this PR via the existing code path (line 913–916: ReadStartElement() → MoveToContent() → sees EndElement → skips node read → returns null). So the fix is consistent with existing behavior for the non-self-closing case. If the team wants <Description/> to produce an XmlElement, then <Description></Description> should too, and that would be a separate design decision.

This is a judgment call for a human reviewer — the fix makes the two equivalent XML forms behave consistently, which is correct. But if users expect an empty XmlElement object for empty elements, that's a deeper design question unrelated to this PR.

⚠️ Compat switch default — Confirm new behavior is on by default

The switch Switch.System.Xml.UseLegacyEmptyXmlElementDeserialization defaults to false (new behavior active by default). For a behavioral change in a serializer, a human reviewer should verify this is the intended default. That said, the old behavior was clearly a bug (consuming sibling elements), so defaulting to the fix is reasonable.

✅ Test quality — Good coverage with [Theory]

The test uses [Theory] with [InlineData] covering:

• Non-empty element (content with child <p>text</p>)
• Self-closing with space (<Description />) — default behavior
• Self-closing compact (<Description/>) — compat switch false
• Non-self-closing empty (<Description></Description>) — compat switch false
• Legacy compat switch enabled (compatSwitch = true) — correctly expects the old buggy behavior (sibling consumed)

The test type TypeWithXmlElementMemberAndSibling is correctly placed in SerializationTypes.RuntimeOnly.cs, and the test is in XmlSerializerTests.RuntimeOnly.cs. No public API changes.

💡 Missing linked issue

The PR doesn't reference a specific GitHub issue (Fixes #N). For a behavioral change to a serializer, having a tracked issue with a customer repro would strengthen the justification. This is minor since the bug is self-evident from code analysis.

💡 Test coverage for XmlDocument member

The tests only cover XmlElement members. Since ReadXmlDocument also flows through ReadXmlNode, a test with an XmlDocument-typed member would provide additional confidence, though it's lower priority since the code path is clearly shared.

Generated by Code Review for issue #126769 · ◷

[StephenMolloy]

lgtm

The concern about producing a null property instead of an empty property are misplaced. Since this repo is about XmlElement, the property in the deserialized object is expected to be the xml element that was contained within the field being deserialized. In other words, the element that XmlElement represents is not the "Description" element in the test, rather it should be the xml contained within that wrapper. And if nothing is contained within that wrapper, that is the absence of an element. Thus, the XmlElement property is null.