-
Notifications
You must be signed in to change notification settings - Fork 55
fix(whatInput): cleanup previously added event listeners #2127
Conversation
if (eventTarget.PointerEvent) { | ||
eventTarget.removeEventListener('pointerdown', setInput) | ||
// @ts-ignore | ||
} else if (window.MSPointerEvent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still thinking why here there is window
and not eventTarget
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am replacing this. @jurokapsiar @layershifter any comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is basically a platform check - saying "does this browser have MSPointerEvent?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will revert it then, but shouldn't the check above mean similarly? eventTarget.PointerEvent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, it would be great if @jurokapsiar could check it as well as he originally added multi-window support
</Provider>, | ||
) | ||
|
||
expect(addEventListener).toHaveBeenCalledTimes(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
black magic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok ok will add a comment why :))))
@@ -87,14 +89,14 @@ const addListeners = (eventTarget: Window) => { | |||
// `pointermove`, `MSPointerMove`, `mousemove` and mouse wheel event binding | |||
// can only demonstrate potential, but not actual, interaction | |||
// and are treated separately | |||
const options = supportsPassive ? { passive: true, useCapture: true } : true | |||
const options = supportsPassive ? { passive: true, capture: true } : true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* cleanup event listeners * type check improvement * replace window occurance * add tests * changelog * improve tests * revert change of window
Fixes #2109
Counting on
target[whatInputInitialized]
the number of Providers requesting to setup the custom-target whatInput.To cleanup, in Provider
componentWillUnmount
we will attempt to remove listeners. If there are still Providers running (target[whatInputInitialized] > 1
) then we will decrement that value. If it's the last one, then we perform cleanup.Extra:
whatInput.ts
I changeduseCapture
withcapture
(line 92). According to the docs that is the correct property if we pass the information inside theoptions
object params.To Do:
Microsoft Reviewers: Open in CodeFlow