[Design] React to aspnet/Razor#653

- aspnet/Razor#643 part 2
- add `HtmlEncoder` parameter to `RazorPage.StartTagHelperWritingScope()`
- note `HtmlEncoder`s used elsewhere e.g. in other `RazorPage` instances are unaffected

Also simplify scope management, removing bizarre assertion when user does something odd.
- see code comments to reviewers about other options here.

Loads of test fallout though this is a WIP: Not yet testing out the new feature.

src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs

@@ -30,8 +30,7 @@ namespace Microsoft.AspNet.Mvc.Razor
     public abstract class RazorPage : IRazorPage
     {
         private readonly HashSet<string> _renderedSections = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
-        private readonly Stack<HtmlContentWrapperTextWriter> _writerScopes;
-        private TextWriter _originalWriter;
+        private readonly Stack<TagHelperScopeInfo> _tagHelperScopes = new Stack<TagHelperScopeInfo>();
         private IUrlHelper _urlHelper;
         private ITagHelperActivator _tagHelperActivator;
         private ITypeActivatorCache _typeActivatorCache;
@@ -44,7 +43,6 @@ public abstract class RazorPage : IRazorPage
         public RazorPage()
         {
             SectionWriters = new Dictionary<string, RenderAsyncDelegate>(StringComparer.OrdinalIgnoreCase);
-            _writerScopes = new Stack<HtmlContentWrapperTextWriter>();
         }
 
         /// <summary>
@@ -62,7 +60,8 @@ public RazorPage()
         public string Layout { get; set; }
 
         /// <summary>
-        /// Gets the <see cref="HtmlEncoder"/> to be used for encoding HTML.
+        /// Gets the <see cref="System.Text.Encodings.Web.HtmlEncoder"/> to be used for encoding HTML. For example,
+        /// used when encoding C# variables referenced on the page.
         /// </summary>
         [RazorInject]
         public HtmlEncoder HtmlEncoder { get; set; }
@@ -116,7 +115,7 @@ public virtual TextWriter Output
         public IDictionary<string, RenderAsyncDelegate> PreviousSectionWriters { get; set; }
 
         /// <inheritdoc />
-        public IDictionary<string, RenderAsyncDelegate> SectionWriters { get; private set; }
+        public IDictionary<string, RenderAsyncDelegate> SectionWriters { get; }
 
         /// <inheritdoc />
         public abstract Task ExecuteAsync();
@@ -201,28 +200,31 @@ public TTagHelper CreateTagHelper<TTagHelper>() where TTagHelper : ITagHelper
         }
 
         /// <summary>
-        /// Starts a new writing scope.
+        /// Starts a new writing scope and overrides <see cref="HtmlEncoder"/> within that scope.
         /// </summary>
+        /// <param name="encoder">
+        /// The <see cref="System.Text.Encodings.Web.HtmlEncoder"/> to use. Does not override all HTML encodings which
+        /// may occur.
+        /// </param>
         /// <remarks>
         /// All writes to the <see cref="Output"/> or <see cref="ViewContext.Writer"/> after calling this method will
         /// be buffered until <see cref="EndTagHelperWritingScope"/> is called.
         /// </remarks>
-        public void StartTagHelperWritingScope()
+        public void StartTagHelperWritingScope(HtmlEncoder encoder)
         {
-            // If there isn't a base writer take the ViewContext.Writer
-            if (_originalWriter == null)
+            _tagHelperScopes.Push(new TagHelperScopeInfo(HtmlEncoder, ViewContext.Writer));
+
+            // If passed an HtmlEncoder, override the property.
+            if (encoder != null)
             {
-                _originalWriter = ViewContext.Writer;
+                HtmlEncoder = encoder;
             }
 
-            var buffer = new ViewBuffer(BufferScope, Path);
-            var writer = new HtmlContentWrapperTextWriter(buffer, _originalWriter.Encoding);
-
             // We need to replace the ViewContext's Writer to ensure that all content (including content written
             // from HTML helpers) is redirected.
+            var buffer = new ViewBuffer(BufferScope, Path);
+            var writer = new HtmlContentWrapperTextWriter(buffer, ViewContext.Writer.Encoding);
             ViewContext.Writer = writer;
-
-            _writerScopes.Push(writer);
         }
 
         /// <summary>
