Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring virtualization spacer element type #42449

Closed
SteveSandersonMS opened this issue Jun 27, 2022 · 2 comments · Fixed by #42446
Closed

Allow configuring virtualization spacer element type #42449

SteveSandersonMS opened this issue Jun 27, 2022 · 2 comments · Fixed by #42446
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components feature-prerendering Issues related to prerendering blazor components
Milestone

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jun 27, 2022

Background and Motivation

Blazor's <Virtualize> component produces a DOM structure like this:

<div style="height: 0px"></div>
... user-rendered element 1 ...
... user-rendered element 2 ...
...
... user-rendered element N ...
<div style="height: 1234px"></div>

The top and bottom elements there are called "spacers" and their job is to:

  1. Have an explicit height to cause the scroll range to be correct and represent the whole virtualized UI
  2. Detect when scrolling is bringing them close to the visible viewport so we know to change the set of rendered elements

The problem is that it's currently hardcoded to use div elements for these spacers. While pretty general-purpose, there are some cases where div is not technically allowed, such as directly inside a tbody.

In the past we decided it works well enough anyway, since the browser doesn't really care if you put a div inside a tbody via JavaScript. However this does cause a problem if you do it in the initial HTML page, such as when prerendering. The browser's HTML parser will think hey that's not right, let me fix that for you and gives you a DOM structure like this:

<div style="height: 0px"></div>
<div style="height: 12345px"></div>
<table>
   <tbody>
        ... user-rendered element 1 ...
        ... user-rendered element 2 ...
        ...
        ... user-rendered element N ...
   </tbody>
<table>

... i.e., it moves the spacers outside the table. This doesn't cause much of a problem because as soon as Blazor starts up interactively, it rebuilds the DOM anyway and puts things in the right places. But during the time between prerendering and interactivity, you may see a flash of malformed content.

Proposed API

public class Virtualize<TItem>
{
+    /// <summary>
+    /// Gets or sets the tag name of the HTML element that will be used as the virtualization spacer.
+    /// One such element will be rendered before the visible items, and one more after them, using
+    /// an explicit "height" style to control the scroll range.
+    ///
+    /// The default value is "div". If you are placing the <see cref="Virtualize{TItem}"/> instance inside
+    /// an element that requires a specific child tag name, consider setting that here. For example when
+    /// rendering inside a "tbody", consider setting <see cref="SpacerElement"/> to the value "tr".
+    /// </summary>
+    [Parameter]
+    public string SpacerElement { get; set; } = "div";
}

Usage Examples

<Virtualize SpacerElement="tr">...</Virtualize>

Alternative Designs

We could avoid having this be configurable, and instead JS logic that looks at the actual parent element at runtime and decides whether to use a div, a tr, or whatever. That's nice but the main problem is it would be a breaking change. I know some people already are using CSS to target the spacer element to do particular things with it like set a background color in case it becomes visible during fast scrolling.

Update: No, that alternative is total nonsense, even ignoring the breaking change aspect. I just realised it wouldn't work in prerendering (as there's no JS running) and the whole point of this fix is to work with prerendering. It has to be done in .NET code. I'm not aware of a sensible alternative design.

Risks

Nothing significant. If people do something nonsensical such as entering an illegal tag name value (e.g., an empty string), the browser will already give them a sensible message at runtime when Blazor tries to use it when constructing an element.

@SteveSandersonMS SteveSandersonMS added api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 27, 2022
@ghost
Copy link

ghost commented Jun 27, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@BrennanConroy
Copy link
Member

API review:

public class Virtualize<TItem>
{
+    [Parameter]
+    public string SpacerElement { get; set; } = "div";
}

API approved!

@BrennanConroy BrennanConroy added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 27, 2022
@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label Jun 28, 2022
@TanayParikh TanayParikh added this to the 7.0-preview7 milestone Jun 28, 2022
@TanayParikh TanayParikh added the feature-prerendering Issues related to prerendering blazor components label Jun 28, 2022
@mkArtakMSFT mkArtakMSFT linked a pull request Jul 21, 2022 that will close this issue
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components feature-prerendering Issues related to prerendering blazor components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants