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

tomjakubowski · 2023-12-29T21:23:31Z

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"

texodus

Thanks for the PR! A couple of things need fixing:

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.
enableShadowDOM should be true by default, e.g. we should default use the Shadow DOM. It looks like the examples work in this mode with this PR (👍 ), so no changes needed there. It is ok if the tests remain as Light DOM tests because they do some DOM surgery to check table elements, but ...
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 (?)
The tests are currently failing regardless (looks like perspective-jupyterlab).
Let's remove the examples/blocks code which parses the URL.

examples/blocks/src/editable/index.html

examples/blocks/src/magic/index.js

packages/perspective-viewer-datagrid/src/js/custom_elements/datagrid.js

tomjakubowski · 2024-01-03T01:25:06Z

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 · 2024-01-17T01:59:49Z

All the checks are passing now, re-review?

tomjakubowski · 2024-01-17T02:01:41Z

tools/perspective-test/results.tar.gz

curious if you have any recommendations for quickly diffing these tarballs?

Not sure what you mean. In theory, if the snapshots don't change, the tarball should be identical. We've added a few flags to the gzip process to force this to be the case regardless of things like file creation time.

I have noticed that this is not always the case, but I don't know why yet, nor have I even validated that this is an issue with the tarball or the actual files.

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" ```

texodus

Thanks for the PR! Looks good!

jakehadar · 2024-05-07T17:06:44Z

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 · 2024-05-08T18:01:18Z

@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 · 2024-05-09T12:21:36Z

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

finos-cla-bot bot added the cla-present label Dec 29, 2023

tomjakubowski force-pushed the bugfix/regular-table-shadow-dom branch from 0132c9f to f34bb03 Compare December 29, 2023 21:30

texodus changed the title ~~datagrid: add feature flag to render table in shadow DOM~~ Add option to render table in the shadow DOM for @finos/perspective-viewer-datagrid Dec 30, 2023

texodus added the enhancement Feature requests or improvements label Dec 30, 2023

texodus requested changes Jan 1, 2024

View reviewed changes

tomjakubowski force-pushed the bugfix/regular-table-shadow-dom branch from c57c0a0 to 83fe46c Compare January 13, 2024 02:28

tomjakubowski changed the title ~~Add option to render table in the shadow DOM for @finos/perspective-viewer-datagrid~~ Render table in the shadow DOM for @finos/perspective-viewer-datagrid Jan 13, 2024

tomjakubowski force-pushed the bugfix/regular-table-shadow-dom branch from fffed57 to 6e7b217 Compare January 17, 2024 01:13

tomjakubowski commented Jan 17, 2024

View reviewed changes

tomjakubowski force-pushed the bugfix/regular-table-shadow-dom branch from 6e7b217 to 5b5624a Compare January 19, 2024 04:48

texodus approved these changes Jan 22, 2024

View reviewed changes

texodus merged commit 345edf5 into finos:master Jan 22, 2024
13 checks passed

tomjakubowski deleted the bugfix/regular-table-shadow-dom branch May 8, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tomjakubowski commented Dec 29, 2023 •

edited

Loading

texodus left a comment

tomjakubowski commented Jan 3, 2024

tomjakubowski commented Jan 17, 2024

tomjakubowski Jan 17, 2024

texodus Jan 18, 2024

texodus left a comment

jakehadar commented May 7, 2024 •

edited

Loading

tomjakubowski commented May 8, 2024

jakehadar commented May 9, 2024 via email •

edited

Loading

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

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

Conversation

tomjakubowski commented Dec 29, 2023 • edited Loading

texodus left a comment

Choose a reason for hiding this comment

tomjakubowski commented Jan 3, 2024

tomjakubowski commented Jan 17, 2024

tomjakubowski Jan 17, 2024

Choose a reason for hiding this comment

texodus Jan 18, 2024

Choose a reason for hiding this comment

texodus left a comment

Choose a reason for hiding this comment

jakehadar commented May 7, 2024 • edited Loading

tomjakubowski commented May 8, 2024

jakehadar commented May 9, 2024 via email • edited Loading

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

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

tomjakubowski commented Dec 29, 2023 •

edited

Loading

jakehadar commented May 7, 2024 •

edited

Loading

jakehadar commented May 9, 2024 via email •

edited

Loading