fix(react-utilities): drag scrollbar should invoke callback in useOnScrollOutside with extending event type
#28964
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "minor", | ||
| "comment": "fix: update Menu open change event type to include `Event` (as `useOnScrollOutside` adds scroll event listener)", | ||
| "packageName": "@fluentui/react-menu", | ||
| "email": "yuanboxue@microsoft.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "minor", | ||
| "comment": "fix: update type `OpenPopoverEvents` to include `Event` (as `useOnScrollOutside` adds scroll event listener)", | ||
| "packageName": "@fluentui/react-popover", | ||
| "email": "yuanboxue@microsoft.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "patch", | ||
| "comment": "fix: `useOnScrollOutside` should invoke callback on dragging scrollbar", | ||
| "packageName": "@fluentui/react-utilities", | ||
| "email": "yuanboxue@microsoft.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -209,6 +209,7 @@ export type OnOpenChangeData = { open: boolean }; | |
| * The supported events that will trigger open/close of the menu | ||
| */ | ||
| export type OpenPopoverEvents = | ||
| | Event | ||
| | MouseEvent | ||
| | TouchEvent | ||
| | React.FocusEvent<HTMLElement> | ||
|
Comment on lines
211
to
215
Contributor
There was a problem hiding this comment. Unfortunately I think this is a breaking change. Existing
Contributor
There was a problem hiding this comment. Yeah good catch 👍, I tried playing with this a bit more in TS playground and it can indeed break typescript build depending on how users define their onOpenChange handlers https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBDAnmYcDyKB2AFCkBuwUAooZjAM5wC8AUHHAD5ynDn1NwCyEArhcFbsGzACp8AxgAshMWvKQo42KHirU4Abw4MImDGwDCUgIaYA5sABccABTX0WXASKyKAShoA+OPggBLABMAbloAX3kJPQp4PQNMYzNLGjsHHn5BMngxSRksz2ofbXDI6PgwVTAKGxU1FM04OKxEi1Qw0NogA This is actually quite worrying because we will never be able to add new events to our callback types without breaking users 😱 - this looks like a good candidate to discuss in tech sync with the wider team
Contributor
There was a problem hiding this comment. In this case we might want to consider casting internally in the However actually adding new events to types in All options are pretty bad for me, WDYT @behowell?
Contributor
Author
There was a problem hiding this comment. The original implementation is at this commit: b73b2bd
Contributor
There was a problem hiding this comment. Casting to avoid a breaking change in types is still a breaking change; and possibly even worse: it results in extra-confusing runtime bugs when the object isn't the type that was promised. In this case, the event object would be missing any of the properties that are in MouseEvents. I think in practice, it's pretty unlikely that it will cause an actual problem in this case, especially since the existing type includes It might be worth discussing this with the rest of the team to come up with the right fix here. This isn't the first time this has caused a problem. In my recent PR #28951, I had to come up with a "hacky" workaround to this problem, by including the new event on the All of the possible options I can think of are broken in some way, so we'd have to decide what the least-bad option is:
Given all of that, I think option 1 might actually be the least-bad option.
Contributor
There was a problem hiding this comment. Yeah I think that 1. might be the least bad option, but it would require discussion with the wider team to make sure we know about it and the consequences. I would propose that we:
In the interim option 2 is the best case IMO, since build breaks generally block our users from upgrading to fix other issues. I can't as easily think of cases where users rely on knowing exactly what the event type is. However for option 1 it's quite common from what I see in codebases to type the event // 👍 no need to do anything
const onOpenChange: PopoverProps['onOpenChange'] = (e, data) => {/**/}
// 👎 breaking change - I've generally seen this more often since developers generally don't pick from PopoverProps as much
const onOpenChange = (e: React.MouseEvent | React.FocusEvent, data: PopoverOpenData) => {/**/}
Contributor
Author
There was a problem hiding this comment. I created a new PR #29062 using the cast option (option 2) to fix the original issue. Let's discuss how to extend event type this wednesday |
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,16 +1,23 @@ | ||||||
| import * as React from 'react'; | ||||||
| import { useEventCallback } from './useEventCallback'; | ||||||
| import { UseOnClickOutsideOptions } from './useOnClickOutside'; | ||||||
|
|
||||||
| export type UseOnScrollOutsideOptions = Pick<UseOnClickOutsideOptions, 'element' | 'refs' | 'contains' | 'disabled'> & { | ||||||
|
Contributor
There was a problem hiding this comment. As far as I can tell, the goal here is to use
Suggested change
Contributor
There was a problem hiding this comment. I'd personally prefer
|
||||||
| /** | ||||||
| * Called if the scroll is outside the element refs | ||||||
| */ | ||||||
| callback: (ev: Event | MouseEvent | TouchEvent) => void; | ||||||
| }; | ||||||
|
Comment on lines
+5
to
+10
Contributor
There was a problem hiding this comment. Should this have
Contributor
There was a problem hiding this comment. We could add it, but I'm not sure what the use of |
||||||
|
|
||||||
| /** | ||||||
| * @internal | ||||||
| * Utility to perform checks where a scroll/touch event was made outside a component | ||||||
| */ | ||||||
| export const useOnScrollOutside = (options: UseOnScrollOutsideOptions) => { | ||||||
| const { refs, callback, element, disabled, contains: containsProp } = options; | ||||||
|
|
||||||
| const listener = useEventCallback((ev: Event | MouseEvent | TouchEvent) => { | ||||||
| const contains: UseOnScrollOutsideOptions['contains'] = | ||||||
| containsProp || ((parent, child) => !!parent?.contains(child)); | ||||||
|
|
||||||
| const target = ev.composedPath()[0] as HTMLElement; | ||||||
|
|
@@ -28,10 +35,13 @@ export const useOnScrollOutside = (options: UseOnClickOrScrollOutsideOptions) => | |||||
|
|
||||||
| element?.addEventListener('wheel', listener); | ||||||
| element?.addEventListener('touchmove', listener); | ||||||
| // use capture phase because scroll does not bubble | ||||||
| element?.addEventListener('scroll', listener, true); | ||||||
|
|
||||||
| return () => { | ||||||
| element?.removeEventListener('wheel', listener); | ||||||
| element?.removeEventListener('touchmove', listener); | ||||||
| element?.removeEventListener('scroll', listener, true); | ||||||
| }; | ||||||
| }, [listener, element, disabled]); | ||||||
| }; | ||||||
Uh oh!
There was an error while loading. Please reload this page.