Support converting deprecated inner text with embedded comments

Fixes 6847

4 files changed, 581 insertions, 75 deletions

diff --git a/src/wix/WixToolset.Converters/ConversionLab.cs b/src/wix/WixToolset.Converters/ConversionLab.cs
new file mode 100644
index 00000000..903071f8
--- /dev/null
+++ b/src/wix/WixToolset.Converters/ConversionLab.cs
@@ -0,0 +1,91 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the Microsoft Reciprocal License. See LICENSE.TXT file in the project root for full license information.
+
+namespace WixToolset.Converters
+{
+    using System;
+    using System.Collections.Generic;
+    using System.Linq;
+    using System.Xml.Linq;
+
+    internal class ConversionLab : IDisposable
+    {
+        private readonly XElement targetElement;
+        private readonly XElement parentElement;
+        private readonly List<XNode> siblingNodes;
+        private int index;
+
+        public ConversionLab(XElement targetElement)
+        {
+            this.targetElement = targetElement;
+            this.parentElement = this.targetElement.Parent;
+            this.siblingNodes = this.parentElement.Nodes().ToList();
+
+            foreach (var siblingNode in this.siblingNodes)
+            {
+                siblingNode.Remove();
+            }
+
+            this.index = this.siblingNodes.IndexOf(this.targetElement);
+        }
+
+        public void RemoveOrphanTextNodes()
+        {
+            if (!this.targetElement.HasElements)
+            {
+                var childNodes = this.targetElement.Nodes().ToList();
+                foreach (var childNode in childNodes)
+                {
+                    childNode.Remove();
+                }
+            }
+        }
+
+        public void RemoveTargetElement()
+        {
+            if (this.index + 1 < this.siblingNodes.Count
+             && this.siblingNodes[this.index + 1] is XText trailingText
+             && String.IsNullOrWhiteSpace(trailingText.Value))
+            {
+                this.siblingNodes.RemoveAt(this.index + 1);
+            }
+            this.siblingNodes.RemoveAt(this.index);
+            if (0 < this.index
+             && this.siblingNodes[this.index - 1] is XText leadingText
+             && String.IsNullOrWhiteSpace(leadingText.Value))
+            {
+                this.siblingNodes.RemoveAt(this.index - 1);
+            }
+        }
+
+        public void ReplaceTargetElement(XElement replacement)
+        {
+            this.siblingNodes[this.index] = replacement;
+        }
+
+        public void AddCommentsAsSiblings(List<XNode> comments)
+        {
+            if (0 < this.index
+             && this.siblingNodes[this.index - 1] is XText leadingText
+             && String.IsNullOrWhiteSpace(leadingText.Value))
+            {
+                var leadingWhitespace = leadingText.Value;
+                --this.index;
+                var newComments = new List<XNode>();
+                foreach(var comment in comments)
+                {
+                    newComments.Add(new XText(leadingWhitespace));
+                    newComments.Add(comment);
+                }
+                comments = newComments;
+            }
+
+            this.siblingNodes.InsertRange(this.index, comments);
+            this.index = this.siblingNodes.IndexOf(this.targetElement);
+        }
+
+        public void Dispose()
+        {
+            this.parentElement.Add(this.siblingNodes);
+        }
+    }
+}
diff --git a/src/wix/WixToolset.Converters/WixConverter.cs b/src/wix/WixToolset.Converters/WixConverter.cs
index 9b0bebc1..de81c876 100644
--- a/src/wix/WixToolset.Converters/WixConverter.cs
+++ b/src/wix/WixToolset.Converters/WixConverter.cs
@@ -525,7 +525,7 @@ namespace WixToolset.Converters
         {
             if (!WixConverter.LeadingWhitespaceValid(this.IndentationAmount, level, whitespace.Value))
             {
-                var message = testType == ConverterTestType.WhitespacePrecedingEndElementWrong ? "The whitespace preceding this end element is incorrect." : "The whitespace preceding this node is incorrect.";
+                var message = testType == ConverterTestType.WhitespacePrecedingEndElementWrong ? "The whitespace preceding this end element is incorrect." : "The whitespace preceding this comment is incorrect.";
 
                 if (this.OnInformation(testType, node, message))
                 {
@@ -538,7 +538,7 @@ namespace WixToolset.Converters
         {
             if (!String.IsNullOrEmpty(whitespace.Value) && whitespace.NodeType != XmlNodeType.CDATA)
             {
-                var message = testType == ConverterTestType.WhitespacePrecedingEndElementWrong ? "The whitespace preceding this end element is incorrect." : "The whitespace preceding this node is incorrect.";
+                var message = testType == ConverterTestType.WhitespacePrecedingEndElementWrong ? "The whitespace preceding this end element is incorrect." : "The whitespace preceding this comment is incorrect.";
 
                 if (this.OnInformation(testType, node, message))
                 {
@@ -924,23 +924,29 @@ namespace WixToolset.Converters
 
         private void ConvertControlElement(XElement element)
         {
-            var remove = new List<XElement>();
-
-            foreach (var xCondition in element.Elements(ConditionElementName))
+            using (var lab = new ConversionLab(element))
             {
-                var action = UppercaseFirstChar(xCondition.Attribute("Action")?.Value);
-                if (!String.IsNullOrEmpty(action) &&
-                    TryGetInnerText(xCondition, out var text) &&
-                    this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the '{1}Condition' attribute instead.", xCondition.Name.LocalName, action))
+                var xConditions = element.Elements(ConditionElementName).ToList();
+                var comments = new List<XNode>();
+
+                foreach (var xCondition in xConditions)
                 {
-                    element.Add(new XAttribute(action + "Condition", text));
-                    remove.Add(xCondition);
+                    var action = UppercaseFirstChar(xCondition.Attribute("Action")?.Value);
+                    if (!String.IsNullOrEmpty(action) &&
+                        TryGetInnerText(xCondition, out var text, out comments, comments) &&
+                        this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the '{1}Condition' attribute instead.", xCondition.Name.LocalName, action))
+                    {
+                        element.Add(new XAttribute(action + "Condition", text));
+                    }
                 }
-            }
 
-            for (var i = remove.Count - 1; i >= 0; i--)
-            {
-                remove[i].Remove();
+                foreach (var xCondition in xConditions)
+                {
+                    xCondition.Remove();
+                }
+
+                lab.RemoveOrphanTextNodes();
+                lab.AddCommentsAsSiblings(comments);
             }
         }
 
@@ -958,11 +964,16 @@ namespace WixToolset.Converters
             var xCondition = element.Element(ConditionElementName);
             if (xCondition != null)
             {
-                if (TryGetInnerText(xCondition, out var text) &&
+                if (TryGetInnerText(xCondition, out var text, out var comments) &&
                     this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Condition' attribute instead.", xCondition.Name.LocalName))
                 {
-                    element.Add(new XAttribute("Condition", text));
-                    xCondition.Remove();
+                    using (var lab = new ConversionLab(element))
+                    {
+                        xCondition.Remove();
+                        element.Add(new XAttribute("Condition", text));
+                        lab.RemoveOrphanTextNodes();
+                        lab.AddCommentsAsSiblings(comments);
+                    }
                 }
             }
 
@@ -1057,14 +1068,16 @@ namespace WixToolset.Converters
             {
                 var level = xCondition.Attribute("Level")?.Value;
                 if (!String.IsNullOrEmpty(level) &&
-                    TryGetInnerText(xCondition, out var text) &&
+                    TryGetInnerText(xCondition, out var text, out var comments) &&
                     this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Level' element instead.", xCondition.Name.LocalName))
                 {
-                    xCondition.AddAfterSelf(new XElement(LevelElementName,
-                        new XAttribute("Value", level),
-                        new XAttribute("Condition", text)
-                        ));
-                    xCondition.Remove();
+                    using (var lab = new ConversionLab(xCondition))
+                    {
+                        lab.ReplaceTargetElement(new XElement(LevelElementName,
+                                                              new XAttribute("Value", level),
+                                                              new XAttribute("Condition", text)));
+                        lab.AddCommentsAsSiblings(comments);
+                    }
                 }
             }
         }
@@ -1097,28 +1110,25 @@ namespace WixToolset.Converters
 
         private void ConvertFragmentElement(XElement element)
         {
-            var remove = new List<XElement>();
+            var xConditions = element.Elements(ConditionElementName).ToList();
 
-            foreach (var xCondition in element.Elements(ConditionElementName))
+            foreach (var xCondition in xConditions)
             {
                 var message = xCondition.Attribute("Message")?.Value;
 
                 if (!String.IsNullOrEmpty(message) &&
-                    TryGetInnerText(xCondition, out var text) &&
+                    TryGetInnerText(xCondition, out var text, out var comments) &&
                     this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Launch' element instead.", xCondition.Name.LocalName))
                 {
-                    xCondition.AddAfterSelf(new XElement(LaunchElementName,
-                        new XAttribute("Condition", text),
-                        new XAttribute("Message", message)
-                        ));
-                    remove.Add(xCondition);
+                    using (var lab = new ConversionLab(xCondition))
+                    {
+                        lab.ReplaceTargetElement(new XElement(LaunchElementName,
+                                                              new XAttribute("Condition", text),
+                                                              new XAttribute("Message", message)));
+                        lab.AddCommentsAsSiblings(comments);
+                    }
                 }
             }
-
-            for (var i = remove.Count - 1; i >= 0; i--)
-            {
-                remove[i].Remove();
-            }
         }
 
         private void ConvertFirewallRemoteAddressElement(XElement element)
@@ -1161,11 +1171,18 @@ namespace WixToolset.Converters
             var xCondition = element.Element(ConditionElementName);
             if (xCondition != null)
             {
-                if (TryGetInnerText(xCondition, out var text) &&
+                if (TryGetInnerText(xCondition, out var text, out var comments) &&
                     this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Condition' attribute instead.", xCondition.Name.LocalName))
                 {
-                    element.Add(new XAttribute("Condition", text));
-                    xCondition.Remove();
+                    using (var lab = new ConversionLab(xCondition))
+                    {
+                        lab.RemoveTargetElement();
+                    }
+                    using (var lab = new ConversionLab(element))
+                    {
+                        element.Add(new XAttribute("Condition", text));
+                        lab.AddCommentsAsSiblings(comments);
+                    }
                 }
             }
         }
@@ -1251,14 +1268,16 @@ namespace WixToolset.Converters
                 var message = xCondition.Attribute("Message")?.Value;
 
                 if (!String.IsNullOrEmpty(message) &&
-                    TryGetInnerText(xCondition, out var text) &&
+                    TryGetInnerText(xCondition, out var text, out var comments) &&
                     this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Launch' element instead.", xCondition.Name.LocalName))
                 {
-                    xCondition.AddAfterSelf(new XElement(LaunchElementName,
-                        new XAttribute("Condition", text),
-                        new XAttribute("Message", message)
-                        ));
-                    xCondition.Remove();
+                    using (var lab = new ConversionLab(xCondition))
+                    {
+                        lab.ReplaceTargetElement(new XElement(LaunchElementName,
+                                                              new XAttribute("Condition", text),
+                                                              new XAttribute("Message", message)));
+                        lab.AddCommentsAsSiblings(comments);
+                    }
                 }
             }
 
@@ -1540,13 +1559,23 @@ namespace WixToolset.Converters
 
         private void ConvertPublishElement(XElement element)
         {
-            this.ConvertInnerTextToAttribute(element, "Condition");
-
-            var xCondition = element.Attribute("Condition");
-            if (xCondition?.Value == "1" &&
-                this.OnInformation(ConverterTestType.PublishConditionOneUnnecessary, element, "Adding Condition='1' on {0} elements is no longer necessary. Remove the Condition attribute.", xCondition.Name.LocalName))
+            if (TryGetInnerText(element, out var text, out var comments) &&
+                this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the 'Condition' attribute instead.", element.Name.LocalName))
             {
-                xCondition.Remove();
+                using (var lab = new ConversionLab(element))
+                {
+                    if ("1" == text)
+                    {
+                        this.OnInformation(ConverterTestType.PublishConditionOneUnnecessary, element, "Adding Condition='1' on {0} elements is no longer necessary. Remove the Condition attribute.", element.Name.LocalName);
+                    }
+                    else
+                    {
+                        element.Add(new XAttribute("Condition", text));
+                    }
+
+                    lab.RemoveOrphanTextNodes();
+                    lab.AddCommentsAsSiblings(comments);
+                }
             }
         }
 
@@ -1824,7 +1853,7 @@ namespace WixToolset.Converters
 
             var xScript = xCustomAction.Attribute("Script");
 
-            if (xScript != null && TryGetInnerText(xCustomAction, out var scriptText))
+            if (xScript != null && TryGetInnerText(xCustomAction, out var scriptText, out var comments))
             {
                 if (this.OnInformation(ConverterTestType.InnerTextDeprecated, xCustomAction, "Using {0} element text is deprecated. Extract the text to a file and use the 'ScriptSourceFile' attribute to reference it.", xCustomAction.Name.LocalName))
                 {
@@ -1837,6 +1866,25 @@ namespace WixToolset.Converters
 
                     RemoveChildren(xCustomAction);
                     xCustomAction.Add(new XAttribute("ScriptSourceFile", scriptFile));
+
+                    if (comments.Any())
+                    {
+                        var remainingNodes = xCustomAction.NodesAfterSelf().ToList();
+                        var replacementNodes = remainingNodes.Where(e => XmlNodeType.Text != e.NodeType);
+                        foreach (var node in remainingNodes)
+                        {
+                            node.Remove();
+                        }
+                        foreach (var comment in comments)
+                        {
+                            xCustomAction.Add(comment);
+                            xCustomAction.Add("\n");
+                        }
+                        foreach (var node in replacementNodes)
+                        {
+                            xCustomAction.Add(node);
+                        }
+                    }
                 }
             }
         }
@@ -1956,11 +2004,15 @@ namespace WixToolset.Converters
 
         private void ConvertInnerTextToAttribute(XElement element, string attributeName)
         {
-            if (TryGetInnerText(element, out var text) &&
+            if (TryGetInnerText(element, out var text, out var comments) &&
                 this.OnInformation(ConverterTestType.InnerTextDeprecated, element, "Using {0} element text is deprecated. Use the '{1}' attribute instead.", element.Name.LocalName, attributeName))
             {
-                element.Add(new XAttribute(attributeName, text));
-                RemoveChildren(element);
+                using (var lab = new ConversionLab(element))
+                {
+                    lab.RemoveOrphanTextNodes();
+                    element.Add(new XAttribute(attributeName, text));
+                    lab.AddCommentsAsSiblings(comments);
+                }
             }
         }
 
@@ -2271,16 +2323,36 @@ namespace WixToolset.Converters
             return value;
         }
 
-        private static bool TryGetInnerText(XElement element, out string value)
+        private static bool TryGetInnerText(XElement element, out string value, out List<XNode> comments)
+        {
+            return TryGetInnerText(element, out value, out comments, new List<XNode>());
+        }
+
+        private static bool TryGetInnerText(XElement element, out string value, out List<XNode> comments, List<XNode> initialComments)
         {
             value = null;
+            comments = null;
             var found = false;
 
             var nodes = element.Nodes().ToList();
+            comments = initialComments;
+            var nonCommentNodes = new List<XNode>();
+
+            foreach(var node in nodes)
+            {
+                if (XmlNodeType.Comment == node.NodeType)
+                {
+                    comments.Add(node);
+                }
+                else
+                {
+                    nonCommentNodes.Add(node);
+                }
+            }
 
-            if (nodes.Any() && nodes.All(e => e.NodeType == XmlNodeType.Text || e.NodeType == XmlNodeType.CDATA))
+            if (nonCommentNodes.Any() && nonCommentNodes.All(e => e.NodeType == XmlNodeType.Text || e.NodeType == XmlNodeType.CDATA))
             {
-                value = String.Join(String.Empty, nodes.Cast<XText>().Select(TrimTextValue));
+                value = String.Join(String.Empty, nonCommentNodes.Cast<XText>().Select(TrimTextValue));
                 found = true;
             }
 
diff --git a/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs b/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs
index daf66a8b..c9fecc94 100644
--- a/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs
+++ b/src/wix/test/WixToolsetTest.Converters/ConditionFixture.cs
@@ -35,13 +35,55 @@ namespace WixToolsetTest.Converters
                 "  <Fragment>",
                 "    <UI>",
                 "      <Dialog Id=\"Dlg1\">",
-                "        <Control Id=\"Control1\" DisableCondition=\"x=y\" HideCondition=\"a&lt;&gt;b\">",
-                "          ",
-                "          ",
+                "        <Control Id=\"Control1\" DisableCondition=\"x=y\" HideCondition=\"a&lt;&gt;b\" />",
+                "      </Dialog>",
+                "    </UI>",
+                "  </Fragment>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(4, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
+
+        [Fact]
+        public void FixControlConditionWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi'>",
+                "  <Fragment>",
+                "    <UI>",
+                "      <Dialog Id='Dlg1'>",
+                "        <Control Id='Control1'>",
+                "          <Condition Action='disable'><!-- Comment 1 -->x=y</Condition>",
+                "          <Condition Action='hide'><!-- Comment 2 -->a&lt;>b</Condition>",
                 "        </Control>",
                 "      </Dialog>",
                 "    </UI>",
                 "  </Fragment>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
+                "  <Fragment>",
+                "    <UI>",
+                "      <Dialog Id=\"Dlg1\">",
+                "        <!-- Comment 1 -->",
+                "        <!-- Comment 2 -->",
+                "        <Control Id=\"Control1\" DisableCondition=\"x=y\" HideCondition=\"a&lt;&gt;b\" />",
+                "      </Dialog>",
+                "    </UI>",
+                "  </Fragment>",
                 "</Wix>"
             };
 
@@ -102,6 +144,53 @@ namespace WixToolsetTest.Converters
             var actualLines = UnformattedDocumentLines(document);
             WixAssert.CompareLineByLine(expected, actualLines);
         }
+        [Fact]
+        public void FixPublishConditionWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi'>",
+                "  <Fragment>",
+                "    <UI>",
+                "      <Dialog Id='Dlg1'>",
+                "        <Control Id='Control1'>",
+                "          <Publish Value='abc'><!-- Comment 1 -->1&lt;2</Publish>",
+                "          <Publish Value='gone'><!-- Comment 2 -->1</Publish>",
+                "        </Control>",
+                "      </Dialog>",
+                "    </UI>",
+                "  </Fragment>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
+                "  <Fragment>",
+                "    <UI>",
+                "      <Dialog Id=\"Dlg1\">",
+                "        <Control Id=\"Control1\">",
+                "          <!-- Comment 1 -->",
+                "          <Publish Value=\"abc\" Condition=\"1&lt;2\" />",
+                "          <!-- Comment 2 -->",
+                "          <Publish Value=\"gone\" />",
+                "        </Control>",
+                "      </Dialog>",
+                "    </UI>",
+                "  </Fragment>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(5, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
 
         [Fact]
         public void FixComponentCondition()
@@ -120,10 +209,43 @@ namespace WixToolsetTest.Converters
             {
                 "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
                 "  <Fragment>",
-                "    <Component Id=\"Comp1\" Directory=\"ApplicationFolder\" Condition=\"1&lt;2\">",
-                "      ",
+                "    <Component Id=\"Comp1\" Directory=\"ApplicationFolder\" Condition=\"1&lt;2\" />",
+                "  </Fragment>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(3, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
+
+        [Fact]
+        public void FixComponentConditionWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi'>",
+                "  <Fragment>",
+                "    <Component Id='Comp1' Directory='ApplicationFolder'>",
+                "      <Condition><!-- Comment -->1&lt;2</Condition>",
                 "    </Component>",
                 "  </Fragment>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
+                "  <Fragment>",
+                "    <!-- Comment -->",
+                "    <Component Id=\"Comp1\" Directory=\"ApplicationFolder\" Condition=\"1&lt;2\" />",
+                "  </Fragment>",
                 "</Wix>"
             };
 
@@ -175,27 +297,24 @@ namespace WixToolsetTest.Converters
             Assert.Equal(4, errors);
         }
 
-        [Fact(Skip = "Test demonstrates failure")]
+        [Fact]
         public void FixConditionWithComment()
         {
             var parse = String.Join(Environment.NewLine,
                 "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
                 "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi'>",
                 "  <Fragment>",
-                "    <Condition Message='commented cdata'>",
-                "        <!-- commented condition -->",
-                "        <![CDATA[cdatacontent]]>",
-                "    </Condition>",
+                "    <Condition Message='commented cdata'><!-- commented conditional -->",
+                "<![CDATA[cdatacontent]]></Condition>",
                 "  </Fragment>",
                 "</Wix>");
 
-            //TODO: expected value is not set in stone but must have replaced Condition element with Launch and should have kept the comment.
             var expected = new[]
             {
                 "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
                 "  <Fragment>",
+                "    <!-- commented conditional -->",
                 "    <Launch Condition=\"cdatacontent\" Message=\"commented cdata\" />",
-                "        <!-- commented condition -->",
                 "  </Fragment>",
                 "</Wix>"
             };
@@ -210,7 +329,7 @@ namespace WixToolsetTest.Converters
             var actualLines = UnformattedDocumentLines(document);
             WixAssert.CompareLineByLine(expected, actualLines);
 
-            Assert.Equal(2, errors);
+            Assert.Equal(3, errors);
         }
 
         [Fact]
@@ -285,6 +404,43 @@ namespace WixToolsetTest.Converters
         }
 
         [Fact]
+        public void FixFeatureConditionWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi'>",
+                "  <Fragment>",
+                "    <Feature Id='Feature1'>",
+                "      <Condition Level='0'><!-- Comment -->PROP = 1</Condition>",
+                "    </Feature>",
+                "  </Fragment>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
+                "  <Fragment>",
+                "    <Feature Id=\"Feature1\">",
+                "      <!-- Comment -->",
+                "      <Level Value=\"0\" Condition=\"PROP = 1\" />",
+                "    </Feature>",
+                "  </Fragment>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(3, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
+
+        [Fact]
         public void FixLaunchCondition()
         {
             var parse = String.Join(Environment.NewLine,
@@ -323,6 +479,46 @@ namespace WixToolsetTest.Converters
         }
 
         [Fact]
+        public void FixLaunchConditionWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi'>",
+                "  <Fragment>",
+                "    <Condition Message='Stop the install'>",
+                "      <!-- Comment 1 -->1&lt;2",
+                "    </Condition>",
+                "    <Condition Message='Do not stop'>",
+                "      <!-- Comment 2 -->1=2",
+                "    </Condition>",
+                "  </Fragment>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
+                "  <Fragment>",
+                "    <!-- Comment 1 -->",
+                "    <Launch Condition=\"1&lt;2\" Message=\"Stop the install\" />",
+                "    <!-- Comment 2 -->",
+                "    <Launch Condition=\"1=2\" Message=\"Do not stop\" />",
+                "  </Fragment>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(4, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
+
+        [Fact]
         public void FixLaunchConditionInProduct()
         {
             var parse = String.Join(Environment.NewLine,
@@ -363,6 +559,48 @@ namespace WixToolsetTest.Converters
         }
 
         [Fact]
+        public void FixLaunchConditionInProductWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi'>",
+                "  <Product>",
+                "  <Package />",
+                "    <Condition Message='Stop the install'>",
+                "      <!-- Comment 1 -->1&lt;2",
+                "    </Condition>",
+                "    <Condition Message='Do not stop'>",
+                "      <!-- Comment 2 -->1=2",
+                "    </Condition>",
+                "  </Product>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
+                "  <Package Compressed=\"no\">",
+                "  ",
+                "    <!-- Comment 1 -->",
+                "    <Launch Condition=\"1&lt;2\" Message=\"Stop the install\" />",
+                "    <!-- Comment 2 -->",
+                "    <Launch Condition=\"1=2\" Message=\"Do not stop\" />",
+                "  </Package>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(6, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
+
+        [Fact]
         public void FixPermissionExCondition()
         {
             var parse = String.Join(Environment.NewLine,
@@ -383,11 +621,49 @@ namespace WixToolsetTest.Converters
                 "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
                 "  <Fragment>",
                 "    <Component Id=\"Comp1\" Directory=\"ApplicationFolder\">",
-                "      <PermissionEx Sddl=\"sddl\" Condition=\"1&lt;2\">",
-                "        ",
+                "      <PermissionEx Sddl=\"sddl\" Condition=\"1&lt;2\" />",
+                "    </Component>",
+                "  </Fragment>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(3, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
+
+        [Fact]
+        public void FixPermissionExConditionWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<!-- Comment 1 -->",
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi'>",
+                "  <Fragment>",
+                "    <Component Id='Comp1' Guid='*' Directory='ApplicationFolder'>",
+                "      <PermissionEx Sddl='sddl'>",
+                "        <Condition><!-- Comment 2 -->1&lt;2</Condition>",
                 "      </PermissionEx>",
                 "    </Component>",
                 "  </Fragment>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<!-- Comment 1 -->",
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
+                "  <Fragment>",
+                "    <Component Id=\"Comp1\" Directory=\"ApplicationFolder\">",
+                "      <!-- Comment 2 -->",
+                "      <PermissionEx Sddl=\"sddl\" Condition=\"1&lt;2\" />",
+                "    </Component>",
+                "  </Fragment>",
                 "</Wix>"
             };
 
diff --git a/src/wix/test/WixToolsetTest.Converters/UtilExtensionFixture.cs b/src/wix/test/WixToolsetTest.Converters/UtilExtensionFixture.cs
index 10450c68..0f23d87b 100644
--- a/src/wix/test/WixToolsetTest.Converters/UtilExtensionFixture.cs
+++ b/src/wix/test/WixToolsetTest.Converters/UtilExtensionFixture.cs
@@ -45,6 +45,40 @@ namespace WixToolsetTest.Converters
         }
 
         [Fact]
+        public void FixCloseAppsConditionWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi' xmlns:util='http://schemas.microsoft.com/wix/UtilExtension'>",
+                "  <Fragment>",
+                "    <util:CloseApplication Id='EndApp' Target='example.exe'>",
+                "      <!-- Comment -->a&lt;>b",
+                "    </util:CloseApplication>",
+                "  </Fragment>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\" xmlns:util=\"http://wixtoolset.org/schemas/v4/wxs/util\">",
+                "  <Fragment>",
+                "    <!-- Comment -->",
+                "    <util:CloseApplication Id=\"EndApp\" Target=\"example.exe\" Condition=\"a&lt;&gt;b\" />",
+                "  </Fragment>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(3, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
+
+        [Fact]
         public void FixXmlConfigValue()
         {
             var parse = String.Join(Environment.NewLine,
@@ -78,6 +112,40 @@ namespace WixToolsetTest.Converters
         }
 
         [Fact]
+        public void FixXmlConfigValueWithComment()
+        {
+            var parse = String.Join(Environment.NewLine,
+                "<Wix xmlns='http://schemas.microsoft.com/wix/2006/wi' xmlns:util='http://schemas.microsoft.com/wix/UtilExtension'>",
+                "  <Fragment>",
+                "    <util:XmlConfig Id='Change' ElementPath='book'>",
+                "      <!-- Comment -->a&lt;>b",
+                "    </util:XmlConfig>",
+                "  </Fragment>",
+                "</Wix>");
+
+            var expected = new[]
+            {
+                "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\" xmlns:util=\"http://wixtoolset.org/schemas/v4/wxs/util\">",
+                "  <Fragment>",
+                "    <!-- Comment -->",
+                "    <util:XmlConfig Id=\"Change\" ElementPath=\"book\" Value=\"a&lt;&gt;b\" />",
+                "  </Fragment>",
+                "</Wix>"
+            };
+
+            var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
+
+            var messaging = new MockMessaging();
+            var converter = new WixConverter(messaging, 2, null, null);
+
+            var errors = converter.ConvertDocument(document);
+            Assert.Equal(3, errors);
+
+            var actualLines = UnformattedDocumentLines(document);
+            WixAssert.CompareLineByLine(expected, actualLines);
+        }
+
+        [Fact]
         public void WarnsOnAllRegistryValueSearches()
         {
             var parse = String.Join(Environment.NewLine,
@@ -112,7 +180,6 @@ namespace WixToolsetTest.Converters
             WixAssert.CompareLineByLine(expected, actualLines);
         }
 
-
         [Fact]
         public void FixXmlConfigValueCData()
         {