Proposal/RFC scoped styles with custom elements

[tomalec]

Changes proposed in this Pull Request:

This is another proposal to use Shadow DOM to encapsulate at least some of our styles.

This is an alternative to #539 that joins #538
And has all the pros and cons of both :)

Screenshots:

Declutters HTML

• We no longer need to autogenerate class names and Emotion dependency to do scoping
• Element is denoted by its name, so we do not mix the purpose of class attribute of selecting a group of elements of a kind.

Declutters CSS

• We now know which rules comes from element definition, which are applied by the consumer of an element.
  

Coding flexibility

• We can use single file components
• We are CSS-in-JS ready
• But we can still use separate CSS files if we want to
• We can now explicitly import and export just styles for elements, to extend/re-use them
  

Detailed test instructions:

1. checkout, npm run build
2. go to /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fsettings
3. Inspect the "Connected" element.

Additional details:

1. To make it work in Safari, we'd need either use a CSS polyfill, like https://www.npmjs.com/package/construct-style-sheets-polyfill - still lighter, and more bulletproof than emotion. Or we can stamp <style> tag to every root to play safe.
2. Also, in long run we could enrich the css tagged template, or replace it with something from an external library, like lit.
3. To get css</code> highlighted in VSCode, you need a plugin like https://marketplace.visualstudio.com/items?itemName=bashmish.es6-string-css or lit-html`
4. If we would like to avoid custom element name collisions. In case somebody else happen to register an element with <gla-connected-icon-label> or we want to use two different versions of the same component. What we currently cannot with @wordpress/ and @woocommerce/components we can simply add generated suffix to defineShadowStylesHost and make it return the name. Styles will keep their isolation thanks to Shadow DOM.

Changelog entry

Dev - scope styles with shadow hosting custom element.

[eason9487]

If I understood correctly, this solution is mainly to reduce/avoid the style naming conflicts with upstream or other extensions, and this brings a further benefit that the children's style naming within a scoped component will be easier, right?

If I remember rightly, the style conflicts that had/have happened in practice were probably these situations:

1. Conflicts between WP and WC components themselves due to the current use of DEWP
   • Fix the giant icon problem in the warning messages #528
   • WP 6.1 Compatibility: Fix the giant icons in the DatePicker component #1739
2. GLA's style affects others
   • Prevent the GLA button styles from overriding other styles or being overridden by other styles #1391
   • CSS file is loaded in Marketing Overview page causing CSS issue #1765
3. Others' style affects GLA
   • Prevent the GLA button styles from overriding other styles or being overridden by other styles #1391
   • PMax Assets: Add a tour to the Dashboard page for introducing the campaign assets #1880 (comment)
   • PMax Assets: Prevent the button styles in ImagesSelector from overriding by AutomateWoo #1909

In the first case, after WP and WC packages are externalized, it should not occur because of GLA's DEWP setting. In the second case, the only direct importing of node_modules/@wordpress/components/src/panel/style will also be removed soon, and with the .gla- prefix and .gla-admin-page scoping support already in place, this is unlikely to happen in the future.

However, the third one is still not effectively addressed, since most GLA components are heavily based on WP and WC components, there seems no way to completely avoid style conflicts from outside on its own CSS name (e.g. .components-button). This solution does not seem to be applicable to solving this problem either?

In addition, the Fast Refresh development mode (npm run start:hot) seems to conflict with this approach. Once a code change occurs within a component that uses defineShadowStylesHost, subsequent code changes will not make Fast Refresh update the page. Must refresh the page to resume.

[tomalec]

If I understood correctly, this solution is mainly to reduce/avoid the style naming conflicts with upstream or other extensions, and this brings a further benefit that the children's style naming within a scoped component will be easier, right?

Not quite.

This PRs aims to improve the DevX to make our developer life merrier, help to avoid bugs (incl. style conflicts), and speed up debugging. The improvements are listed in bullet points in OP. The ones I'd highlight:

• We have a way of denoting styles for a component (set in component's definition), distinct from a way to add styles for s subclass of instances by the consumers of those elements. (Currently, we use the class attribute for both)
• It's easier to find the component in the elements tab in dev tools
• Styles are modular and reusable in a more JSy way
• Styles are actually and technically isolated. So there is a technical solution protecting us from the style leak. As opposed gla- prefix, which is just a convention that reduces the chance of such leak but gives no technical protection.

Also, I'd like to emphasize one thing: this solution is mostly to specify and encapsulate the styles for an HTML Element (<div> or other) we do create and govern, not the ones created by external components (<WooCommerceButton>) (Unless we start using those in Shadow DOM, but it has more severe implications, that I'd like to avoid on this baby step).
We are defining styles for a host we created.

So I do not aim to fix the issues you link.

---

1. Conflicts between WP and WC components themselves due to the current use of DEWP

As long as we do not have the ambition to change WP & WC conflict, this proposal does not aim to fix that problem. As you said DEWPing all the things should be the solution.

2. GLA's style affects others

As you mentioned, .gla- & .gla-admin-page fix those specific bugs. However, the solution proposed here aims to protect agins our GLA's styles from leaking and affecting others and has some advantages over .gla- prefix convention.

• It does not clutter the code with more class names and high CSS specificity, nor it requires and parent or wrapper element.
• Allows using/importing our component outside of the GLA scope and still be encapsulated.

3. Others' style affects GLA

The solution proposed here is to style only the specific, GLA-defined host element (gla-connected-icon-label).
And as you said

most GLA components are heavily based on WP and WC components

So "This solution does not seem to be applicable to solving" others' styles styling elements like .components-button.

But the aim here is to encapsulate the styles of <gla-connected-icon-label>. And after this PR, for others to affect those, they would need to either use a selector like *, or gla-connected-icon-label to affect our element, which is unlikely, or they could attach a class attribute to our element. And then it means they do that on purpose and probably mean to do so.

[tomalec]

In addition, the Fast Refresh development mode (npm run start:hot) seems to conflict with this approach. Once a code change occurs within a component that uses defineShadowStylesHost, subsequent code changes will not make Fast Refresh update the page. Must refresh the page to resume.

I didn't use Fast Refresh. I'll take a look and check what happens there

[eason9487]

So I do not aim to fix the issues you link.

Thanks for clarifying.

I originally thought it was related to those style conflict cases because this PR came up in this slack thread: p1668008895895869/1668000686.809249-slack-C0410KV3YLW

Also, I'd like to emphasize one thing: this solution is mostly to specify and encapsulate the styles for an HTML Element (<div> or other) we do create and govern, not the ones created by external components (<WooCommerceButton>) (Unless we start using those in Shadow DOM, but it has more severe implications, that I'd like to avoid on this baby step).
We are defining styles for a host we created.

After a quick browsing of the .~/components directory, there are probably only a few components completely hosted by GLA:

• AppTableCardDiv
• ContentButtonLayout
• VerticalGapLayout
• WistiaVideo
• .~/components/paid-ads/campaign-preview/mockup-*.js
• .~/components/paid-ads/campaign-preview/components
• .~/components/stepper

Considering that

• there are not many cases that can be applied in practice,
• GLA style leak problem has been generally solved, although .gla- and .gla-admin-page are not complete technical guarantees, they are low cost and actually effective,
• this solution might not be effective for the major style conflict problem,

  3. Others' style affects GLA

Currently, I think this is probably still an experimental-stage method and a bit far from the actual adoption. Or, maybe try WistiaVideo as a starting point to have a taste of this. (Before solving the problem of Fast Refresh mode, campaign preview is not so suitable, because the development of it requires a lot of style adjustment, so manually refreshing the page would reduce productivity too much.)