feat: apply proxy to ElementInternals object
#3742
Conversation
| return typeof internalsPropertyValue === 'function' | ||
| ? internalsPropertyValue.bind(target) | ||
| : internalsPropertyValue; |
There was a problem hiding this comment.
This is so function invocations like ElementInternals.setFormValue('on', 'checked') is able to be invoked with the correct context (ElementInternals instance).
There was a problem hiding this comment.
TIL. I assumed Proxies just handled the binding for you when you do foo.bar().
There was a problem hiding this comment.
I looked into it, and it seems proxies should handle the this properly.
I think what's happening here is that the browser has some internal logic that is throwing TypeError: Illegal invocation when using the proxy. The bind is a nice workaround for this, although technically it does change the semantics. (E.g. const foo = internals.checkValidity && foo() should not work.) However, this is definitely an edge case.
Googling didn't turn up much, but yeah, seems like bind is the typical workaround.
| return attachInternals(elm); | ||
| const internals = attachInternals(elm); | ||
| // #TODO[2970]: remove proxy once `UpgradeableConstructor` has been removed | ||
| return createElementInternalsProxy(internals, Boolean(formAssociated)); |
There was a problem hiding this comment.
Note Boolean(formAssociated) will return true for a false string but that's how it works with vanilla web components too.
|
|
||
| if (typeof ElementInternals !== 'undefined') { | ||
| // Verify ElementInternals proxy getter throws error. | ||
| it('form-related operations and attributes should throw DOMException for non-form-associated custom elements.', () => { |
There was a problem hiding this comment.
Added these sanity tests just to confirm that form-associated behavior works as expected with the proxy.
packages/@lwc/engine-core/src/framework/base-lightning-element.ts
Outdated
Show resolved
Hide resolved
| throw new DOMException( | ||
| `Failed to execute '${key}' on 'ElementInternals': The target element is not a form-associated custom element.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
This error may be worth extracting out into a shared utility.
| return typeof internalsPropertyValue === 'function' | ||
| ? internalsPropertyValue.bind(target) | ||
| : internalsPropertyValue; |
There was a problem hiding this comment.
TIL. I assumed Proxies just handled the binding for you when you do foo.bar().
...-karma/test/component/LightningElement.attachInternals/elementInternals/sanity/index.spec.js
Show resolved
Hide resolved
| // Firefox does not support ARIAMixin inside ElementInternals. | ||
| // Check to see if ARIAMixin value is defined on ElementInternals before | ||
| // testing accessibility. | ||
| if (Object.prototype.hasOwnProperty.call(window.ElementInternals.prototype, 'ariaAtomic')) { |
There was a problem hiding this comment.
This may change with Firefox 119. Might be worth testing this in Firefox Nightly.
There was a problem hiding this comment.
You're right it's there in Firefox Nightly! Added a TODO to remove this check once it's officially supported.
There was a problem hiding this comment.
I'm more worried about Fiefox having partial support and it breaking some of our tests.
Do they pass our tests fully?
There was a problem hiding this comment.
I'll need to take a deeper look but at first glance it looks like Firefox nightly breaks all the aria polyfill tests.
The ARIAMixin tests on ElementInternals pass though!
There was a problem hiding this comment.
Opened an issue: #3753
…tu/element-internals-proxy
…tu/element-internals-proxy
Details
This PR wraps the
ElementInternalsobject in a proxy object to prevent LWCs that do not set theformAssociatedvalue from being form-associated when placed inside a form.See comment for more background.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-13818672