@@ -231,28 +233,27 @@ public void StartTagHelperWritingScope()
         /// <returns>The buffered <see cref="TagHelperContent"/>.</returns>
         public TagHelperContent EndTagHelperWritingScope()
         {
-            if (_writerScopes.Count == 0)
+            if (_tagHelperScopes.Count == 0)
             {
                 throw new InvalidOperationException(Resources.RazorPage_ThereIsNoActiveWritingScopeToEnd);
             }
 
-            var writer = _writerScopes.Pop();
-            Debug.Assert(writer == ViewContext.Writer);
+            // Reviewers: Debug.Assert meant we weren't supporting user changes to ViewContext.Writer in any case.
+            // Asserting about a user behavior also just doesn't make sense. I chose to silently ignore that case
+            // instead. Other reasonable options are to (a) throw if ViewContext.Writer is not of expected type or
+            // (b) go back to pushing changed values in Start..., also throw above when Count is 1, Pop and throw if
+            // values are unexpected, and Peek() normally but Pop() in the restoration code below when Count was 2.
 
-            if (_writerScopes.Count > 0)
-            {
-                ViewContext.Writer = _writerScopes.Peek();
-            }
-            else
-            {
-                ViewContext.Writer = _originalWriter;
+            // Get the content written during the current scope.
+            var writer = ViewContext.Writer as HtmlContentWrapperTextWriter;
+            var tagHelperContent = new DefaultTagHelperContent();
+            tagHelperContent.AppendHtml(writer?.ContentBuilder);
 
-                // No longer a base writer
-                _originalWriter = null;
-            }
+            // Restore previous scope.
+            var scopeInfo = _tagHelperScopes.Pop();
+            HtmlEncoder = scopeInfo.Encoder;
+            ViewContext.Writer = scopeInfo.Writer;
 
-            var tagHelperContent = new DefaultTagHelperContent();
-            tagHelperContent.AppendHtml(writer.ContentBuilder);
             return tagHelperContent;
         }
 
@@ -290,7 +291,9 @@ public virtual void WriteTo(TextWriter writer, object value)
         /// Writes the specified <paramref name="value"/> with HTML encoding to given <paramref name="writer"/>.
         /// </summary>
         /// <param name="writer">The <see cref="TextWriter"/> instance to write to.</param>
-        /// <param name="encoder">The <see cref="HtmlEncoder"/> to use when encoding <paramref name="value"/>.</param>
+        /// <param name="encoder">
+        /// The <see cref="System.Text.Encodings.Web.HtmlEncoder"/> to use when encoding <paramref name="value"/>.
+        /// </param>
         /// <param name="value">The <see cref="object"/> to write.</param>
         /// <remarks>
         /// <paramref name="value"/>s of type <see cref="IHtmlContent"/> are written using
