From 0ac9675adb8a91e455ca271c54ab6a575e5ed514 Mon Sep 17 00:00:00 2001 From: Daniel Alley Date: Thu, 30 Jul 2026 14:49:20 -0400 Subject: [PATCH 1/2] Fix indented writer misaligning End tags after Comment + Text When an element contained a non-text child (e.g. Comment) followed by a Text event, the closing End tag lost its line break because Text unconditionally cleared should_line_break. Track the indent level of the most recent non-text event and force a line break for the End tag when that level is deeper than the current level after shrink. closes #276 --- Changelog.md | 5 +++ src/writer.rs | 63 ++++++++++++++++++++++++++++--------- src/writer/async_tokio.rs | 38 ++++++++++++++++++---- tests/writer-indentation.rs | 29 ++++++++++++++++- 4 files changed, 114 insertions(+), 21 deletions(-) diff --git a/Changelog.md b/Changelog.md index 4f9082e5..866ac0e3 100644 --- a/Changelog.md +++ b/Changelog.md @@ -61,6 +61,10 @@ The MSRV has been raised to 1.86. ### Bug Fixes +- [#276]: Fixed `Writer::new_with_indent` producing misaligned closing tags + when an element contained a non-text child (e.g. a comment) followed by a + text node. The `` was appended directly after the text instead of + starting on a new line at the correct indentation level. - [#977]: `NamespaceResolver::push` (and hence every `NsReader` `Start`/`Empty` event) now returns the new `NamespaceError::TooDeeplyNested` when a document nests elements deeper than `u16::MAX`, instead of overflowing the internal @@ -85,6 +89,7 @@ The MSRV has been raised to 1.86. `decode_and_unescape_value_with()`. Use `normalized_value()` and `normalized_value_with()` instead. +[#276]: https://github.com/tafia/quick-xml/issues/276 [#963]: https://github.com/tafia/quick-xml/pull/963 [#977]: https://github.com/tafia/quick-xml/issues/977 [#980]: https://github.com/tafia/quick-xml/issues/980 diff --git a/src/writer.rs b/src/writer.rs index 4affb674..6fb99414 100644 --- a/src/writer.rs +++ b/src/writer.rs @@ -260,10 +260,12 @@ impl Writer { /// Writes the given event to the underlying writer. pub fn write_event<'a, E: Into>>(&mut self, event: E) -> io::Result<()> { let mut next_should_line_break = true; + let mut mark_non_text = false; let result = match event.into() { Event::Start(e) => { let result = self.write_wrapped("<", &e, ">"); if let Some(i) = self.indent.as_mut() { + i.mark_non_text_content(); i.grow(); } result @@ -271,36 +273,60 @@ impl Writer { Event::End(e) => { if let Some(i) = self.indent.as_mut() { i.shrink(); + if i.last_non_text_level > i.current_indent_len { + i.should_line_break = true; + } } self.write_wrapped("") } - Event::Empty(e) => self.write_wrapped( - "<", - &e, - if self.config.add_space_before_slash_in_empty_elements { - " />" - } else { - "/>" - }, - ), + Event::Empty(e) => { + mark_non_text = true; + self.write_wrapped( + "<", + &e, + if self.config.add_space_before_slash_in_empty_elements { + " />" + } else { + "/>" + }, + ) + } Event::Text(e) => { next_should_line_break = false; self.write(e.as_bytes()) } - Event::Comment(e) => self.write_wrapped(""), + Event::Comment(e) => { + mark_non_text = true; + self.write_wrapped("") + } Event::CData(e) => { next_should_line_break = false; self.write(b"") } - Event::Decl(e) => self.write_wrapped(""), - Event::PI(e) => self.write_wrapped(""), - Event::DocType(e) => self.write_wrapped(""), - Event::GeneralRef(e) => self.write_wrapped("&", &e, ";"), + Event::Decl(e) => { + mark_non_text = true; + self.write_wrapped("") + } + Event::PI(e) => { + mark_non_text = true; + self.write_wrapped("") + } + Event::DocType(e) => { + mark_non_text = true; + self.write_wrapped("") + } + Event::GeneralRef(e) => { + mark_non_text = true; + self.write_wrapped("&", &e, ";") + } Event::Eof => Ok(()), }; if let Some(i) = self.indent.as_mut() { + if mark_non_text { + i.mark_non_text_content(); + } i.should_line_break = next_should_line_break; } result @@ -679,6 +705,10 @@ pub(crate) struct Indentation { indents: String, /// The current amount of indentation current_indent_len: usize, + /// Indent level at which the most recent non-text event was written. + /// Used by End to force a line break when a non-text child (Comment, + /// Empty, child Start, etc.) preceded a Text event in the same element. + last_non_text_level: usize, } impl Indentation { @@ -690,6 +720,7 @@ impl Indentation { indent_size, indents: std::iter::repeat(indent_char).take(128).collect(), current_indent_len: 0, + last_non_text_level: 0, } } @@ -721,4 +752,8 @@ impl Indentation { self.indents.push(self.indent_char); } } + + fn mark_non_text_content(&mut self) { + self.last_non_text_level = self.current_indent_len; + } } diff --git a/src/writer/async_tokio.rs b/src/writer/async_tokio.rs index a08678c9..5f6f3d0b 100644 --- a/src/writer/async_tokio.rs +++ b/src/writer/async_tokio.rs @@ -11,10 +11,12 @@ impl Writer { /// Writes the given event to the underlying writer. Async version of [`Writer::write_event`]. pub async fn write_event_async<'a, E: Into>>(&mut self, event: E) -> Result<()> { let mut next_should_line_break = true; + let mut mark_non_text = false; let result = match event.into() { Event::Start(e) => { let result = self.write_wrapped_async("<", &e, ">").await; if let Some(i) = self.indent.as_mut() { + i.mark_non_text_content(); i.grow(); } result @@ -22,28 +24,52 @@ impl Writer { Event::End(e) => { if let Some(i) = self.indent.as_mut() { i.shrink(); + if i.last_non_text_level > i.current_indent_len { + i.should_line_break = true; + } } self.write_wrapped_async("").await } - Event::Empty(e) => self.write_wrapped_async("<", &e, "/>").await, + Event::Empty(e) => { + mark_non_text = true; + self.write_wrapped_async("<", &e, "/>").await + } Event::Text(e) => { next_should_line_break = false; self.write_async(e.as_bytes()).await } - Event::Comment(e) => self.write_wrapped_async("").await, + Event::Comment(e) => { + mark_non_text = true; + self.write_wrapped_async("").await + } Event::CData(e) => { next_should_line_break = false; self.write_async(b"").await } - Event::Decl(e) => self.write_wrapped_async("").await, - Event::PI(e) => self.write_wrapped_async("").await, - Event::DocType(e) => self.write_wrapped_async("").await, - Event::GeneralRef(e) => self.write_wrapped_async("&", &e, ";").await, + Event::Decl(e) => { + mark_non_text = true; + self.write_wrapped_async("").await + } + Event::PI(e) => { + mark_non_text = true; + self.write_wrapped_async("").await + } + Event::DocType(e) => { + mark_non_text = true; + self.write_wrapped_async("").await + } + Event::GeneralRef(e) => { + mark_non_text = true; + self.write_wrapped_async("&", &e, ";").await + } Event::Eof => Ok(()), }; if let Some(i) = self.indent.as_mut() { + if mark_non_text { + i.mark_non_text_content(); + } i.should_line_break = next_should_line_break; } result diff --git a/tests/writer-indentation.rs b/tests/writer-indentation.rs index 4e23bbc6..17f933e0 100644 --- a/tests/writer-indentation.rs +++ b/tests/writer-indentation.rs @@ -1,4 +1,4 @@ -use quick_xml::events::{BytesStart, BytesText, Event}; +use quick_xml::events::{BytesEnd, BytesStart, BytesText, Event}; use quick_xml::writer::Writer; use pretty_assertions::assert_eq; @@ -550,3 +550,30 @@ mod in_attributes_multi { ); } } + +// Regression test for https://github.com/tafia/quick-xml/issues/276 +// Elements containing both a comment and a text node should not produce +// misaligned closing tags. +#[test] +fn issue_276_comment_then_text_indent() { + let mut buffer = Vec::new(); + let mut writer = Writer::new_with_indent(&mut buffer, b' ', 4); + + writer + .write_event(Event::Start(BytesStart::new("tag2"))) + .unwrap(); + writer + .write_event(Event::Comment(BytesText::new("Test comment"))) + .unwrap(); + writer + .write_event(Event::Text(BytesText::new("Test"))) + .unwrap(); + writer + .write_event(Event::End(BytesEnd::new("tag2"))) + .unwrap(); + + let output = std::str::from_utf8(&buffer).unwrap(); + // The closing must start on a new line at the same indentation + // level as , not immediately after the text node. + assert_eq!(output, "\n Test\n"); +} From cda93fcaa0cfbde057e880d2d698366639798322 Mon Sep 17 00:00:00 2001 From: Daniel Alley Date: Thu, 30 Jul 2026 17:45:10 -0400 Subject: [PATCH 2/2] temp --- src/writer.rs | 54 ++++++++++++++------------------------- src/writer/async_tokio.rs | 38 ++++++++------------------- 2 files changed, 30 insertions(+), 62 deletions(-) diff --git a/src/writer.rs b/src/writer.rs index 6fb99414..45c8c534 100644 --- a/src/writer.rs +++ b/src/writer.rs @@ -260,12 +260,12 @@ impl Writer { /// Writes the given event to the underlying writer. pub fn write_event<'a, E: Into>>(&mut self, event: E) -> io::Result<()> { let mut next_should_line_break = true; - let mut mark_non_text = false; + let mut should_mark_non_text = true; let result = match event.into() { Event::Start(e) => { + should_mark_non_text = false; let result = self.write_wrapped("<", &e, ">"); if let Some(i) = self.indent.as_mut() { - i.mark_non_text_content(); i.grow(); } result @@ -279,52 +279,36 @@ impl Writer { } self.write_wrapped("") } - Event::Empty(e) => { - mark_non_text = true; - self.write_wrapped( - "<", - &e, - if self.config.add_space_before_slash_in_empty_elements { - " />" - } else { - "/>" - }, - ) - } + Event::Empty(e) => self.write_wrapped( + "<", + &e, + if self.config.add_space_before_slash_in_empty_elements { + " />" + } else { + "/>" + }, + ), Event::Text(e) => { next_should_line_break = false; + should_mark_non_text = false; self.write(e.as_bytes()) } - Event::Comment(e) => { - mark_non_text = true; - self.write_wrapped("") - } + Event::Comment(e) => self.write_wrapped(""), Event::CData(e) => { next_should_line_break = false; + should_mark_non_text = false; self.write(b"") } - Event::Decl(e) => { - mark_non_text = true; - self.write_wrapped("") - } - Event::PI(e) => { - mark_non_text = true; - self.write_wrapped("") - } - Event::DocType(e) => { - mark_non_text = true; - self.write_wrapped("") - } - Event::GeneralRef(e) => { - mark_non_text = true; - self.write_wrapped("&", &e, ";") - } + Event::Decl(e) => self.write_wrapped(""), + Event::PI(e) => self.write_wrapped(""), + Event::DocType(e) => self.write_wrapped(""), + Event::GeneralRef(e) => self.write_wrapped("&", &e, ";"), Event::Eof => Ok(()), }; if let Some(i) = self.indent.as_mut() { - if mark_non_text { + if should_mark_non_text { i.mark_non_text_content(); } i.should_line_break = next_should_line_break; diff --git a/src/writer/async_tokio.rs b/src/writer/async_tokio.rs index 5f6f3d0b..eeca9b2b 100644 --- a/src/writer/async_tokio.rs +++ b/src/writer/async_tokio.rs @@ -11,12 +11,12 @@ impl Writer { /// Writes the given event to the underlying writer. Async version of [`Writer::write_event`]. pub async fn write_event_async<'a, E: Into>>(&mut self, event: E) -> Result<()> { let mut next_should_line_break = true; - let mut mark_non_text = false; + let mut should_mark_non_text = true; let result = match event.into() { Event::Start(e) => { + should_mark_non_text = false; let result = self.write_wrapped_async("<", &e, ">").await; if let Some(i) = self.indent.as_mut() { - i.mark_non_text_content(); i.grow(); } result @@ -30,44 +30,28 @@ impl Writer { } self.write_wrapped_async("").await } - Event::Empty(e) => { - mark_non_text = true; - self.write_wrapped_async("<", &e, "/>").await - } + Event::Empty(e) => self.write_wrapped_async("<", &e, "/>").await, Event::Text(e) => { next_should_line_break = false; + should_mark_non_text = false; self.write_async(e.as_bytes()).await } - Event::Comment(e) => { - mark_non_text = true; - self.write_wrapped_async("").await - } + Event::Comment(e) => self.write_wrapped_async("").await, Event::CData(e) => { next_should_line_break = false; + should_mark_non_text = false; self.write_async(b"").await } - Event::Decl(e) => { - mark_non_text = true; - self.write_wrapped_async("").await - } - Event::PI(e) => { - mark_non_text = true; - self.write_wrapped_async("").await - } - Event::DocType(e) => { - mark_non_text = true; - self.write_wrapped_async("").await - } - Event::GeneralRef(e) => { - mark_non_text = true; - self.write_wrapped_async("&", &e, ";").await - } + Event::Decl(e) => self.write_wrapped_async("").await, + Event::PI(e) => self.write_wrapped_async("").await, + Event::DocType(e) => self.write_wrapped_async("").await, + Event::GeneralRef(e) => self.write_wrapped_async("&", &e, ";").await, Event::Eof => Ok(()), }; if let Some(i) = self.indent.as_mut() { - if mark_non_text { + if should_mark_non_text { i.mark_non_text_content(); } i.should_line_break = next_should_line_break;