fix(engine): relatedTarget might not be present #857
Conversation
| @@ -146,6 +146,89 @@ describe('events', () => { | |||
| expect(dispatched).toHaveLength(0); | |||
| }); | |||
| }); | |||
| describe('relatedTarget', () => { | |||
| it('should report correct the retargeted value', () => { | |||
There was a problem hiding this comment.
sanity unit for relatedTarget, but we do have a bunch of integrations, not sure why not having units. that's a question for @davidturissini, in any case, it is fine.
| const e = new CustomEvent('focus'); | ||
| elm.shadowRoot.querySelector('input').dispatchEvent(e); | ||
| return Promise.resolve().then(() => { | ||
| expect(relatedTarget).toBe(undefined); |
There was a problem hiding this comment.
testing that a custom event named focus should not have a relatedTarget accessor (returning undefined)
| @@ -209,9 +292,6 @@ describe('events', () => { | |||
| handleClick(evt) { | |||
| // event handler is here to trigger patching of the event | |||
| } | |||
| render() { | |||
There was a problem hiding this comment.
duplicated render method
| @@ -251,9 +331,6 @@ describe('events', () => { | |||
| handleClick(evt) { | |||
| // event handler is here to trigger patching of the event | |||
| } | |||
| render() { | |||
There was a problem hiding this comment.
duplicated render method
| @@ -47,52 +47,57 @@ type ComposableEvent = (Event & { | |||
| composed: boolean | |||
| }); | |||
|
|
|||
| const EventPatchDescriptors: PropertyDescriptorMap = { | |||
There was a problem hiding this comment.
instead of having this as static, we have to define it per event that is being patched, so we can inspect the original event, and provide relatedTarget if needed.
There was a problem hiding this comment.
basically, this entire block was moved down into the patchEvent() method.
| } | ||
| // not all events implement the relatedTarget getter, that's why we need to extract it from the instance | ||
| // Note: we can't really use the super here because of issues with the typescript transpilation for accessors | ||
| const originalRelatedTargetDescriptor = getPropertyDescriptor(event, 'relatedTarget'); |
There was a problem hiding this comment.
extracting the original descriptor from the proto chain (notice that it is our utility getPropertyDescriptor).
There was a problem hiding this comment.
the rest is pretty much the same, just moving things to the patch method.
|
@davidturissini I'm not sure if the same issue affects our delegateFocus madness because those events we add them a the lower level, but they might be triggered by the user somehow. Maybe just ignoring those that are not trusted (isTrusted), or something to make sure that we only listen for those keyboard or mouse interactions. |
Benchmark resultsBase commit: lwc-engine-benchmark
|
There was a problem hiding this comment.
This will probably work for us right now, at least it will not throw, I did a quick code search and so far we only are using that Aura blur event for tests, but ideally we should try to figure out why $A.fireDomEvent('blur') is posing as a focusout event instead.
| const originalRelatedTargetDescriptor = getPropertyDescriptor(event, 'relatedTarget'); | ||
| defineProperties(event, { | ||
| relatedTarget: { | ||
| get(this: ComposableEvent): EventTarget | null | undefined { |
Yeah, I was worried about this in the beginning as well. |
Details
Fixes #854
During the retargeting process for relatedTarget, we were assuming that all events will have that accessor, which is incorrect, and triggers an "Invalid Invocation" when attempting to extract the relatedTarget of an event that doesn't have that getter.
Does this PR introduce a breaking change?