@@ -825,9 +828,8 @@ private async Task<HtmlString> RenderSectionAsyncCore(string sectionName, bool r
         /// </remarks>
         public async Task<HtmlString> FlushAsync()
         {
-            // If there are active writing scopes then we should throw. Cannot flush content that has the potential to
-            // change.
-            if (_writerScopes.Count > 0)
+            // If there are active scopes, then we should throw. Cannot flush content that has the potential to change.
+            if (_tagHelperScopes.Count > 0)
             {
                 throw new InvalidOperationException(
                     Resources.FormatRazorPage_CannotFlushWhileInAWritingScope(nameof(FlushAsync), Path));
@@ -999,5 +1001,18 @@ public TagHelperAttributeInfo(
 
             public bool Suppressed { get; set; }
         }
+
+        private struct TagHelperScopeInfo
+        {
+            public TagHelperScopeInfo(HtmlEncoder encoder, TextWriter writer)
+            {
+                Encoder = encoder;
+                Writer = writer;
+            }
+
+            public HtmlEncoder Encoder { get; }
+
+            public TextWriter Writer { get; }
+        }
     }
 }

test/Microsoft.AspNet.Mvc.Razor.Test/RazorPageTest.cs

@@ -41,7 +41,7 @@ public async Task WritingScopesRedirectContentWrittenToViewContextWriter()
             {
                 v.HtmlEncoder = new HtmlTestEncoder();
                 v.Write("Hello Prefix");
-                v.StartTagHelperWritingScope();
+                v.StartTagHelperWritingScope(encoder: null);
                 v.Write("Hello from Output");
                 v.ViewContext.Writer.Write("Hello from view context writer");
                 var scopeValue = v.EndTagHelperWritingScope();
@@ -67,7 +67,7 @@ public async Task WritingScopesRedirectsContentWrittenToOutput()
             {
                 v.HtmlEncoder = new HtmlTestEncoder();
                 v.Write("Hello Prefix");
-                v.StartTagHelperWritingScope();
+                v.StartTagHelperWritingScope(encoder: null);
                 v.Write("Hello In Scope");
                 var scopeValue = v.EndTagHelperWritingScope();
                 v.Write("From Scope: ");
@@ -91,10 +91,10 @@ public async Task WritingScopesCanNest()
             {
                 v.HtmlEncoder = new HtmlTestEncoder();
                 v.Write("Hello Prefix");
-                v.StartTagHelperWritingScope();
+                v.StartTagHelperWritingScope(encoder: null);
                 v.Write("Hello In Scope Pre Nest");
 
-                v.StartTagHelperWritingScope();
+                v.StartTagHelperWritingScope(encoder: null);
                 v.Write("Hello In Nested Scope");
                 var scopeValue1 = v.EndTagHelperWritingScope();
 
@@ -123,7 +123,7 @@ public async Task StartNewWritingScope_CannotFlushInWritingScope()
             var page = CreatePage(async v =>
             {
                 v.Path = "/Views/TestPath/Test.cshtml";
-                v.StartTagHelperWritingScope();
+                v.StartTagHelperWritingScope(encoder: null);
                 await v.FlushAsync();
             });
 
@@ -164,7 +164,7 @@ public async Task EndTagHelperWritingScope_ReturnsAppropriateContent()
             var page = CreatePage(v =>
             {
                 v.HtmlEncoder = new HtmlTestEncoder();
-                v.StartTagHelperWritingScope();
+                v.StartTagHelperWritingScope(encoder: null);
                 v.Write("Hello World!");
                 var returnValue = v.EndTagHelperWritingScope();
 
@@ -923,7 +923,7 @@ public void AddHtmlAttributeValues_AddsToHtmlAttributesAsExpected(
                 items: new Dictionary<object, object>(),
                 uniqueId: string.Empty,
                 executeChildContentAsync: () => Task.FromResult(result: true),
-                startTagHelperWritingScope: () => { },
+                startTagHelperWritingScope: _ => { },
                 endTagHelperWritingScope: () => new DefaultTagHelperContent());
 
             // Act
@@ -964,7 +964,7 @@ public void AddHtmlAttributeValues_OnlyAddsToAllAttributesWhenAttributeRemoved(
                 items: new Dictionary<object, object>(),
                 uniqueId: string.Empty,
                 executeChildContentAsync: () => Task.FromResult(result: true),
-                startTagHelperWritingScope: () => { },
+                startTagHelperWritingScope: _ => { },
                 endTagHelperWritingScope: () => new DefaultTagHelperContent());
 
             // Act
@@ -992,7 +992,7 @@ public void AddHtmlAttributeValues_AddsAttributeNameAsValueWhenValueIsUnprefixed
                 items: new Dictionary<object, object>(),
                 uniqueId: string.Empty,
                 executeChildContentAsync: () => Task.FromResult(result: true),
-                startTagHelperWritingScope: () => { },
+                startTagHelperWritingScope: _ => { },
                 endTagHelperWritingScope: () => new DefaultTagHelperContent());
 
             // Act

test/Microsoft.AspNet.Mvc.Razor.Test/TagHelpers/UrlResolutionTagHelperTest.cs

@@ -47,7 +47,7 @@ public void Process_ResolvesTildeSlashValues(string url, string expectedHref)
                 {
                     { "href", url }
                 },
-                getChildContentAsync: _ => Task.FromResult<TagHelperContent>(null));
+                getChildContentAsync: (useCachedResult, encoder) => Task.FromResult<TagHelperContent>(null));
             var urlHelperMock = new Mock<IUrlHelper>();
             urlHelperMock
                 .Setup(urlHelper => urlHelper.Content(It.IsAny<string>()))
@@ -103,7 +103,7 @@ public void Process_ResolvesTildeSlashValues_InHtmlString(object url, string exp
                 {
                     { "href", url }
                 },
-                getChildContentAsync: _ => Task.FromResult<TagHelperContent>(null));
+                getChildContentAsync: (useCachedResult, encoder) => Task.FromResult<TagHelperContent>(null));
             var urlHelperMock = new Mock<IUrlHelper>();
             urlHelperMock
                 .Setup(urlHelper => urlHelper.Content(It.IsAny<string>()))
@@ -158,7 +158,7 @@ public void Process_DoesNotResolveNonTildeSlashValues(string url)
                 {
                     { "href", url }
                 },
-                getChildContentAsync: _ => Task.FromResult<TagHelperContent>(null));
+                getChildContentAsync: (useCachedResult, encoder) => Task.FromResult<TagHelperContent>(null));
             var urlHelperMock = new Mock<IUrlHelper>();
             urlHelperMock
                 .Setup(urlHelper => urlHelper.Content(It.IsAny<string>()))
