Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: enable face lifecycle methods and wrap ElementInternals in a proxy #3695

Merged
merged 22 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
8cc69e0
refactor: move element internals validation to shared
jmsjtu Aug 31, 2023
f87e6b6
feat: face callback implementation
jmsjtu Aug 31, 2023
3a0beae
test: add tests
jmsjtu Aug 31, 2023
2e968d3
Merge branch 'master' of github.com:salesforce/lwc into jtu/face-call…
jmsjtu Sep 1, 2023
088e6dc
test: include light dom test in synthetic lifecycle
jmsjtu Sep 1, 2023
616d6f7
Merge branch 'master' of github.com:salesforce/lwc into jtu/face-call…
jmsjtu Sep 15, 2023
4c80dd5
refactor: move element internals check to engine-dom
jmsjtu Sep 15, 2023
510a8fd
refactor: rename toThrowConnectedError to toThrowNativeCallbackError
jmsjtu Sep 15, 2023
2a1fe36
refactor: make face callbacks fail silently when formAssociated missing
jmsjtu Sep 15, 2023
d6c58fd
refactor: remove browser check on face callbacks
jmsjtu Sep 15, 2023
5928112
Merge branch 'master' of github.com:salesforce/lwc into jtu/face-call…
jmsjtu Sep 15, 2023
2ae5bb0
fix: add check on static formAssociated set to false
jmsjtu Sep 18, 2023
ae9f3c4
test: update test for safari bug
jmsjtu Sep 20, 2023
c8239d0
test: fix custom element registering safari-focus-bug multiple times
jmsjtu Sep 20, 2023
be9daea
Merge branch 'master' of github.com:salesforce/lwc into jtu/face-call…
jmsjtu Sep 21, 2023
3747c72
test: check ElementInternals is available in browser
jmsjtu Sep 21, 2023
8172583
Merge branch 'master' of github.com:salesforce/lwc into jtu/face-call…
jmsjtu Sep 27, 2023
5175017
test: update test for error condition
jmsjtu Sep 27, 2023
290084f
Merge branch 'jtu/face-callbacks' of github.com:salesforce/lwc into j…
jmsjtu Sep 27, 2023
3f63b07
test: clean up safari focus bug test
jmsjtu Sep 27, 2023
11ea65f
Merge branch 'master' of github.com:salesforce/lwc into jtu/face-call…
jmsjtu Sep 29, 2023
16850ca
feat: apply proxy to `ElementInternals` object (#3742)
jmsjtu Sep 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions packages/@lwc/engine-core/src/framework/base-bridge-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
isNull,
} from '@lwc/shared';
import { applyAriaReflection } from '@lwc/aria-reflection';
import { logError, logWarn } from '../shared/logger';
import { logWarn } from '../shared/logger';
import { getAssociatedVM } from './vm';
import { getReadOnlyProxy } from './membrane';
import { HTMLElementConstructor, HTMLElementPrototype } from './html-element';
Expand Down Expand Up @@ -204,15 +204,39 @@ export function HTMLBridgeElementFactory(

// To avoid leaking private component details, accessing internals from outside a component is not allowed.
descriptors.attachInternals = {
set() {
if (process.env.NODE_ENV !== 'production') {
logWarn(
'attachInternals cannot be accessed outside of a component. Use this.attachInternals instead.'
);
}
},
get() {
if (process.env.NODE_ENV !== 'production') {
logError(
logWarn(
'attachInternals cannot be accessed outside of a component. Use this.attachInternals instead.'
);
}
},
};

descriptors.formAssociated = {
set() {
if (process.env.NODE_ENV !== 'production') {
logWarn(
'formAssociated cannot be accessed outside of a component. Set the value within the component class.'
);
}
},
get() {
if (process.env.NODE_ENV !== 'production') {
logWarn(
'formAssociated cannot be accessed outside of a component. Set the value within the component class.'
);
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
}
},
};

// Specify attributes for which we want to reflect changes back to their corresponding
// properties via attributeChangedCallback.
defineProperty(HTMLBridgeElement, 'observedAttributes', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
defineProperties,
defineProperty,
freeze,
isFalse,
isFunction,
isNull,
isObject,
Expand Down Expand Up @@ -136,6 +135,7 @@ export interface LightningElementConstructor {

delegatesFocus?: boolean;
renderMode?: 'light' | 'shadow';
formAssociated?: boolean;
shadowSupportMode?: ShadowSupportMode;
stylesheets: TemplateStylesheetFactories;
}
Expand Down Expand Up @@ -193,6 +193,10 @@ export interface LightningElement extends HTMLElementTheGoodParts, AccessibleEle
disconnectedCallback?(): void;
renderedCallback?(): void;
errorCallback?(error: any, stack: string): void;
formAssociatedCallback?(): void;
formResetCallback?(): void;
formDisabledCallback?(): void;
formStateRestoreCallback?(): void;
}

/**
Expand Down Expand Up @@ -296,7 +300,59 @@ function warnIfInvokedDuringConstruction(vm: VM, methodOrPropName: string) {
}
}

const supportsElementInternals = typeof ElementInternals !== 'undefined';
// List of properties on ElementInternals that are formAssociated can be found in the spec:
// https://html.spec.whatwg.org/multipage/custom-elements.html#form-associated-custom-elements
const formAssociatedProps = new Set([
'setFormValue',
'form',
'setValidity',
'willValidate',
'validity',
'validationMessage',
'checkValidity',
'reportValidity',
'labels',
]);

// Verify that access to a form-associated property of the ElementInternals proxy has formAssociated set in the LWC.
function assertFormAssociatedPropertySet(propertyKey: string, isFormAssociated: boolean) {
if (formAssociatedProps.has(propertyKey) && !isFormAssociated) {
//Note this error message mirrors Chrome and Firefox error messages, in Safari the error is slightly different.
throw new DOMException(
`Failed to execute '${propertyKey}' on 'ElementInternals': The target element is not a form-associated custom element.`
);
}
}

// Wrap all ElementInternal objects in a proxy to prevent form association when `formAssociated` is not set on an LWC.
// This is needed because the 1UpgradeableConstructor1 always sets `formAssociated=true`, which means all
// ElementInternal objects will have form-associated properties set when an LWC is placed in a form.
// We are doing this to guard against customers taking a dependency on form elements being associated to ElementInternals
// when 'formAssociated' has not been set on the LWC.
function createElementInternalsProxy(
elementInternals: ElementInternals,
isFormAssociated: boolean
) {
const elementInternalsProxy = new Proxy(elementInternals, {
set(target, propertyKey, newValue) {
// ElementInternals implementation uses strings as property keys exclusively in chrome, firefox, and safari
assertFormAssociatedPropertySet(propertyKey as string, isFormAssociated);
return Reflect.set(target, propertyKey, newValue);
},
get(target, propertyKey) {
// ElementInternals implementation uses strings as property keys exclusively in chrome, firefox, and safari
assertFormAssociatedPropertySet(propertyKey as string, isFormAssociated);
const internalsPropertyValue = Reflect.get(target, propertyKey);
// Bind the property value to the target so that function invocations are called with the
// correct context ('this' value).
return typeof internalsPropertyValue === 'function'
? internalsPropertyValue.bind(target)
: internalsPropertyValue;
},
});

return elementInternalsProxy;
}

// @ts-ignore
LightningElement.prototype = {
Expand Down Expand Up @@ -467,21 +523,19 @@ LightningElement.prototype = {
const vm = getAssociatedVM(this);
const {
elm,
def: { formAssociated },
renderer: { attachInternals },
} = vm;

if (isFalse(supportsElementInternals)) {
// Browsers that don't support attachInternals will need to be polyfilled before LWC is loaded.
throw new Error('attachInternals API is not supported in this browser environment.');
}

if (vm.renderMode === RenderMode.Light || vm.shadowMode === ShadowMode.Synthetic) {
if (vm.shadowMode === ShadowMode.Synthetic) {
jmsjtu marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(
'attachInternals API is not supported in light DOM or synthetic shadow.'
);
}

return attachInternals(elm);
const internals = attachInternals(elm);
// #TODO[2970]: remove proxy once `UpgradeableConstructor` has been removed
return createElementInternalsProxy(internals, Boolean(formAssociated));
},

get isConnected(): boolean {
Expand Down
41 changes: 37 additions & 4 deletions packages/@lwc/engine-core/src/framework/def.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,17 @@ export interface ComponentDef {
template: Template;
renderMode: RenderMode;
shadowSupportMode: ShadowSupportMode;
formAssociated: boolean | undefined;
ctor: LightningElementConstructor;
bridge: HTMLElementConstructor;
connectedCallback?: LightningElement['connectedCallback'];
disconnectedCallback?: LightningElement['disconnectedCallback'];
renderedCallback?: LightningElement['renderedCallback'];
errorCallback?: LightningElement['errorCallback'];
formAssociatedCallback?: LightningElement['formAssociatedCallback'];
formResetCallback?: LightningElement['formResetCallback'];
formDisabledCallback?: LightningElement['formDisabledCallback'];
formStateRestoreCallback?: LightningElement['formStateRestoreCallback'];
render: LightningElement['render'];
}

Expand Down Expand Up @@ -96,7 +101,11 @@ function getCtorProto(Ctor: LightningElementConstructor): LightningElementConstr
}

function createComponentDef(Ctor: LightningElementConstructor): ComponentDef {
const { shadowSupportMode: ctorShadowSupportMode, renderMode: ctorRenderMode } = Ctor;
const {
shadowSupportMode: ctorShadowSupportMode,
renderMode: ctorRenderMode,
formAssociated: ctorFormAssociated,
} = Ctor;

if (process.env.NODE_ENV !== 'production') {
const ctorName = Ctor.name;
Expand Down Expand Up @@ -137,8 +146,17 @@ function createComponentDef(Ctor: LightningElementConstructor): ComponentDef {
decoratorsMeta;
const proto = Ctor.prototype;

let { connectedCallback, disconnectedCallback, renderedCallback, errorCallback, render } =
proto;
let {
connectedCallback,
disconnectedCallback,
renderedCallback,
errorCallback,
formAssociatedCallback,
formResetCallback,
formDisabledCallback,
formStateRestoreCallback,
render,
} = proto;
const superProto = getCtorProto(Ctor);
const superDef =
superProto !== LightningElement ? getComponentInternalDef(superProto) : lightingElementDef;
Expand All @@ -162,6 +180,10 @@ function createComponentDef(Ctor: LightningElementConstructor): ComponentDef {
disconnectedCallback = disconnectedCallback || superDef.disconnectedCallback;
renderedCallback = renderedCallback || superDef.renderedCallback;
errorCallback = errorCallback || superDef.errorCallback;
formAssociatedCallback = formAssociatedCallback || superDef.formAssociatedCallback;
formResetCallback = formResetCallback || superDef.formResetCallback;
formDisabledCallback = formDisabledCallback || superDef.formDisabledCallback;
formStateRestoreCallback = formStateRestoreCallback || superDef.formStateRestoreCallback;
render = render || superDef.render;

let shadowSupportMode = superDef.shadowSupportMode;
Expand All @@ -174,6 +196,11 @@ function createComponentDef(Ctor: LightningElementConstructor): ComponentDef {
renderMode = ctorRenderMode === 'light' ? RenderMode.Light : RenderMode.Shadow;
}

let formAssociated = superDef.formAssociated;
if (!isUndefined(ctorFormAssociated)) {
formAssociated = ctorFormAssociated;
}
Comment on lines +199 to +202
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let formAssociated = superDef.formAssociated;
if (!isUndefined(ctorFormAssociated)) {
formAssociated = ctorFormAssociated;
}
const formAssociated = Boolean(superDef.formAssociated);

I looked into it, and it seems that the browser's treatment of formAssociated is based on truthiness. It would make our internal code simpler if we set formAssociated to either true/false in one place in the code, so that we could rely on it being a boolean everywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or undefined | boolean is also fine – just that we don't actually know what type superDef.formAssociated is right? The user could put static formAssociated = 'yolo'.


const template = getComponentRegisteredTemplate(Ctor) || superDef.template;
const name = Ctor.name || superDef.name;

Expand All @@ -191,10 +218,15 @@ function createComponentDef(Ctor: LightningElementConstructor): ComponentDef {
template,
renderMode,
shadowSupportMode,
formAssociated,
connectedCallback,
disconnectedCallback,
renderedCallback,
errorCallback,
formAssociatedCallback,
formDisabledCallback,
formResetCallback,
formStateRestoreCallback,
renderedCallback,
render,
};

Expand Down Expand Up @@ -289,6 +321,7 @@ const lightingElementDef: ComponentDef = {
methods: EmptyObject,
renderMode: RenderMode.Shadow,
shadowSupportMode: ShadowSupportMode.Default,
formAssociated: undefined,
wire: EmptyObject,
bridge: BaseBridgeElement,
template: defaultEmptyTemplate,
Expand Down
4 changes: 4 additions & 0 deletions packages/@lwc/engine-core/src/framework/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ export {
disconnectRootElement,
getAssociatedVMIfPresent,
computeShadowAndRenderMode,
runFormAssociatedCallback,
runFormDisabledCallback,
runFormResetCallback,
runFormStateRestoreCallback,
} from './vm';
export { createContextProviderWithRegister } from './wiring';

Expand Down
6 changes: 5 additions & 1 deletion packages/@lwc/engine-core/src/framework/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ export interface RendererAPI {
tagName: string,
upgradeCallback: LifecycleCallback,
connectedCallback?: LifecycleCallback,
disconnectedCallback?: LifecycleCallback
disconnectedCallback?: LifecycleCallback,
formAssociatedCallback?: LifecycleCallback,
formDisabledCallback?: LifecycleCallback,
formResetCallback?: LifecycleCallback,
formStateRestoreCallback?: LifecycleCallback
) => E;
ownerDocument(elm: E): Document;
registerContextConsumer: (
Expand Down
26 changes: 25 additions & 1 deletion packages/@lwc/engine-core/src/framework/rendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ import {
RenderMode,
rerenderVM,
runConnectedCallback,
runFormAssociatedCallback,
runFormDisabledCallback,
runFormResetCallback,
runFormStateRestoreCallback,
ShadowMode,
VM,
VMState,
Expand Down Expand Up @@ -319,6 +323,10 @@ function mountCustomElement(

let connectedCallback: LifecycleCallback | undefined;
let disconnectedCallback: LifecycleCallback | undefined;
let formAssociatedCallback: LifecycleCallback | undefined;
let formDisabledCallback: LifecycleCallback | undefined;
let formResetCallback: LifecycleCallback | undefined;
let formStateRestoreCallback: LifecycleCallback | undefined;

if (lwcRuntimeFlags.ENABLE_NATIVE_CUSTOM_ELEMENT_LIFECYCLE) {
connectedCallback = (elm: HTMLElement) => {
Expand All @@ -327,6 +335,18 @@ function mountCustomElement(
disconnectedCallback = (elm: HTMLElement) => {
disconnectRootElement(elm);
};
formAssociatedCallback = (elm: HTMLElement) => {
runFormAssociatedCallback(elm);
};
formDisabledCallback = (elm: HTMLElement) => {
runFormDisabledCallback(elm);
};
formResetCallback = (elm: HTMLElement) => {
runFormResetCallback(elm);
};
formStateRestoreCallback = (elm: HTMLElement) => {
runFormStateRestoreCallback(elm);
};
}

// Should never get a tag with upper case letter at this point; the compiler
Expand All @@ -338,7 +358,11 @@ function mountCustomElement(
normalizedTagname,
upgradeCallback,
connectedCallback,
disconnectedCallback
disconnectedCallback,
formAssociatedCallback,
formDisabledCallback,
formResetCallback,
formStateRestoreCallback
);

vnode.elm = elm;
Expand Down
Loading