Add HTML Block rewriting #1146

rynowak · 2018-07-18T00:37:21Z

/cc @SteveSandersonMS - handing this off for you to do the runtime piece.

Rewrites HTML-only subtrees of the IR in an HTML 'block' node with simple string content.

This will rely on new runtime/rendertree support in order to work E2E.

rynowak · 2018-07-18T00:45:19Z

src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeBuilder.cs

+        /// <param name="textContent">Content for the new markup text frame.</param>
+        public void AddMarkupContent(int sequence, string textContent) =>
+            // TODO stevesa implement me
+            throw null;


rynowak · 2018-07-18T00:54:42Z

FYI the test failures are of course expected right now. The rendering tests and E2E tests throw because the RenderTreeBuilder throws.

If you rename the new method on RenderTreeBuilder you'll have to regen the baselines for the runtime codegen tests.

SteveSandersonMS · 2018-07-20T13:20:36Z

Thanks @rynowak! This looks perfect.

I've rebased on master (hence the changes to the commit history here now) and will proceed with the runtime bits.

SteveSandersonMS · 2018-07-20T16:19:19Z

Still to do:

Support UpdateMarkup edit frames in browser-side code
Stop assuming that markup frames content will represent exactly 1 HTML element. Even though our Razor compiler does produce markup frames for each top-level HTML element, that wouldn't be the case if we use this mechanism for IHtmlString.

SteveSandersonMS · 2018-07-22T22:03:39Z

OK all done now.