@@ -213,7 +213,7 @@ public void Process_DoesNotResolveNonTildeSlashValues_InHtmlString(HtmlString ur
                 {
                     { "href", url }
                 },
-                getChildContentAsync: _ => Task.FromResult<TagHelperContent>(null));
+                getChildContentAsync: (useCachedResult, encoder) => Task.FromResult<TagHelperContent>(null));
             var urlHelperMock = new Mock<IUrlHelper>();
             urlHelperMock
                 .Setup(urlHelper => urlHelper.Content(It.IsAny<string>()))
@@ -251,7 +251,7 @@ public void Process_IgnoresNonHtmlStringOrStringValues()
                 {
                     { "href", true }
                 },
-                getChildContentAsync: _ => Task.FromResult<TagHelperContent>(null));
+                getChildContentAsync: (useCachedResult, encoder) => Task.FromResult<TagHelperContent>(null));
             var tagHelper = new UrlResolutionTagHelper(urlHelperFactory: null, htmlEncoder: null);
 
             var context = new TagHelperContext(
@@ -288,7 +288,7 @@ public void Process_ThrowsWhenEncodingNeededAndIUrlHelperActsUnexpectedly()
                 {
                     { "href", new HtmlString(relativeUrl) }
                 },
-                getChildContentAsync: _ => Task.FromResult<TagHelperContent>(null));
+                getChildContentAsync: (useCachedResult, encoder) => Task.FromResult<TagHelperContent>(null));
             var urlHelperMock = new Mock<IUrlHelper>();
             urlHelperMock
                 .Setup(urlHelper => urlHelper.Content(It.IsAny<string>()))

test/Microsoft.AspNet.Mvc.TagHelpers.Test/AnchorTagHelperTest.cs

@@ -44,7 +44,7 @@ public async Task ProcessAsync_GeneratesExpectedOutput()
                 {
                     { "id", "myanchor" },
                 },
-                getChildContentAsync: useCachedResult =>
+                getChildContentAsync: (useCachedResult, encoder) =>
                 {
                     var tagHelperContent = new DefaultTagHelperContent();
                     tagHelperContent.SetContent("Something Else");
@@ -99,7 +99,7 @@ public async Task ProcessAsync_CallsIntoRouteLinkWithExpectedParameters()
             var output = new TagHelperOutput(
                 "a",
                 attributes: new TagHelperAttributeList(),
-                getChildContentAsync: useCachedResult =>
+                getChildContentAsync: (useCachedResult, encoder) =>
                 {
                     var tagHelperContent = new DefaultTagHelperContent();
                     tagHelperContent.SetContent("Something");
@@ -148,7 +148,7 @@ public async Task ProcessAsync_CallsIntoActionLinkWithExpectedParameters()
             var output = new TagHelperOutput(
                 "a",
                 attributes: new TagHelperAttributeList(),
-                getChildContentAsync: useCachedResult =>
+                getChildContentAsync: (useCachedResult, encoder) =>
                 {
                     var tagHelperContent = new DefaultTagHelperContent();
                     tagHelperContent.SetContent("Something");
@@ -209,7 +209,7 @@ public async Task ProcessAsync_ThrowsIfHrefConflictsWithBoundAttributes(string p
                 {
                     { "href", "http://www.contoso.com" }
                 },
-                getChildContentAsync: _ => Task.FromResult<TagHelperContent>(null));
+                getChildContentAsync: (useCachedResult, encoder) => Task.FromResult<TagHelperContent>(null));
             if (propertyName == "asp-route-")
             {
                 anchorTagHelper.RouteValues.Add("name", "value");
@@ -255,7 +255,7 @@ public async Task ProcessAsync_ThrowsIfRouteAndActionOrControllerProvided(string
             var output = new TagHelperOutput(
                 "a",
                 attributes: new TagHelperAttributeList(),
-                getChildContentAsync: _ => Task.FromResult<TagHelperContent>(null));
+                getChildContentAsync: (useCachedResult, encoder) => Task.FromResult<TagHelperContent>(null));
             var expectedErrorMessage = "Cannot determine an 'href' attribute for <a>. An <a> with a specified " +
                 "'asp-route' must not have an 'asp-action' or 'asp-controller' attribute.";
 

test/Microsoft.AspNet.Mvc.TagHelpers.Test/CacheTagHelperTest.cs

@@ -740,7 +740,7 @@ public async Task ProcessAsync_FlowsEntryLinkThatAllowsAddingTriggersToAddedEntr
             var tagHelperOutput = new TagHelperOutput(
                 "cache",
                 new TagHelperAttributeList { { "attr", "value" } },
-                getChildContentAsync: useCachedResult =>
+                getChildContentAsync: (useCachedResult, encoder) =>
                 {
                     TagHelperContent tagHelperContent;
                     if (!cache.TryGetValue("key1", out tagHelperContent))
@@ -806,7 +806,7 @@ private static TagHelperOutput GetTagHelperOutput(
             return new TagHelperOutput(
                 tagName,
                 attributes,
-                getChildContentAsync: useCachedResult =>
+                getChildContentAsync: (useCachedResult, encoder) =>
                 {
                     var tagHelperContent = new DefaultTagHelperContent();
                     tagHelperContent.SetHtmlContent(childContent);

test/Microsoft.AspNet.Mvc.TagHelpers.Test/EnvironmentTagHelperTest.cs

@@ -143,7 +143,7 @@ private TagHelperOutput MakeTagHelperOutput(
             return new TagHelperOutput(
                 tagName,
                 attributes,
-                getChildContentAsync: useCachedResult =>
+                getChildContentAsync: (useCachedResult, encoder) =>
                 {
                     var tagHelperContent = new DefaultTagHelperContent();
                     tagHelperContent.SetContent(childContent);