fix(engine-core): revert "optimize computation of transitive shadow mode" #2859

nolanlawson · 2022-05-31T21:58:24Z

Details

It turns out that #2803 (95ce4c3) introduced a regression in shadow DOM mixed mode. We didn't notice the regression, because it was only caught by a test @jodarove wrote, which was in #2781, which isn't merged yet. After merging that PR with master, I noticed the regression.

This PR fixes the regression, and adds two tests to capture the regression, based on Jose's original test.

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

Shadow DOM mixed mode will work correctly now.

packages/@lwc/engine-core/src/framework/vm.ts

nolanlawson · 2022-05-31T22:01:52Z

packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js

+            });
+        });
+    });
+}


This test passes before 95ce4c3, fails starting with 95ce4c3, and passes in this PR.

nolanlawson · 2022-05-31T22:03:07Z

packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js

+                    expect(native.label.id).toEqual('foo');
+                    expect(native.input.getAttribute('aria-labelledby')).toEqual('foo');
+                });
+            });


The style token attributes and ID scoping are just two visible effects of shadowMode that we can test here. I don't think the ordering actually matters, but I kept it from Jose's other test.

ekashida

One thing I like about this change is that it goes back to the original assumption that ShadowMode is never null. My mental model of ShadowMode.Synthetic and ShadowMode.Native is that the latter simply means "not synthetic shadow" so a Light DOM component would be considered ShadowMode.Native. That being said, maybe there's a better way of categorizing the enums because it is admittedly confusing.

Could you confirm whether this change works for the case where you have a light dom component as the root component and the child component with no shadowSupportMode property?

packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js

…mponent/index.spec.js Co-authored-by: Eugene Kashida <ekashida@gmail.com>

pmdartus · 2022-06-01T08:02:32Z

packages/@lwc/engine-core/src/framework/vm.ts

                    // Synthetic if neither this component nor any of its ancestors are configured
                    // to be native.


This comment doesn't sound correct anymore. In this case, the shadowMode is inherited from its parent and falls back to SyntheticShadow if it's the root component.

Suggested change

// Synthetic if neither this component nor any of its ancestors are configured

// to be native.

// Synthetic if it is the root component.

The old comment is still correct. Rootness has nothing to do with it.

There is confusion here around words like "inheritance" and "parent"/"child" because it gets confusing when we talk about subclasses/superclasses versus ancestors/descendants. In this case, "ancestor" refers to DOM hierarchy and not subclasses/superclasses.

…ode (#2803)" This reverts commit 95ce4c3.

nolanlawson · 2022-06-01T18:14:20Z

After writing some more tests for light DOM mode (light DOM parent, shadow DOM child), I discovered than neither 95ce4c3 nor this PR actually resulted in the correct behavior. So I've opted to just revert 95ce4c3 instead.

If we would like to revisit 95ce4c3 in some other PR, then that's great, but for now we should revert and add tests to ensure we don't regress again.

nolanlawson · 2022-06-01T18:16:10Z

packages/integration-karma/test/mixed-shadow-mode/synthetic-behavior/index.spec.js

+                });
+            });
+        });
+    });


There are probably even more tests we can add here:

grandparent with reset/any, parent with reset/any, leaf with reset/any (8 tests total)

slotter with reset/any, slottable with reset/any, leaf with reset/any (8 tests total)

I got tired just writing the tests I did, but it's probably valuable to add these at some point.

nolanlawson · 2022-06-01T18:18:25Z

packages/integration-karma/test/mixed-shadow-mode/synthetic-behavior/index.spec.js

+
+                it('should render the shadow root in the correct shadow mode', () => {
+                    expect(isNativeShadowRootInstance(ids.leaf.shadowRoot)).toEqual(nativeLeaf);
+                });


The reason for this test is that it's actually possible for us to render the shadow root in one shadow mode, and yet treat it as the other mode for scope attributes and ID-mangling. That was the whole bug we ran into.

can you elaborate more?

It's possible for us to render in native shadow mode (i.e. native browser shadow root), but still apply ID-mangling and style scope attributes. That's the bug this PR fixes.

If you run the tests in this PR without reverting 95ce4c3 then you can see an example of it:

ekashida

Thanks for adding the tests, we should definitely revisit this.

nolanlawson · 2022-06-03T17:22:37Z

Opened an issue to track revisiting this: #2863

* fix: only remove slot children in synthetic shadow (#2843) * fix: only remove slot children in synthetic shadow * fix: use case block * fix: only add version mismatch test code in karma (#2852) * test(integration-karma): ensure constructable stylesheets are re-used (#2844) * test(integration-karma): ensure constructable stylesheets are re-used * test: add test for shared style * chore(nucleus): remove more downstreams (#2855) * chore(nucleus): remove another downstream (#2857) * docs: fix typo in template compiler readme (#2848) * docs: fix typo in template compiler readme * docs: rewording usage of lwc dynamic directive Co-authored-by: Eugene Kashida <ekashida@gmail.com> Co-authored-by: Eugene Kashida <ekashida@gmail.com> * test(integration-karma): update ACT components (#2862) * fix(engine-core): revert "optimize computation of transitive shadow mode" (#2859) * fix(engine-core): correctly compute shadowMode * fix: fix comment * fix: update packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js Co-authored-by: Eugene Kashida <ekashida@gmail.com> * fix: fix another one * fix: improve tests * fix: improve tests * fix: improve test * fix: revert * Revert "refactor(engine): optimize computation of transitive shadow mode (#2803)" This reverts commit 95ce4c3. Co-authored-by: Eugene Kashida <ekashida@gmail.com> * chore: release v2.14.1 (#2866) Co-authored-by: Nolan Lawson <nolan@nolanlawson.com> Co-authored-by: Mohan Raj Rajamanickam <1509984+mohanraj-r@users.noreply.github.com> Co-authored-by: Eugene Kashida <ekashida@gmail.com> Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>

wip: add a static vnode type wip: needs key and patch wip: all tests passing exept a few because of simple things wip: set style tokens for fragments during template evaluation wip: set element shadow token for fragments during template evaluation wip: propagate the shadow resolver key of static fragments wip: do not gen static nodes for text or comments wip: use tagedTemplate expression to replate stylesheetToken wip: use cloneNode from renderer wip: treeWalker to work in ie11 refactor: do not strip empty attr or empty class attr fix: using incorrect key wip: trim value of textNodes and review feedback fix: hydration feat: custom static element serializer wip: remove unessesary import fix: hydration fix: snapshot tests fix: missing karma test fix: test due rebase test: add test for static content needing nodeOwner fix: escape strings in serializer refactor: remove unused apis on generated code refactor: review suggestions fix: support mixed mode wip: fix compilation snapshots fix: increase 0.5kb bundlesize for engine dom fix: flapper wip: helpers.ts review wip: codegen.ts review wip: missing items from pm review wip: review comments fix: respect preserveComments and fuse $1,2 into 3 fix: svg content with the correct namespace feat(template-compiler): add option to disable static content optimizations wip: remove invalid comment chore: bump version to v2.13.0 (#2784) chore: dependencies upgrade (#2785) test: fix Node warning about event emitters (#2789) chore: run karma and integration tests in parallel (#2792) * chore: run karma and integration tests in parallel * fix: remove log lines fix(babel-plugin-component): remove import validation (#2719) test: remove flakey IE integration test (#2796) test: update test to use lwc imports (#2794) chore: Restrict further import order (#2795) chore: bump version to v2.13.1 (#2804) refactor(engine): moving vm references from dom into core (#2801) * refactor(engine): moving vm references from dom into core chore(nucleus): remove salesforcedevs/developer-website (#2807) test(integration-karma): small quality-of-life improvements (#2809) chore(deps): bump ejs from 3.1.6 to 3.1.7 (#2810) chore: weekly dependencies upgrade (#2816) * chore: weekly dependencies upgrade * fix: update yarn.lock` refactor(engine): optimize computation of transitive shadow mode (#2803) chore(deps): bump async from 2.6.3 to 2.6.4 (#2815) Bumps [async](https://github.com/caolan/async) from 2.6.3 to 2.6.4. - [Release notes](https://github.com/caolan/async/releases) - [Changelog](https://github.com/caolan/async/blob/v2.6.4/CHANGELOG.md) - [Commits](caolan/async@v2.6.3...v2.6.4) --- updated-dependencies: - dependency-name: async dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore: bump version to v2.13.2 (#2819) chore: retry failed Circle CI tests (#2814) * chore: retry failed Circle CI tests W-10946477 * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: fix * chore: deliberately fail a test to see what happens * chore: improve retry script * fix: whitespace * Revert "chore: deliberately fail a test to see what happens" This reverts commit 611fc34. * chore: rename to retry_karma fix(engine-core): add shim for old stylesheetTokens internal API (#2821) W-11093934 chore: bump version to v2.13.3 (#2823) fix(build): remove swc, switch back to babel and terser (#2818) feat: add freezeTemplate() API, warn on mutation (#2825) * feat: add freezeTemplate() API, warn on mutation * fix: warn on slots/renderMode as well * fix: add comment * fix: remove duplicate process.env.NODE_ENV check fix(engine-dom): refactor stylesheet API for consistency (#2827) * fix(engine-dom): refactor stylesheet API for consistency * fix: remove useless code comment * test: remove unnecessary test * test: remove unnecessary test * refactor: slight refactor * fix: add code comments * fix: add code comments * fix: add better comment fix: relax static id validation in iterations (#2830) fix(rollup-plugin): emit warnings during compilation (#2833) * fix(rollup-plugin): emit warnings during compilation Fixes #2771 W-10930894 * fix: add code comment fix(engine-dom): make feature flags work (#2812) * fix(engine-dom): make feature flags work Fixes #2811 * fix: license headers * test: fix jest tests * test: fix test * test: fix test * fix: use Eugene's technique instead * Revert "fix: use Eugene's technique instead" This reverts commit 72afdc0. * fix: use Eugene's technique instead * fix: revert unnecessary test change * fix: revert, use the elaborate test instead * fix: fix feature flags in engine-server as well perf(engine-dom): refactor style cache to reduce lookups (#2832) * perf(engine-dom): refactor style cache to reduce lookups * fix: tidy up comments * fix: update packages/@lwc/engine-dom/src/styles.ts Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> * fix: remove semi * fix: remove "used" flag * fix: refactor * fix: refactor * fix: bring back "used" flag * fix: typo Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> chore: update deps (#2838) test: run feature flag test code only in karma (#2835) fix: trigger slotchange event on removing slot (#2840) test(integration-karma): silence lwc rollup plugin warnings (#2836) * test(integration-karma): silence lwc rollup plugin warnings * fix: use warn API v2.11.7 (#2842) chore: release v2.14.0 (#2846) fix: only remove slot children in synthetic shadow (#2843) * fix: only remove slot children in synthetic shadow * fix: use case block fix: only add version mismatch test code in karma (#2852) test(integration-karma): ensure constructable stylesheets are re-used (#2844) * test(integration-karma): ensure constructable stylesheets are re-used * test: add test for shared style chore(nucleus): remove more downstreams (#2855) chore(nucleus): remove another downstream (#2857) docs: fix typo in template compiler readme (#2848) * docs: fix typo in template compiler readme * docs: rewording usage of lwc dynamic directive Co-authored-by: Eugene Kashida <ekashida@gmail.com> Co-authored-by: Eugene Kashida <ekashida@gmail.com> chore: fix lint test: refactor test, remove test covered in #2859 test: on second thought, bring test back

* fix: only remove slot children in synthetic shadow (#2843) * fix: only remove slot children in synthetic shadow * fix: use case block * fix: only add version mismatch test code in karma (#2852) * test(integration-karma): ensure constructable stylesheets are re-used (#2844) * test(integration-karma): ensure constructable stylesheets are re-used * test: add test for shared style * chore(nucleus): remove more downstreams (#2855) * chore(nucleus): remove another downstream (#2857) * docs: fix typo in template compiler readme (#2848) * docs: fix typo in template compiler readme * docs: rewording usage of lwc dynamic directive Co-authored-by: Eugene Kashida <ekashida@gmail.com> Co-authored-by: Eugene Kashida <ekashida@gmail.com> * test(integration-karma): update ACT components (#2862) * fix(engine-core): revert "optimize computation of transitive shadow mode" (#2859) * fix(engine-core): correctly compute shadowMode * fix: fix comment * fix: update packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js Co-authored-by: Eugene Kashida <ekashida@gmail.com> * fix: fix another one * fix: improve tests * fix: improve tests * fix: improve test * fix: revert * Revert "refactor(engine): optimize computation of transitive shadow mode (#2803)" This reverts commit 95ce4c3. Co-authored-by: Eugene Kashida <ekashida@gmail.com> * chore: release v2.14.1 (#2866) * refactor(engine-core): encapsulate renderer as an object and allow it to be injectable in vnodes (#2763) * refactor(engine-core): passing the renderer from an import statement in compiled templates Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com> Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com> Co-authored-by: Nolan Lawson <nolan@nolanlawson.com> * test(integration-karma): more shadow transitivity tests (#2864) * chore: release v.2.14.2 @W-7258582 (#2871) * Revert "chore: release v2.14.1 (#2866) @W-7258582 (#2867)" This reverts commit 518ab2e. Co-authored-by: Nolan Lawson <nolan@nolanlawson.com> Co-authored-by: Mohan Raj Rajamanickam <1509984+mohanraj-r@users.noreply.github.com> Co-authored-by: Eugene Kashida <ekashida@gmail.com> Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com> Co-authored-by: Caridy Patiño <caridy@gmail.com> Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>

fix(engine-core): correctly compute shadowMode

b6c5f62

nolanlawson requested review from caridy and ekashida May 31, 2022 21:58

nolanlawson commented May 31, 2022

View reviewed changes

packages/@lwc/engine-core/src/framework/vm.ts Show resolved Hide resolved

nolanlawson commented May 31, 2022

View reviewed changes

fix: fix comment

9c93afd

nolanlawson added a commit that referenced this pull request May 31, 2022

test: refactor test, remove test covered in #2859

c4640ef

ekashida reviewed May 31, 2022

View reviewed changes

packages/integration-karma/test/mixed-shadow-mode/dual-component/index.spec.js Outdated Show resolved Hide resolved

nolanlawson and others added 2 commits May 31, 2022 16:42

fix: update packages/integration-karma/test/mixed-shadow-mode/dual-co…

4c115d2

…mponent/index.spec.js Co-authored-by: Eugene Kashida <ekashida@gmail.com>

fix: fix another one

1e0f0a7

pmdartus reviewed Jun 1, 2022

View reviewed changes

nolanlawson added 5 commits June 1, 2022 10:53

fix: improve tests

872fb77

fix: improve tests

aacbf02

fix: improve test

338bb67

fix: revert

a8d6889

Revert "refactor(engine): optimize computation of transitive shadow m…

79b7cdd

…ode (#2803)" This reverts commit 95ce4c3.

nolanlawson commented Jun 1, 2022

View reviewed changes

nolanlawson requested review from ekashida and pmdartus June 1, 2022 18:16

nolanlawson commented Jun 1, 2022

View reviewed changes

nolanlawson changed the title ~~fix(engine-core): correctly compute shadowMode~~ fix(engine-core): revert "optimize computation of transitive shadow mode" Jun 1, 2022

nolanlawson mentioned this pull request Jun 3, 2022

perf: special handling for static elements #2781

Merged

ekashida approved these changes Jun 3, 2022

View reviewed changes

nolanlawson merged commit 27eee98 into master Jun 3, 2022

nolanlawson deleted the nolan/shadow-mode-fix branch June 3, 2022 17:21

nolanlawson mentioned this pull request Jun 3, 2022

Optimize computation of transitive shadow mode #2863

Open

nolanlawson mentioned this pull request Jun 3, 2022

test(integration-karma): more shadow transitivity tests #2864

Merged

		// Synthetic if neither this component nor any of its ancestors are configured
		// to be native.

	// Synthetic if neither this component nor any of its ancestors are configured
	// to be native.
	// Synthetic if it is the root component.

fix(engine-core): revert "optimize computation of transitive shadow mode" #2859

fix(engine-core): revert "optimize computation of transitive shadow mode" #2859

Uh oh!

Conversation

nolanlawson commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

Uh oh!

Uh oh!

nolanlawson May 31, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekashida left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmdartus Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson commented Jun 1, 2022

Uh oh!

nolanlawson Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

caridy Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

nolanlawson Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

ekashida left a comment

Choose a reason for hiding this comment

Uh oh!

nolanlawson commented Jun 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nolanlawson commented May 31, 2022 •

edited

Loading

nolanlawson May 31, 2022 •

edited

Loading