perf: switch synthetic scoping from attributes to classes

[nolanlawson]

Details

Switches synthetic shadow from using attributes to classes for style scoping.

I had already done some perf testing using non-LWC benchmarks that demonstrated that classes are faster than attributes, but for this one I wrote a new benchmark using this PR itself, and I still see an improvement, especially in Chrome. This improvement holds even in Chrome Canary, which has this fix for selectors like [foo] *.

In Safari and Firefox, it's basically a wash.

	Attributes	Classes	Delta	Delta (%)
Chrome 90.0.4430.212	537.55	453.57	-83.98	-15.62%
Chrome Canary 92.0.4506.0	653.6	568	-85.6	-13.10%
Safari 14.1	253	233	-20	-7.91%
Firefox 88.0.1	289	288	-1	-0.35%
Firefox Nightly 90.0a1	292	300	8	2.74%

Does this PR introduce breaking changes?

• ✅ No, it does not introduce breaking changes.

Not a breaking change per se, but an observable change. It seems unlikely to me though that these shadow/host tokens will conflict with any classes users are using.

GUS work item

W-7257730

[pmdartus]

It is pointed in the description, this change is a non-breaking but visible change. The fact that this change is fairly self-contained makes me comfortable that we will be capable without much effort to roll it back if necessary.

I would vote for moving with this. But I would like to get @ravijayaramappa and @ekashida opinions on this.

[ravijayaramappa]

Looks like this was the last remaining usage of removeAttribute. Please clean up removeAttribute in ../env/element.

[ravijayaramappa]

LGTM.

1. This might affect testing tools that are expecting the attribute to locate host elements.
2. The lwc-test utils appear to be not relying on this attribute, so that is good.

[ravijayaramappa]

@trevor-bliss FYI, this PR is moving the host locator from being an attribute to a css class. Tagging you to alert us if there are testing tools that rely on the attribute.

[nolanlawson]

I admit I am concerned about unexpected downstream breakages. E.g. customers doing something like:

this.template.querySelector('h1').className = 'woot' // clearing out entire class

So I would like to verify this perf improvement in the actual perf lab, to justify the risk.

That said, hopefully the risk is low, and we should be able to revert this pretty quickly if needed.

[caridy]

The PR looks good... good number as well, I only have one thing to consider:

In the past, we went for an attribute for one reason: classes can accidentally be wiped out from within and from outside of the component, causing the styles to be all messed up, while attributes are a lot more difficult, e.g.:

export default class Foo extends LightningElement {
       handleClick() {
           this.template.querySelector('x-foo').className = 'abc'; // very common way of setting a class according to google search
       }
}

As a result, x-foo classes, and everything below x-foo (its shadow included) is going to be broken. So, the chance of an accidental issue is way higher with the classes than with the attribute. That's my only concern. If the numbers worth this issue, then I'm good with it.

[nolanlawson]

Resolved today that we'll test this in the perf lab before deciding to proceed.

[ekashida]

When we discussed moving from attributes to classes, I was under the impression that we would have safeguards in place to prevent developers from accidentally interacting with our style scoping class. This seems necessary since class attributes are used so much.

[nolanlawson]

Perf tests in the lab do not show a statistically significant difference between classes and attributes. Closing this PR.