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

Conversation

jmsjtu
Copy link
Member

@jmsjtu jmsjtu commented Sep 1, 2023

Details

This PR exposes the face lifecycle methods as described in the Element Internals and Form Association.

Note that exposing ElementInternals was already done in #3670.

These lifecycle methods are designed to only work with native lifecycle enabled and only for native shadow or light DOM.

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-13818672

@jmsjtu jmsjtu requested a review from a team as a code owner September 1, 2023 17:39
Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a few small nitpicks!

packages/@lwc/shared/src/html-element.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/vm.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/vm.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/vm.ts Outdated Show resolved Hide resolved
@jmsjtu
Copy link
Member Author

jmsjtu commented Sep 15, 2023

@nolanlawson The PR is ready for another round of reviews!

Also, moving forward with the ElementInternals enablement, I think we may need to do some additional work on how to handle setting the formAssociated value.

Since formAssociated = true is always set on the UpgradableConstructor when the component is placed inside a form element, the form property on ElementInternals will have a value regardless of whether formAssociated was set on the LWC.

Example:

<form>
    <x-foo></x-foo>
</form>
import { LightningElement, api } from 'lwc';

export default class extends LightningElement {
    @api
    internals = this.attachInternals();
}
Screenshot 2023-09-14 at 6 50 13 PM

In contrast, the browser behavior shows an exception when it's not set:
DOMException: Failed to read the 'form' property from 'ElementInternals': The target element is not a form-associated custom element.

Screenshot 2023-09-14 at 6 51 23 PM

Once we remove the UpgradeableConstructor and directly invoke the LWC constructor we will break any customers that create a dependency on the form being associated with ElementInternals when formAssociated is not set.

I think we can handle this in a few ways:

  1. Once the UpgradeableConstructor is removed, we could always set the formAssociated = true in the LightningElementConstructor.
  2. We could create some additional logic for now to overwrite the value of form on the ElementInternals object when attachInternals is called and replace it with an exception.
  3. Enforce setting formAssociated when attachInternals is set until UpgradeableConstructor has been removed (throw an error?).

Wanted to get your thoughts on how we should handle this moving forward.

This might be more straightforward or complicated depending on the Locker/LWS pivot implementation.

@nolanlawson
Copy link
Collaborator

@jmsjtu Great catch. I think if we do nothing, then people might end up taking a dependency on elementInterals.form being available regardless of whether they set formAssociated or not.

I hate doing this kind of thing, but my preference here is to return a Proxy object rather than the actual ElementInternals, and to mimic the browser behavior when the form getter is called. Sadly I cannot think of a better solution.

Are there any other properties on elementInternals that behave this way?

@jmsjtu
Copy link
Member Author

jmsjtu commented Sep 15, 2023

Discussed with @nolanlawson and we will need to create a proxy for the following instance properties:

  • ElementInternals.form
  • ElementInternals.willvalidate
  • ElementInternals.validity
  • ElementInternals.validationMessage
  • ElementInternals.states

Additionally, there's currently a bug in Safari with formAssociated causing non-focusable elements to become focusable.

This is causing a karma test failure.

@AllanOricil
Copy link
Contributor

This feature is cool. Nice job

@nolanlawson
Copy link
Collaborator

Additionally, there's currently a bug in Safari with formAssociated causing non-focusable elements to become focusable.

Let's ignore Safari in the test for now. We can also add a comment with a TODO saying to keep an eye on the WebKit bug.

@@ -80,8 +80,27 @@ describe('LightningElement.focus', () => {
expect(elm.shadowRoot.activeElement).toBeNull();
});

// TODO [#2566]: Firefox behaves differently from Chrome/Safari in this test
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this TODO since #2566 has been fixed.

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

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
Collaborator

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'.

Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

I have a nitpick about types, but overall this looks great to me. :shipit:

@jmsjtu jmsjtu changed the title feat: enable face lifecycle methods in lwc feat: enable face lifecycle methods and wrap ElementInternals in a proxy Sep 29, 2023
@@ -2,7 +2,7 @@
"files": [
{
"path": "packages/@lwc/engine-dom/dist/index.js",
"maxSize": "21KB"
"maxSize": "21.2KB"
Copy link
Contributor

Choose a reason for hiding this comment

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

image

@jmsjtu jmsjtu merged commit a03f44b into master Sep 29, 2023
2 checks passed
@jmsjtu jmsjtu deleted the jtu/face-callbacks branch September 29, 2023 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants