finos / perspective

A data visualization and analytics component, especially well-suited for large and/or streaming datasets.
https://perspective.finos.org/
Apache License 2.0
7.73k stars 1.05k forks source link

Render table in the shadow DOM for `@finos/perspective-viewer-datagrid` #2482

Closed tomjakubowski closed 5 months ago

tomjakubowski commented 6 months ago

Move the <regular-table> element of datagrid into the shadow DOM by default.

This change isolates the regular-table from global styles, so CSS rules on regular-table or table no longer affect datagrid. Style rules on the regular-table can be ported using the part named regular-table

// old style
perspective-viewer-datagrid regular-table {
  font-family: "Comic Sans MS";
}
// shadow dom
perspective-viewer-datagrid::part(regular-table) {
  font-family: "Comic Sans MS";
}

To disable this feature and continue to use light DOM rendering, set:

HTMLPerspectiveViewerDatagridPluginElement.renderTarget = "light"
tomjakubowski commented 6 months ago

I'm ok with the configuration flag being a global static on the perspective-viewer-datagrid element, because there is no global settings feature yet, but one is obviously needed. However, let's please rename this to enableShadowDOM.

Another API I was thinking about was sticking it as an attribute/flag on <perspective-viewer>, like <perspective-viewer shadow />. A problem is I imagine it would be disastrous to update a particular element to use/not use shadow DOM at runtime, i.e. to toggle usage of shadow DOM in response to changes to that attribute.

There needs to be at least a few tests added that test Shadow DOM mode specifically. If it's doable, I'd really like to see all the tests run in parameterized in both modes, but this may be a lot of work (?)

I'll try this, it's a good idea to rerun the same tests with shadow DOM enabled/disabled. And I'll think about a test or two just for shadow DOM

tomjakubowski commented 5 months ago

All the checks are passing now, re-review?

jakehadar commented 2 months ago

I'm ok with the configuration flag being a global static on the perspective-viewer-datagrid element, because there is no global settings feature yet, but one is obviously needed. However, let's please rename this to enableShadowDOM.

Another API I was thinking about was sticking it as an attribute/flag on <perspective-viewer>, like <perspective-viewer shadow />. A problem is I imagine it would be disastrous to update a particular element to use/not use shadow DOM at runtime, i.e. to toggle usage of shadow DOM in response to changes to that attribute.

There needs to be at least a few tests added that test Shadow DOM mode specifically. If it's doable, I'd really like to see all the tests run in parameterized in both modes, but this may be a lot of work (?)

I'll try this, it's a good idea to rerun the same tests with shadow DOM enabled/disabled. And I'll think about a test or two just for shadow DOM

Hi, was this attribute flag ever exposed publicly? Looks like it is a static/private attribute set at runtime based on browser capaibility https://github.com/finos/perspective/blame/84605851230c3dd1cc58652b47babd04cc9fcc31/packages/perspective-viewer-datagrid/src/js/custom_elements/datagrid.js#L141 This introduces inconsistencies between browsers and i believe the shadow rendering policy should be something the consumer should have the option to opt-out of

tomjakubowski commented 1 month ago

@jakehadar The inferred setting is a default. You can override it by using customElements.whenDefined() to access the custom element constructor when it is added to the registry, and then set the renderTarget property on it to whichever you prefer.

customElements.whenDefined("perspective-viewer-datagrid").then((datagrid) => {
  console.log("setting datagrid render target");
  datagrid.renderTarget = "light";
});

datagrid.renderTarget is currently an undocumented API, but I don't anticipate it will change in the near term.

jakehadar commented 1 month ago

Awesome, this is exactly what I need. Thank you for the quick response!