Prerender select elements with value; move HtmlRenderer into Mvc.ViewFeatures #12996

SteveSandersonMS · 2019-08-09T10:36:01Z

This deals with #12667 in the case where the <option> element isn't embedded into a markup frame.

The remainder of the fix for #12667 would be to change the Razor compiler's markup pass to avoid recursing into <select> elements, as per @rynowak's description. @ajaybhargavb, would it be possible for you to handle the remaining piece inside the Razor compiler? This is because I'm trying to maximise the amount of API-review changes I can get done in my remaining few days on preview 9 - not sure what your backlog currently looks like, @ajaybhargavb.

Further API review thought: if there's any reasonable way we could move HtmlRenderer out of .Components and into .Web, that would be much better. This whole concept is completely web-specific. I understand the limit there is that the MVC prerendering feature depends on this. @javiercn / @rynowak, do you know if anything stops us from making MVC prerendering depend on M.A.Components.Web?

SteveSandersonMS · 2019-08-09T10:37:18Z

src/Components/Components/src/Rendering/HtmlRenderer.cs

                case RenderTreeFrameType.Attribute:
-                    return RenderAttributes(result, frames, position, 1);
+                    throw new InvalidOperationException($"Attributes should only be encountered within {nameof(RenderElement)}");


I think this is true, and all the tests do still pass. @javiercn, do you have any concerns about this?

Do we have a test with more than 1 attribute? I thought we iterated over the attributes that way, but I might be wrong.

The new tests for prerendering <select> put multiple attributes on a single element and work OK.

javiercn · 2019-08-09T10:41:15Z

Components.Web depends on JSInterop, doesn't it? Not sure about how others feels about it. On one side, it's all the shared framework. On the other, we would have to explain customers why MVC has a reference to something they can't use.

SteveSandersonMS · 2019-08-09T11:32:43Z

Components.Web depends on JSInterop, doesn't it?

M.A.Components itself already has a reference to JSInterop (though we do plan to move that out to .Web later). People haven't found it troublesome yet.

On the other, we would have to explain customers why MVC has a reference to something they can't use.

Whether they can use it depends on what parts of ASP.NET Core they have configured for use in their application. We have a lot of things like that, so it doesn't seem too problematic to me, but maybe we can cover this in our discussions about JSInterop.

SteveSandersonMS · 2019-08-09T16:14:59Z

Update We agreed offline that we will move HtmlRenderer into .Web. So I'll add that behavior either here or in a different PR.

ajaybhargavb · 2019-08-09T20:53:38Z

PR with the compiler changes dotnet/razor#952

…erendering

SteveSandersonMS · 2019-08-12T12:56:16Z

OK @javiercn @rynowak:

I've updated this quite a bit and expanded the scope of the PR to untangling HtmlRenderer from the rest of the system:

RemoteRenderer no longer inherits from HtmlRenderer. The fact that it did before was a strange implementation quirk of stateful prerendering which is no longer required.
HtmlRenderer itself moves all the way into Mvc.ViewFeatures and becomes purely internal, so this and ComponentRenderedText disappear from the public API surface entirely

The one new bit of API we have to expose to enable this is changing Renderer's GetCurrentRenderTreeFrames from private protected to protected. I think this is the right accessibility modifier, as it really is necessary for subclasses to access this info for some reasonable scenarios (HTML rendering, Blutter). The fact that it only shows up for people subclassing Renderer means this doesn't change anything for building normal apps.

javiercn · 2019-08-12T13:08:38Z

HtmlRenderer itself moves all the way into Mvc.ViewFeatures and becomes purely internal, so this and ComponentRenderedText disappear from the public API surface entirely

This sounds great. I didn't like the shape of ComponentRenderedText and was planning to change it if we had time. It's better now as we can improve perf without causing breaking changes.

javiercn · 2019-08-12T16:29:52Z

src/Mvc/Mvc.ViewFeatures/src/RazorComponents/HtmlRenderer.cs

@@ -247,6 +259,13 @@ private async Task<(int, ArrayRange<RenderTreeFrame>)> CreateInitialRenderAsync(

            return (componentId, GetCurrentRenderTreeFrames(componentId));
        }
+
+        private class HtmlRenderingContext


It's only one per render, and if we make it a mutable struct we have to be careful about passing by ref everywhere. I'd be happier keeping it as class for simplicity.

javiercn · 2019-08-12T16:40:45Z

src/Mvc/Mvc.ViewFeatures/src/RazorComponents/HtmlRenderer.cs