@rynowak We should discuss the existence of the new MarkupString type (and why I didn't use HtmlString or IHtmlContent from the HTML abstractions package), but other than that it should be ready to go.

rynowak · 2018-07-22T23:44:17Z

src/Microsoft.AspNetCore.Blazor/MarkupString.cs

+        /// <param name="value">The value for the new instance.</param>
+        public MarkupString(string value)
+        {
+            _value = value ?? string.Empty;


Why this? Is it ever meaningful to have null markup string?

For instance the following is true. ((MarkupString)null).ToString() == ""

Good point.

I've now changed it so that MarkupString can hold the value null.

If you .ToString() such an instance, you get string.Empty just like HtmlAbstractions' HtmlString.

If you renderTreeBuilder.AddContent such an instance, it creates a markup frame whose value is string.Empty, just like if you AddContent((string)null).

So the net effect is that nulls continue to be treated like string.Empty, but now it's possible to observe that null was stored in the MarkupString to avoid losing that info.

rynowak · 2018-07-22T23:47:12Z

src/Microsoft.AspNetCore.Blazor/RenderTree/RenderTreeBuilder.cs

+            if (markupContent == null)
+            {
+                throw new ArgumentNullException(nameof(markupContent));
+            }


Should we check non-null here? new MarkupContent(string) is happy to accept null. IMO we should permit both or neither

Good point. It now just null-coalesces with string.Empty like we do for regular string values.

rynowak · 2018-07-22T23:51:04Z

test/Microsoft.AspNetCore.Blazor.Build.Test/RenderingRazorIntegrationTest.cs

+            // The JavaScript-side rendering code does not rely on this behavior. It supports
+            // inserting markup frames with arbitrary markup (e.g., multiple top-level elements
+            // or none). This test exists only as an observation of the current behavior rather
+            // than a promise that we never want to change it.


Do you think it's desirable that we change this in the compiler? It would be straightforward to do.

Perf-wise yes, the ideal would be for adjacent literal markup frames (and any adjacent literal text frames) to be coalesced.

If you think this is a simple addition that would be neat, but I think it’s also a good improvement as-is.

Note that we shouldn’t treat text literals as markup unless they are coalesced into a markup frame, because text nodes on their own are much cheaper to render than markup frames.

Moved this to #1170

rynowak · 2018-07-22T23:52:30Z

We should discuss the existence of the new MarkupString type (and why I didn't use HtmlString or IHtmlContent from the HTML abstractions package), but other than that it should be ready to go.

My assumption is that you wanted to avoid the allocation and the extra references. In particular I think WebEncoders is pretty heavy-weight - this is why I didn't suggest it from the beginning.

rynowak · 2018-07-22T23:52:51Z

from me. I'm not allowed to make it approved since I created the PR.

SteveSandersonMS · 2018-07-23T16:36:35Z

My assumption is that you wanted to avoid the allocation and the extra references. In particular I think WebEncoders is pretty heavy-weight - this is why I didn't suggest it from the beginning.

Yes, avoiding unnecessary allocations was one reason, the other was that simply referencing Microsoft.AspNetCore.Html.Abstractions suddenly exposes a bunch of new APIs that aren't really applicable. Finally the name HtmlString implies the markup has to be HTML, which is not the case for this feature - SVG is also supported (though that is a minor quibble).

…er tree.

… markup dynamically.

…eding custom RenderFragment code

* Add HTML Block rewriter * Baseline updates * test gaps * Update some unit tests to represent same behavior as before * Define Markup frame type. Tests for rendering markup frames into render tree. * Support markup frames during diffing (retain, insert, update, delete) * Support markup blocks on WebAssembly * Support rendering markup frames with server-side execution too * Support markup blocks with multiple top-level nodes. Support updating markup dynamically. * Define MarkupString type as a way to insert dynamic markup without needing custom RenderFragment code * Remove comment * CR: Better null value handling

rynowak requested a review from SteveSandersonMS July 18, 2018 00:37

rynowak commented Jul 18, 2018

View reviewed changes

SteveSandersonMS force-pushed the rynowak/html-block branch from 9d21e23 to 61baf31 Compare July 20, 2018 13:19

rynowak commented Jul 22, 2018

View reviewed changes

SteveSandersonMS added this to the 0.5.0 milestone Jul 23, 2018

rynowak and others added 12 commits July 23, 2018 10:03

Add HTML Block rewriter

921bcf7

Baseline updates

6531d80

test gaps

d09884d

Update some unit tests to represent same behavior as before

f459dfe

Define Markup frame type. Tests for rendering markup frames into rend…

32fc23c

…er tree.

Support markup frames during diffing (retain, insert, update, delete)

f9c2380

Support markup blocks on WebAssembly

015c65e

Support rendering markup frames with server-side execution too

636358b

Support markup blocks with multiple top-level nodes. Support updating…

34d0cf1

… markup dynamically.

Define MarkupString type as a way to insert dynamic markup without ne…

75fdfa9

…eding custom RenderFragment code

Remove comment

9013080

CR: Better null value handling

19bb950

SteveSandersonMS force-pushed the rynowak/html-block branch from 2f49cc4 to 19bb950 Compare July 23, 2018 17:04

SteveSandersonMS merged commit 3c4f7b5 into master Jul 23, 2018

SteveSandersonMS deleted the rynowak/html-block branch July 23, 2018 17:18

SteveSandersonMS added the enhancement label Jul 25, 2018

SteveSandersonMS added the 3 - Done label Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTML Block rewriting #1146

Add HTML Block rewriting #1146

rynowak commented Jul 18, 2018

rynowak Jul 18, 2018

rynowak commented Jul 18, 2018 •

edited

Loading

SteveSandersonMS commented Jul 20, 2018

SteveSandersonMS commented Jul 20, 2018 •

edited

Loading

SteveSandersonMS commented Jul 22, 2018

rynowak Jul 22, 2018

SteveSandersonMS Jul 23, 2018

rynowak Jul 22, 2018

SteveSandersonMS Jul 23, 2018

rynowak Jul 22, 2018

SteveSandersonMS Jul 22, 2018

SteveSandersonMS Jul 23, 2018

rynowak commented Jul 22, 2018

rynowak commented Jul 22, 2018

SteveSandersonMS commented Jul 23, 2018 •

edited

Loading

Add HTML Block rewriting #1146

Add HTML Block rewriting #1146

Conversation

rynowak commented Jul 18, 2018

Choose a reason for hiding this comment

rynowak commented Jul 18, 2018 • edited Loading

SteveSandersonMS commented Jul 20, 2018

SteveSandersonMS commented Jul 20, 2018 • edited Loading

SteveSandersonMS commented Jul 22, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Jul 22, 2018

rynowak commented Jul 22, 2018

SteveSandersonMS commented Jul 23, 2018 • edited Loading

rynowak commented Jul 18, 2018 •

edited

Loading

SteveSandersonMS commented Jul 20, 2018 •

edited

Loading

SteveSandersonMS commented Jul 23, 2018 •

edited

Loading