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
Panel, Modal, Overlay: Optional prop that disables bodyscroll block on touch devices #11339
Conversation
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: 0a85f594efeaf767da188c80db6bb4ae9bbd4399 (build) |
Component Perf AnalysisNo significant results to display. All results
|
@@ -5531,6 +5531,7 @@ export interface IModal { | |||
|
|||
// @public (undocumented) | |||
export interface IModalProps extends React.ClassAttributes<ModalBase>, IWithResponsiveModeState, IAccessiblePopupProps { | |||
allowIosBodyScroll?: boolean; |
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.
Rename this to allowTouchBodyScroll
or something non-platform specific.
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 good point, i will change it to allowTouchBodyScroll, i think i took the name from _disableIosBodyScroll
@@ -270,7 +281,7 @@ export class ModalBase extends BaseComponent<IModalProps, IDialogState> implemen | |||
// Allow the user to scroll within the modal but not on the body | |||
private _allowScrollOnModal = (elt: HTMLDivElement | null): void => { | |||
if (elt) { | |||
allowScrollOnElement(elt, this._events); | |||
allowScrollOnElement(elt, this._events, this._allowIosBodyScroll); |
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.
What's missing from the current allowScrollOnElement that misses the touch capability? Instead of making a separate, new method to handle touch, can we fill in the missing gaps to handle touch in allowScrollOnElement?
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.
allowScrollOnElement allows scrolling of content within the element and when top or botton of content is reached, events are blocked, this way it prevents overscrolling _preventOverscrolling where body would be scrolled. But this creates an issue, because _preventOverscrolling blocks all touch events. Example is my original issue #10982 where if you are zoomed in on mobile device and open panel, you can not pan around using content of panel because of _preventOverscrolling event handler, this way user is stuck (he cant reach close button for example) and all he can do is scroll panel content. We need to somehow allow the user to scroll the content, but also be able to pan around while zoomed in. I am not even sure it is possible to differentiate touch events that will scroll the body and from ones that will pan your zoomed in view.
My easiest and minimal to implement solution was, to just make blocking happening in _preventOverscrolling optional, that what boolean argument allowIosOverscroll is used for.
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.
With this PR, i could create Panel and Modal components that dont block touch events. This is not perfect solution, as it allows body scrolling behind the overlay, which is a bit irritating, but UI is still intuitively usable. Where in current state, user is stuck in not usable UI.
@@ -18,7 +18,7 @@ export function addDirectionalKeyCode(which: number): void; | |||
export function addElementAtIndex<T>(array: T[], index: number, itemToAdd: T): T[]; | |||
|
|||
// @public | |||
export const allowScrollOnElement: (element: HTMLElement | null, events: EventGroup) => void; | |||
export const allowScrollOnElement: (element: HTMLElement | null, events: EventGroup, allowIosOverscroll?: boolean) => void; |
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.
Adding a boolean arg is usually not recommended - can we circumvent this without a boolean arg?
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.
See: https://github.com/OfficeDev/office-ui-fabric-react/wiki/Pull-request-reviewing-guidance
Avoid using boolean params in method definitions. Boolean params usually mean a conditional if statement will be used which means the caller probably knows too much about the implementation of the method it's calling. It also makes it hard to change the method later. Make 2 methods that do different things instead.
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.
Originaly i tought of this, but this would create duplicate code. It would be just 2 slighty different versions of _makeElementScrollAllower. But now i see if you remove the code that is needed for _preventOverscrolling handler, not much is left and duplicate code will be minimal.
i want to investigate allowScrollOnElement more, especially the single vs multiple finger touch event. It is not clear to me, why _preventOverscrolling says that it only responds to single finger touch events, but zooming (2 finger event) is still partially blocked on Panel content. |
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.
@laiprorus I have pulled down this branch and attempted to use the functionality but haven't been able to observe the fix. I have attempted using the allowTouchBodyScroll={true} in chrome dev tools mobile emulator and on my samsung s7 phone using a codepen but panning and pinch zooming are still locked when the Panel is shown. Perhaps my testing environment or tools are insufficient for testing this feature? Can you provide a gif or video showing the functionality in action, thanks!
@maxwellred panel with allowTouchBodyScroll |
@maxwellred here 2 examples without allowTouchBodyScroll (how it is current fabric). in second, i zoom in into button, open panel, scroll to left until no panel is in view point, and stuck again unable to zoom out or reach close button. in both examples, some touch actions (inside textfield for example) could not be blocked by fabric and are executed. you can see errors in console saying fabric tried to block but it was intervented by browser. |
Here is code of component i used in this example:
|
@laiprorus This is great, thank you so much! Everything looks good to me |
i restricted my solution to only mounting of components. to eliminate possible side effects and bugs, like allowTouchBodyScroll prop being changed during touchmove event. I think respecting prop change after mount can implemented later? |
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.
Thanks for making this change and being so thorough!! LGTM - @JasonGore might have more input on the Modal side
Hi @maxwellred |
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 do have some hesitation about adding another one-dimensional prop (particularly to 3 different component interfaces that already expose their props to one another.) However, given the existing props, I don't think there's a more meaningful way to implement this without refactoring the mobile approach these components take in general. Additionally, duplicating the prop in 3 components does seem to make them easier to use, as well.
Thank you everyone for your feedback and aprroval. Is there anything i could for this PR or should i just wait? |
🎉 Handy links: |
🎉 Handy links: |
Why is it |
This was the behaviour before this PR. |
Pull request checklist
$ yarn change
Description of changes
Adds optional prop "allowTouchBodyScroll" to Panel, Modal and Overlay components. This prop allows body scroll events on touch devices.
This prop disables overscroll blocks in allowScrollOnElement and doesnt call disableBodyScroll on overlay mount.
As this prop is optional, without it being set to true, components work exactly as they were before.
This change is a simple workaround for breaking behaviour described in issue: #10982
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow