fix(ssr): global attribute disparities by jhefferman-sfdc · Pull Request #5247 · salesforce/lwc

jhefferman-sfdc · 2025-02-28T00:42:12Z

Details

Does this pull request introduce a breaking change?

😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

🤞 No, it does not introduce an observable change.

GUS work item

W-17572462

...xtures/attribute-global-html/as-component-prop/undeclared/modules/x/component/component.html

...c/__tests__/fixtures/attribute-global-html/as-component-prop/without-@api-values/config.json

jhefferman-sfdc · 2025-02-28T00:48:08Z

...rver/src/__tests__/fixtures/attribute-global-html/as-component-prop/without-@api/config.json

-    "entry": "x/component"
+    "entry": "x/component",
+    "ssrFiles": {
+        "error": "error-ssr.txt",


V1/V2 deviate, where V2 parent overrides the child value (as does CSR). V1 is incorrect.

packages/@lwc/engine-server/src/__tests__/fixtures/attribute-global-html/draggable/config.json

packages/@lwc/engine-server/src/__tests__/fixtures/attribute-global-html/hidden/config.json

packages/@lwc/engine-server/src/__tests__/fixtures/attribute-global-html/spellcheck/config.json

jhefferman-sfdc · 2025-02-28T00:52:21Z

packages/@lwc/engine-server/src/__tests__/fixtures/attribute-global-html/tabindex/config.json

+    "entry": "x/component",
+    "ssrFiles": {
+        "error": "error-ssr.txt",
+        "expected": "expected-ssr.html"


V1/V2 deviate, where V2 matches CSR

...ixtures/known-boolean-attributes/default-def-html-attributes/static-on-component/config.json

jhefferman-sfdc · 2025-02-28T00:54:57Z

packages/@lwc/ssr-runtime/src/lightning-element.ts

        // Avoid setting the following types of properties that should not be set:
        // - Properties that are not public.
        // - Properties that are not global.
-        // - Properties that are global but are internally overridden.


Does not seem correct. In CSR, child definitions do not override parent. We had tests for this, added more.

If we had fixture tests for this, and it wasn't caught, we should add some karma tests to get the real browser behavior.

yeah i'm adding some karma for these

jhefferman-sfdc · 2025-02-28T16:25:21Z

/nucleus ignore --reason "Unrelated __lwc failure in webruntime"

wjhsf · 2025-02-28T16:21:30Z

packages/@lwc/ssr-runtime/src/lightning-element.ts

        // Avoid setting the following types of properties that should not be set:
        // - Properties that are not public.
        // - Properties that are not global.
-        // - Properties that are global but are internally overridden.


If we had fixture tests for this, and it wasn't caught, we should add some karma tests to get the real browser behavior.

wjhsf · 2025-02-28T16:23:20Z

packages/@lwc/ssr-compiler/src/compile-template/shared.ts

+                    literalValue = shouldNormalize ? '0' : literalValue;
                }
                return b.property('init', key, b.literal(literalValue));
            } else if (value.type === 'Literal' && typeof value.value === 'boolean') {


What happens if tabindex is set to a boolean value? Can it be?

Yes in the case of <div tabindex>. I don't know how that would be treated in the browser. Good thing to check.

Yup, per the strange CSR logic, booleans are preserved, so nothing to do for this case here.

packages/@lwc/ssr-compiler/src/compile-js/generate-markup.ts

wjhsf · 2025-02-28T16:27:51Z

packages/@lwc/ssr-compiler/src/compile-template/shared.ts

+                    // Global HTML "tabindex" attribute is specially massaged into a stringified number
+                    // This follows the historical behavior in api.ts:
+                    // https://github.com/salesforce/lwc/blob/f34a347/packages/%40lwc/engine-core/src/framework/api.ts#L193-L211
+                    const shouldNormalize = parseInt(literalValue) > 0;
+                    literalValue = shouldNormalize ? '0' : literalValue;


Suggested change

// Global HTML "tabindex" attribute is specially massaged into a stringified number

// This follows the historical behavior in api.ts:

// https://github.com/salesforce/lwc/blob/f34a347/packages/%40lwc/engine-core/src/framework/api.ts#L193-L211

const shouldNormalize = parseInt(literalValue) > 0;

literalValue = shouldNormalize ? '0' : literalValue;

literalValue = normalizeTabIndex(literalValue);

If we've got the util, use it!

wjhsf · 2025-02-28T16:30:35Z

packages/@lwc/shared/src/normalize-tab-index.ts

+ * This follows the historical behavior in api.ts:
+ * https://github.com/salesforce/lwc/blob/f34a347/packages/%40lwc/engine-core/src/framework/api.ts#L193-L211
+ */
+export function normalizeTabIndex(value: any): any {


Add some unit tests for this!

wjhsf · 2025-02-28T16:37:31Z

packages/@lwc/shared/src/normalize-tab-index.ts

+ * This follows the historical behavior in api.ts:
+ * https://github.com/salesforce/lwc/blob/f34a347/packages/%40lwc/engine-core/src/framework/api.ts#L193-L211
+ */
+export function normalizeTabIndex(value: any): any {


Suggested change

export function normalizeTabIndex(value: any): any {

export function normalizeTabIndex(value: number): string | number {

The original function was more broadly typed than necessary, since this is new we can be more restrictive.

Also, it's weird that this returns either the unmodified number or the string '0'. We should either always return a number or always return a string. (It's diverging from the original implementation, but not in a meaningful way.)

Yes this is weird behavior. There's probably an actual reason for it though - I wouldn't necessarily assume that the divergence is unmeaningful.

In the end, the attribute is going to end up a string attribute / numeric prop. This particular function is just an intermediate step in that. This particular function copies the business logic from engine-core, but the rest of the implementation (outside the function) is different, so I don't think it makes much of a difference whether the final cast to string/number happens in or out of the function.

Suggested change

export function normalizeTabIndex(value: any): any {

export function normalizeTabIndex<T>(value: T): T | string {

If you want to allow any type.

I tried generic but it added issues with the comparison. I wanted to keep it exactly as the original to avoid any discrepancies

...erver/src/__tests__/fixtures/attribute-global-html/tabindex/modules/x/component/component.js

divmain

Looks like there's a tiny bit more polish needed, but otherwise looks fantastic.

packages/@lwc/engine-server/src/__tests__/fixtures/attribute-global-html/hidden/config.json

divmain · 2025-02-28T18:55:16Z

packages/@lwc/shared/src/normalize-tab-index.ts

@@ -0,0 +1,16 @@
+/*
+ * Copyright (c) 2020, salesforce.com, inc.


Suggested change

* Copyright (c) 2020, salesforce.com, inc.

* Copyright (c) 2025, salesforce.com, inc.

divmain · 2025-02-28T18:57:27Z

packages/@lwc/shared/src/normalize-tab-index.ts

+ * This follows the historical behavior in api.ts:
+ * https://github.com/salesforce/lwc/blob/f34a347/packages/%40lwc/engine-core/src/framework/api.ts#L193-L211
+ */
+export function normalizeTabIndex(value: any): any {


Yes this is weird behavior. There's probably an actual reason for it though - I wouldn't necessarily assume that the divergence is unmeaningful.

divmain · 2025-02-28T18:57:45Z

packages/@lwc/ssr-compiler/src/__tests__/utils/expected-failures.ts

 export const expectedFailures = new Set([
-    'attribute-global-html/as-component-prop/undeclared/config.json',
-    'attribute-global-html/as-component-prop/without-@api/config.json',
-    'known-boolean-attributes/default-def-html-attributes/static-on-component/config.json',


🎉🎉🎉🎉🎉

divmain · 2025-02-28T18:59:19Z

packages/@lwc/ssr-compiler/src/compile-template/shared.ts

+                    literalValue = shouldNormalize ? '0' : literalValue;
                }
                return b.property('init', key, b.literal(literalValue));
            } else if (value.type === 'Literal' && typeof value.value === 'boolean') {


Yes in the case of <div tabindex>. I don't know how that would be treated in the browser. Good thing to check.

divmain · 2025-02-28T19:00:09Z

packages/@lwc/ssr-compiler/src/compile-template/shared.ts

                if (name === 'class') {
                    cxt.import('normalizeClass');
                    propValue = b.callExpression(b.identifier('normalizeClass'), [propValue]);
+                } else if (name.toLowerCase() === 'tabindex') {


nit: there is more than one name.toLowerCase() showing up in this function; probably could define nameLower variable.

jhefferman-sfdc · 2025-03-01T02:04:31Z

/nucleus ignore --reason "Unrelated __lwc failure in webruntime"

jhefferman-sfdc · 2025-03-01T02:06:49Z

I added hydration coverage for global property precedence. As SSR V1 renders the wrong values, it causes hydration errors. I added a check here and an expectation for those failures. Here is the work item to review this and change the pattern if needed: W-17939029

jhefferman-sfdc added 6 commits February 14, 2025 15:11

chore: testing hydration

03a7d22

chore: added tests

491420e

Merge branch 'master' into jhefferman/attribute-props

3a8cef5

chore: added dedicated tests for v1/2 delta

8ca936b

chore: modified public property test

6546b7e

fix: tabindex corrections, additional tests

1264593

jhefferman-sfdc requested a review from a team as a code owner February 28, 2025 00:42