+                {
+                    // There's no concept of nested <select> elements, so as soon as we're exiting one of them,
+                    // we can safely say there is no longer any value for this
+                    context.ClosestSelectValueAsString = null;


Should we try and detect this case and throw in that scenario? I'm concerned between the difference between prerender and non-prerender here.

I don't think there's any difference between prerender and non-prerender in any valid scenario. It's never valid to nest <select> elements.

javiercn

Great stuff!

I only had a small comment about detecting nested selects, I'll leave it up to you.

rynowak · 2019-08-12T17:26:07Z

src/Components/Components/src/Rendering/Renderer.cs

@@ -93,7 +93,7 @@ protected internal int AssignRootComponentId(IComponent component)
        /// </summary>
        /// <param name="componentId">The id for the component.</param>
        /// <returns>The <see cref="RenderTreeBuilder"/> representing the current render tree.</returns>
-        private protected ArrayRange<RenderTreeFrame> GetCurrentRenderTreeFrames(int componentId) => GetRequiredComponentState(componentId).CurrentRenderTree.GetFrames();
+        protected ArrayRange<RenderTreeFrame> GetCurrentRenderTreeFrames(int componentId) => GetRequiredComponentState(componentId).CurrentRenderTree.GetFrames();


Is the expectation that if you get the current frames you can or can't hold on to them? Does this make a copy?

It doesn't make a copy. The expectation is that you only access them synchronously.

We could rename to “ViewCurrentRenderTreeFrames” to clarify this. Longer term we could use the same technique we plan for ParameterView to throw if you access after the underlying store is mutated further.

ViewCurrentRenderTreeFrames

I'm not actually going to rename it to that, because it's a strange name. The existing name GetCurrentRenderTreeFrames is more conventional. We can add the "don't read after mutation" check in the future if we want.

SteveSandersonMS added area-blazor Includes: Blazor, Razor Components tell-mode Indicates a PR which is being merged during tell-mode labels Aug 9, 2019

SteveSandersonMS added this to the 3.0.0-preview9 milestone Aug 9, 2019

SteveSandersonMS requested review from rynowak, ajaybhargavb and javiercn August 9, 2019 10:36

SteveSandersonMS commented Aug 9, 2019

View reviewed changes

ajaybhargavb mentioned this pull request Aug 9, 2019

Don't coalesce <option> tag in ComponentMarkupBlockPass dotnet/razor#952

Merged

SteveSandersonMS added 2 commits August 12, 2019 11:17

Add context to HtmlRenderer

35d9c40

Auto-add "selected" attribute to matching "option" elements during pr…

3df6664

…erendering

SteveSandersonMS force-pushed the stevesa/fix-prerendering-select branch from 4e1477f to 3df6664 Compare August 12, 2019 10:18

SteveSandersonMS added 5 commits August 12, 2019 12:59

Move HtmlRenderer into .Web

db63b9d

Decouple RemoteRenderer from HtmlRenderer

e7c3a95

Update ref sources

3139a2c

Move HtmlRenderer to Mvc.ViewFeatures

319ae36

Update reference & ref sources

ed6679a

SteveSandersonMS changed the title ~~Prerender select elements with value~~ Prerender select elements with value; move HtmlRenderer into Mvc.ViewFeatures Aug 12, 2019

javiercn reviewed Aug 12, 2019

View reviewed changes

javiercn approved these changes Aug 12, 2019

View reviewed changes

rynowak reviewed Aug 12, 2019

View reviewed changes

SteveSandersonMS merged commit 6b2d9f2 into release/3.0 Aug 13, 2019

ghost deleted the stevesa/fix-prerendering-select branch August 13, 2019 08:34

SteveSandersonMS mentioned this pull request Aug 13, 2019

Server-side blazor : <select> value is bound after being rendered #12667

Closed

SteveSandersonMS mentioned this pull request Aug 22, 2019

Validate prerendering select #13337

Closed

pranavkm mentioned this pull request Sep 12, 2019

Revisit RemoteRenderer deriving from HtmlRenderer #7805

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prerender select elements with value; move HtmlRenderer into Mvc.ViewFeatures #12996

Prerender select elements with value; move HtmlRenderer into Mvc.ViewFeatures #12996

SteveSandersonMS commented Aug 9, 2019

SteveSandersonMS Aug 9, 2019

javiercn Aug 12, 2019

SteveSandersonMS Aug 12, 2019

javiercn commented Aug 9, 2019

SteveSandersonMS commented Aug 9, 2019

SteveSandersonMS commented Aug 9, 2019

ajaybhargavb commented Aug 9, 2019

SteveSandersonMS commented Aug 12, 2019

javiercn commented Aug 12, 2019

javiercn Aug 12, 2019

SteveSandersonMS Aug 12, 2019

javiercn Aug 12, 2019

SteveSandersonMS Aug 12, 2019

javiercn left a comment

rynowak Aug 12, 2019

SteveSandersonMS Aug 12, 2019

SteveSandersonMS Aug 12, 2019

SteveSandersonMS Aug 13, 2019

Prerender select elements with value; move HtmlRenderer into Mvc.ViewFeatures #12996

Prerender select elements with value; move HtmlRenderer into Mvc.ViewFeatures #12996

Conversation

SteveSandersonMS commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn commented Aug 9, 2019

SteveSandersonMS commented Aug 9, 2019

SteveSandersonMS commented Aug 9, 2019

ajaybhargavb commented Aug 9, 2019

SteveSandersonMS commented Aug 12, 2019

javiercn commented Aug 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment