Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Panel, Modal, Overlay: Optional prop that disables bodyscroll block on touch devices #11339

Merged
merged 8 commits into from Dec 19, 2019
Merged

Panel, Modal, Overlay: Optional prop that disables bodyscroll block on touch devices #11339

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
00c16da
modal, panel, overlay: ios allow bodyscroll
Nov 28, 2019
0b8a5f3
Change files
Nov 28, 2019
a4e0df6
Merge branch 'master' into master
Dec 3, 2019
8bdd921
formatting
Dec 4, 2019
329e9cb
renamed ios -> touch, new event handler instead of boolean parameter
Dec 5, 2019
23e5068
Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…
Dec 9, 2019
4fb954d
Update change files
Dec 11, 2019
2f93c87
Update api description
Dec 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions change/@uifabric-utilities-2019-11-28-22-34-32-master.json
View file
Open in desktop
@@ -0,0 +1,8 @@
{
"type": "minor",
"comment": "scroll.ts: extended event handler provided by _makeElementScrollAllower with allowIosOverscroll argument to make blocking of body scroll on touch devices optional",
"packageName": "@uifabric/utilities",
"email": "dmitriy.ravdin@siemens.com",
"commit": "00c16da9f0d9ee3de29c6de6a0aa32d50397cdcc",
"date": "2019-11-28T21:34:32.268Z"
}
8 changes: 8 additions & 0 deletions change/office-ui-fabric-react-2019-11-28-22-34-32-master.json
View file
Open in desktop
@@ -0,0 +1,8 @@
{
"type": "minor",
"comment": "modal, panel, overlay components: added optional prop allowIosBodyScroll to allow body scroll on touch devices",
"packageName": "office-ui-fabric-react",
"email": "dmitriy.ravdin@siemens.com",
"commit": "00c16da9f0d9ee3de29c6de6a0aa32d50397cdcc",
"date": "2019-11-28T21:34:30.900Z"
}
4 changes: 4 additions & 0 deletions packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md
View file
Open in desktop
Expand Up @@ -5531,6 +5531,7 @@ export interface IModal {

// @public (undocumented)
export interface IModalProps extends React.ClassAttributes<ModalBase>, IWithResponsiveModeState, IAccessiblePopupProps {
allowIosBodyScroll?: boolean;
Copy link
Contributor

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.

Copy link
Contributor Author

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

className?: string;
componentRef?: IRefObject<IModal>;
containerClassName?: string;
Expand Down Expand Up @@ -5739,6 +5740,7 @@ export interface IOverlay {

// @public (undocumented)
export interface IOverlayProps extends React.HTMLAttributes<HTMLElement> {
allowIosBodyScroll?: boolean;
className?: string;
componentRef?: IRefObject<IOverlay>;
isDarkThemed?: boolean;
Expand Down Expand Up @@ -5818,6 +5820,7 @@ export interface IPanelHeaderRenderer extends IRenderFunction<IPanelProps> {
//
// @public (undocumented)
export interface IPanelProps extends React.HTMLAttributes<PanelBase> {
allowIosBodyScroll?: boolean;
className?: string;
closeButtonAriaLabel?: string;
// @deprecated
Expand Down Expand Up @@ -8219,6 +8222,7 @@ export const Overlay: React.StatelessComponent<IOverlayProps>;

// @public (undocumented)
export class OverlayBase extends BaseComponent<IOverlayProps, {}> {
constructor(props: IOverlayProps);
// (undocumented)
componentDidMount(): void;
// (undocumented)
Expand Down
15 changes: 13 additions & 2 deletions packages/office-ui-fabric-react/src/components/Modal/Modal.base.tsx
View file
Open in desktop
Expand Up @@ -47,6 +47,7 @@ export class ModalBase extends BaseComponent<IModalProps, IDialogState> implemen
private _scrollableContent: HTMLDivElement | null;
private _lastSetX: number;
private _lastSetY: number;
private _allowIosBodyScroll: boolean;
private _hasRegisteredKeyUp: boolean;

constructor(props: IModalProps) {
Expand All @@ -66,6 +67,9 @@ export class ModalBase extends BaseComponent<IModalProps, IDialogState> implemen
this._warnDeprecations({
onLayerDidMount: 'layerProps.onLayerDidMount'
});

const { allowIosBodyScroll = false } = this.props;
this._allowIosBodyScroll = allowIosBodyScroll;
}

// tslint:disable-next-line function-name
Expand Down Expand Up @@ -238,7 +242,14 @@ export class ModalBase extends BaseComponent<IModalProps, IDialogState> implemen
onDismiss={onDismiss}
>
<div className={classNames.root}>
{!isModeless && <Overlay isDarkThemed={isDarkOverlay} onClick={isBlocking ? undefined : (onDismiss as any)} {...overlay} />}
{!isModeless && (
<Overlay
isDarkThemed={isDarkOverlay}
onClick={isBlocking ? undefined : (onDismiss as any)}
allowIosBodyScroll={this._allowIosBodyScroll}
{...overlay}
/>
)}
{dragOptions ? (
<DraggableZone
handleSelector={dragOptions.dragHandleSelector || `.${classNames.main.split(' ')[0]}`}
Expand Down Expand Up @@ -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);
Copy link
Contributor

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?

Copy link
Contributor Author

@laiprorus laiprorus Dec 5, 2019 •
edited

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.

Copy link
Contributor Author

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.

} else {
this._events.off(this._scrollableContent);
}
Expand Down
6 changes: 6 additions & 0 deletions packages/office-ui-fabric-react/src/components/Modal/Modal.types.ts
View file
Open in desktop
Expand Up @@ -153,6 +153,12 @@ export interface IModalProps extends React.ClassAttributes<ModalBase>, IWithResp
* The options to make the modal draggable
*/
dragOptions?: IDragOptions;

/**
* Allow body scroll on content and overlay. Changing after mounting has no effect.
* @defaultvalue false
*/
allowIosBodyScroll?: boolean;
}

/**
Expand Down
13 changes: 11 additions & 2 deletions packages/office-ui-fabric-react/src/components/Overlay/Overlay.base.tsx
View file
Open in desktop
Expand Up @@ -5,12 +5,21 @@ import { IOverlayProps, IOverlayStyleProps, IOverlayStyles } from './Overlay.typ
const getClassNames = classNamesFunction<IOverlayStyleProps, IOverlayStyles>();

export class OverlayBase extends BaseComponent<IOverlayProps, {}> {
private _allowIosBodyScroll: boolean;

constructor(props: IOverlayProps) {
super(props);

const { allowIosBodyScroll = false } = this.props;
this._allowIosBodyScroll = allowIosBodyScroll;
}

public componentDidMount(): void {
disableBodyScroll();
!this._allowIosBodyScroll && disableBodyScroll();
}

public componentWillUnmount(): void {
enableBodyScroll();
!this._allowIosBodyScroll && enableBodyScroll();
}

public render(): JSX.Element {
Expand Down
6 changes: 6 additions & 0 deletions packages/office-ui-fabric-react/src/components/Overlay/Overlay.types.ts
View file
Open in desktop
Expand Up @@ -39,6 +39,12 @@ export interface IOverlayProps extends React.HTMLAttributes<HTMLElement> {
isDarkThemed?: boolean;

onClick?: () => void;

/**
* Allow body scroll on ios. Changing after mounting has no effect.
* @defaultvalue false
*/
allowIosBodyScroll?: boolean;
}

/**
Expand Down
9 changes: 7 additions & 2 deletions packages/office-ui-fabric-react/src/components/Panel/Panel.base.tsx
View file
Open in desktop
Expand Up @@ -45,6 +45,7 @@ export class PanelBase extends BaseComponent<IPanelProps, IPanelState> implement
private _classNames: IProcessedStyleSet<IPanelStyles>;
private _scrollableContent: HTMLDivElement | null;
private _animationCallback: number | null = null;
private _allowIosBodyScroll: boolean;

public static getDerivedStateFromProps(nextProps: Readonly<IPanelProps>, prevState: Readonly<IPanelState>): Partial<IPanelState> | null {
if (nextProps.isOpen === undefined) {
Expand All @@ -68,6 +69,9 @@ export class PanelBase extends BaseComponent<IPanelProps, IPanelState> implement
constructor(props: IPanelProps) {
super(props);

const { allowIosBodyScroll = false } = this.props;
this._allowIosBodyScroll = allowIosBodyScroll;

this._warnDeprecations({
ignoreExternalFocusing: 'focusTrapZoneProps',
forceFocusInsideTrap: 'focusTrapZoneProps',
Expand Down Expand Up @@ -169,7 +173,7 @@ export class PanelBase extends BaseComponent<IPanelProps, IPanelState> implement
type
});

const { _classNames } = this;
const { _classNames, _allowIosBodyScroll } = this;

let overlay;
if (isBlocking && isOpen) {
Expand All @@ -178,6 +182,7 @@ export class PanelBase extends BaseComponent<IPanelProps, IPanelState> implement
className={_classNames.overlay}
isDarkThemed={false}
onClick={isLightDismiss ? onLightDismissClick : undefined}
allowIosBodyScroll={_allowIosBodyScroll}
{...overlayProps}
/>
);
Expand Down Expand Up @@ -268,7 +273,7 @@ export class PanelBase extends BaseComponent<IPanelProps, IPanelState> implement
// Allow the user to scroll within the panel but not on the body
private _allowScrollOnPanel = (elt: HTMLDivElement | null): void => {
if (elt) {
allowScrollOnElement(elt, this._events);
allowScrollOnElement(elt, this._events, this._allowIosBodyScroll);
} else {
this._events.off(this._scrollableContent);
}
Expand Down
6 changes: 6 additions & 0 deletions packages/office-ui-fabric-react/src/components/Panel/Panel.types.ts
View file
Open in desktop
Expand Up @@ -225,6 +225,12 @@ export interface IPanelProps extends React.HTMLAttributes<PanelBase> {
* @deprecated Serves no function.
*/
componentId?: string;

/**
* Allow body scroll on content and overlay. Changing after mounting has no effect.
* @defaultvalue false
*/
allowIosBodyScroll?: boolean;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/utilities/etc/utilities.api.md
View file
Open in desktop
Expand Up @@ -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;
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.


// @public
export const anchorProperties: string[];
Expand Down
17 changes: 14 additions & 3 deletions packages/utilities/src/scroll.ts
View file
Open in desktop
Expand Up @@ -32,7 +32,7 @@ const _makeElementScrollAllower = () => {

// prevent the body from scrolling when the user attempts
// to scroll past the top or bottom of the element
const _preventOverscrolling = (event: TouchEvent): void => {
const _preventOverscrolling = (event: TouchEvent, allowIosOverscroll: boolean): void => {
// only respond to a single-finger touch
if (event.targetTouches.length !== 1) {
return;
Expand All @@ -53,6 +53,10 @@ const _makeElementScrollAllower = () => {
_element = scrollableParent;
}

if (allowIosOverscroll) {
return;
}

// if the element is scrolled to the top,
// prevent the user from scrolling up
if (_element.scrollTop === 0 && clientY > 0) {
Expand All @@ -66,13 +70,20 @@ const _makeElementScrollAllower = () => {
}
};

return (element: HTMLElement | null, events: EventGroup): void => {
return (element: HTMLElement | null, events: EventGroup, allowIosOverscroll: boolean = false): void => {
if (!element) {
return;
}

events.on(element, 'touchstart', _saveClientY, { passive: false });
events.on(element, 'touchmove', _preventOverscrolling, { passive: false });
events.on(
element,
'touchmove',
(event: TouchEvent): void => {
_preventOverscrolling(event, allowIosOverscroll);
},
{ passive: false }
);

_element = element;
};
Expand Down