Skip to content
Snippets Groups Projects
Commit b1cba4da authored by Jack Reed's avatar Jack Reed Committed by Chris Beer
Browse files

Remove RootRef and fix scrolling entire page

Fixes some underlying inconsistencies with scrolling behavior including:

 - Making sure that the ScrollTo component is the child
 - Removing RootRef which is removed from Material-UI and deprecated in
React.StrictMode
 - While scrollIntoView is awesome, it scrolls the entire page even when
in an iframe which can cause unexpected jumping.

Fixes #3302
parent 152afd22
Branches
Tags
No related merge requests found
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<meta name="theme-color" content="#000000">
<title>Mirador in an iframe</title>
</head>
<body>
<div style="width: 100%; height: 500px; background-color: green"></div>
<div style="width: 100%; height: 500px; background-color: red"></div>
<iframe src="./index.html" width="100%" height="600px"></iframe>
<div style="width: 100%; height: 500px; background-color: green"></div>
</body>
</html>
......@@ -20,20 +20,20 @@ describe('ScrollTo', () => {
const scrollToElAboveBoundingRect = { bottom: -200, top: -300 };
const scrollToElBelowBoundingRect = { bottom: 601, top: 501 };
const visibleScrollToElBoundingRect = { bottom: 300, top: 200 };
const containerBoundingRect = { bottom: 500, top: 0 };
const containerBoundingRect = { bottom: 500, height: 440, top: 0 };
it('wraps the given children in a RootRef element', () => {
it('wraps the given children in a div element', () => {
wrapper = createWrapper();
expect(wrapper.find('RootRef').length).toBe(1);
expect(wrapper.find('RootRef').children().text()).toEqual('Child Prop');
expect(wrapper.find('div').length).toBe(1);
expect(wrapper.find('div').children().text()).toEqual('Child Prop');
});
describe('when updating the scrollTo prop', () => {
describe('when setting from true to false', () => {
it('does not scroll to the selected element', () => {
const scrollIntoView = jest.fn();
jest.spyOn(ScrollTo.prototype, 'elementToScrollTo').mockImplementation(() => ({ scrollIntoView }));
const scrollTo = jest.fn();
jest.spyOn(ScrollTo.prototype, 'elementToScrollTo').mockImplementation(() => ({ offsetTop: 450 }));
jest.spyOn(ScrollTo.prototype, 'scrollabelContainer').mockImplementation(() => ({ scrollTo }));
jest.spyOn(ScrollTo.prototype, 'containerBoundingRect').mockImplementation(() => ({
...containerBoundingRect,
}));
......@@ -44,14 +44,15 @@ describe('ScrollTo', () => {
wrapper.setProps({ scrollTo: false });
// It is called once when initially rendered w/ true
expect(scrollIntoView).not.toHaveBeenCalledTimes(2);
expect(scrollTo).not.toHaveBeenCalledTimes(2);
});
});
describe('when set from false to true', () => {
it('scrolls to the selected element when it is hidden above the container', () => {
const scrollIntoView = jest.fn();
jest.spyOn(ScrollTo.prototype, 'elementToScrollTo').mockImplementation(() => ({ scrollIntoView }));
const scrollTo = jest.fn();
jest.spyOn(ScrollTo.prototype, 'elementToScrollTo').mockImplementation(() => ({ offsetTop: 450 }));
jest.spyOn(ScrollTo.prototype, 'scrollabelContainer').mockImplementation(() => ({ scrollTo }));
jest.spyOn(ScrollTo.prototype, 'containerBoundingRect').mockImplementation(() => ({
...containerBoundingRect,
}));
......@@ -61,12 +62,13 @@ describe('ScrollTo', () => {
wrapper = createWrapper({ scrollTo: false });
wrapper.setProps({ scrollTo: true });
expect(scrollIntoView).toHaveBeenCalledWith({ block: 'center' });
expect(scrollTo).toHaveBeenCalledWith(0, 230);
});
it('scrolls to the selected element when it is hidden above the container', () => {
const scrollIntoView = jest.fn();
jest.spyOn(ScrollTo.prototype, 'elementToScrollTo').mockImplementation(() => ({ scrollIntoView }));
const scrollTo = jest.fn();
jest.spyOn(ScrollTo.prototype, 'elementToScrollTo').mockImplementation(() => ({ offsetTop: 450 }));
jest.spyOn(ScrollTo.prototype, 'scrollabelContainer').mockImplementation(() => ({ scrollTo }));
jest.spyOn(ScrollTo.prototype, 'containerBoundingRect').mockImplementation(() => ({
...containerBoundingRect,
}));
......@@ -76,12 +78,13 @@ describe('ScrollTo', () => {
wrapper = createWrapper({ scrollTo: false });
wrapper.setProps({ scrollTo: true });
expect(scrollIntoView).toHaveBeenCalledWith({ block: 'center' });
expect(scrollTo).toHaveBeenCalledWith(0, 230);
});
it('does not scroll to the selected element when it is visible', () => {
const scrollIntoView = jest.fn();
jest.spyOn(ScrollTo.prototype, 'elementToScrollTo').mockImplementation(() => ({ scrollIntoView }));
const scrollTo = jest.fn();
jest.spyOn(ScrollTo.prototype, 'elementToScrollTo').mockImplementation(() => ({ offsetTop: 450 }));
jest.spyOn(ScrollTo.prototype, 'scrollabelContainer').mockImplementation(() => ({ scrollTo }));
jest.spyOn(ScrollTo.prototype, 'containerBoundingRect').mockImplementation(() => ({
...containerBoundingRect,
}));
......@@ -91,7 +94,7 @@ describe('ScrollTo', () => {
wrapper = createWrapper({ scrollTo: false });
wrapper.setProps({ scrollTo: true });
expect(scrollIntoView).not.toHaveBeenCalled();
expect(scrollTo).not.toHaveBeenCalled();
});
});
});
......
......@@ -96,20 +96,19 @@ describe('SidebarIndexTableOfContents', () => {
it('toggles branch nodes on click, but not leaf nodes', () => {
const wrapper = createWrapper({ setCanvas, toggleNode });
const treeView = wrapper.children(TreeView).at(0);
const node0 = treeView.childAt(0);
const node0 = treeView.childAt(0).childAt(0);
expect(node0.prop('nodeId')).toBe('0-0');
node0.simulate('click');
node0.simulate('click');
expect(toggleNode).toHaveBeenCalledTimes(2);
const node00 = node0.children().at(0);
const node00 = node0.children().at(0).childAt(0);
expect(node00.prop('nodeId')).toBe('0-0-0');
node00.simulate('click');
node00.simulate('click');
expect(toggleNode).toHaveBeenCalledTimes(2);
const node1 = treeView.childAt(1);
const node1 = treeView.childAt(1).childAt(0);
expect(node1.prop('nodeId')).toBe('0-1');
node1.simulate('click');
expect(toggleNode).toHaveBeenCalledTimes(3);
......@@ -123,14 +122,14 @@ describe('SidebarIndexTableOfContents', () => {
});
const treeView = wrapper.children(TreeView).at(0);
const node0 = treeView.childAt(0);
const node0 = treeView.childAt(0).childAt(0);
expect(node0.prop('nodeId')).toBe('0-0');
node0.simulate(...createKeydownProps('ArrowLeft'));
expect(toggleNode).toHaveBeenCalledTimes(1);
const node00 = node0.children().at(0);
const node00 = node0.children().at(0).childAt(0);
expect(node00.prop('nodeId')).toBe('0-0-0');
const node1 = treeView.childAt(1);
const node1 = treeView.childAt(1).childAt(0);
expect(node1.prop('nodeId')).toBe('0-1');
node00.simulate(...createKeydownProps('ArrowLeft'));
......@@ -145,16 +144,16 @@ describe('SidebarIndexTableOfContents', () => {
toggleNode,
});
const treeView = wrapper.children(TreeView).at(0);
const node0 = treeView.childAt(0);
const node0 = treeView.childAt(0).childAt(0);
expect(node0.prop('nodeId')).toBe('0-0');
const node00 = node0.children().at(0);
const node00 = node0.children().at(0).childAt(0);
expect(node00.prop('nodeId')).toBe('0-0-0');
node0.simulate(...createKeydownProps('ArrowRight'));
node00.simulate(...createKeydownProps('ArrowRight'));
expect(toggleNode).toHaveBeenCalledTimes(0);
const node1 = treeView.childAt(1);
const node1 = treeView.childAt(1).childAt(0);
expect(node1.prop('nodeId')).toBe('0-1');
node1.simulate(...createKeydownProps('ArrowRight'));
expect(toggleNode).toHaveBeenCalledTimes(1);
......@@ -163,7 +162,7 @@ describe('SidebarIndexTableOfContents', () => {
it('toggles branch nodes (but not leaf nodes) with Space or Enter key', () => {
const wrapper = createWrapper({ setCanvas, toggleNode });
const treeView = wrapper.children(TreeView).at(0);
const node0 = treeView.childAt(0);
const node0 = treeView.childAt(0).childAt(0);
node0.simulate(...createKeydownProps('Enter'));
expect(toggleNode).toHaveBeenCalledTimes(1);
node0.simulate(...createKeydownProps(' '));
......@@ -174,26 +173,26 @@ describe('SidebarIndexTableOfContents', () => {
node0.children().at(0).simulate(...createKeydownProps('Enter'));
node0.children().at(0).simulate(...createKeydownProps(' '));
expect(toggleNode).toHaveBeenCalledTimes(3);
treeView.childAt(1).simulate(...createKeydownProps('Enter'));
treeView.childAt(1).simulate(...createKeydownProps(' '));
treeView.childAt(1).childAt(0).simulate(...createKeydownProps('Enter'));
treeView.childAt(1).childAt(0).simulate(...createKeydownProps(' '));
expect(toggleNode).toHaveBeenCalledTimes(5);
});
it('calls setCanvas only on click for ranges with canvases that do not have children', () => {
const wrapper = createWrapper({ setCanvas, toggleNode });
const treeView = wrapper.children(TreeView).at(0);
const node0 = treeView.childAt(0);
const node0 = treeView.childAt(0).childAt(0);
expect(node0.prop('nodeId')).toBe('0-0');
node0.simulate('click');
expect(setCanvas).toHaveBeenCalledTimes(0);
node0.childAt(0).simulate('click');
node0.childAt(0).childAt(0).simulate('click');
expect(setCanvas).toHaveBeenCalledTimes(1);
node0.childAt(1).simulate('click');
node0.childAt(1).childAt(0).simulate('click');
expect(setCanvas).toHaveBeenCalledTimes(2);
node0.childAt(2).simulate('click');
node0.childAt(2).childAt(0).simulate('click');
expect(setCanvas).toHaveBeenCalledTimes(3);
const node1 = treeView.childAt(1);
const node1 = treeView.childAt(1).childAt(0);
expect(node1.prop('nodeId')).toBe('0-1');
node1.simulate(...createKeydownProps('ArrowRight'));
expect(setCanvas).toHaveBeenCalledTimes(3);
......@@ -206,7 +205,7 @@ describe('SidebarIndexTableOfContents', () => {
windowId: 'w1',
});
const treeView = version2wrapper.children(TreeView).at(0);
const node3 = treeView.childAt(3);
const node3 = treeView.childAt(3).childAt(0);
expect(node3.prop('nodeId')).toBe('0-3');
node3.simulate('click');
expect(setCanvas).toHaveBeenCalledWith('w1', 'http://foo.test/1/canvas/c11');
......@@ -218,19 +217,18 @@ describe('SidebarIndexTableOfContents', () => {
windowId: 'w1',
});
const treeViewVersion3 = version3wrapper.children(TreeView).at(0);
const rootNode = treeViewVersion3.childAt(0);
const version3node1 = rootNode.childAt(1);
const rootNode = treeViewVersion3.childAt(0).childAt(0);
const version3node1 = rootNode.childAt(1).childAt(0);
expect(version3node1.prop('nodeId')).toBe('0-0-1');
version3node1.simulate('click');
expect(setCanvas).toHaveBeenLastCalledWith('w1', 'http://foo.test/1/canvas/c7');
const version3node2 = rootNode.childAt(2);
const version3node2 = rootNode.childAt(2).childAt(0);
expect(version3node2.prop('nodeId')).toBe('0-0-2');
version3node2.simulate('click');
expect(setCanvas).toHaveBeenLastCalledWith('w1', 'http://foo.test/1/canvas/c9');
const version3node3 = rootNode.childAt(3);
const version3node3 = rootNode.childAt(3).childAt(0);
expect(version3node3.prop('nodeId')).toBe('0-0-3');
version3node3.simulate('click');
expect(setCanvas).toHaveBeenLastCalledWith('w1', 'http://foo.test/1/canvas/c10');
......@@ -239,7 +237,7 @@ describe('SidebarIndexTableOfContents', () => {
it('does not select a canvas when opening a node with the right arrow key', () => {
const wrapper = createWrapper({ setCanvas, toggleNode });
const treeView = wrapper.children(TreeView).at(0);
const node0 = treeView.childAt(0);
const node0 = treeView.childAt(0).childAt(0);
expect(node0.prop('nodeId')).toBe('0-0');
node0.simulate(...createKeydownProps('ArrowRight'));
expect(setCanvas).toHaveBeenCalledTimes(0);
......@@ -249,7 +247,7 @@ describe('SidebarIndexTableOfContents', () => {
it('does not select a canvas when closing a node with the left arrow key', () => {
const wrapper = createWrapper({ expandedNodeIds: ['0-0'], setCanvas, toggleNode });
const treeView = wrapper.children(TreeView).at(0);
const node0 = treeView.childAt(0);
const node0 = treeView.childAt(0).childAt(0);
expect(node0.prop('nodeId')).toBe('0-0');
node0.simulate(...createKeydownProps('ArrowLeft'));
expect(setCanvas).toHaveBeenCalledTimes(0);
......
......@@ -172,7 +172,10 @@ export class CompanionWindow extends Component {
)
}
</Toolbar>
<Paper className={classes.content} elevation={0}>
<Paper
className={[classes.content, ns('scrollto-scrollable')].join(' ')}
elevation={0}
>
{childrenWithAdditionalProps}
</Paper>
</Rnd>
......
import React, { Component } from 'react';
import PropTypes from 'prop-types';
import RootRef from '@material-ui/core/RootRef';
/**
* ScrollTo ~
......@@ -37,8 +36,9 @@ export class ScrollTo extends Component {
containerBoundingRect() {
const { containerRef } = this.props;
if (!containerRef || !containerRef.current) return {};
return containerRef.current.getBoundingClientRect();
if (!containerRef || !containerRef.current || !containerRef.current.domEl) return {};
return containerRef.current.domEl.getBoundingClientRect();
}
/**
......@@ -58,6 +58,17 @@ export class ScrollTo extends Component {
return this.scrollToRef.current;
}
/**
* The container provided in the containersRef dome structure in which scrolling
* should happen.
*/
scrollabelContainer() {
const { containerRef } = this.props;
if (!containerRef || !containerRef.current || !containerRef.current.domEl) return null;
return containerRef.current.domEl.getElementsByClassName('mirador-scrollto-scrollable')[0];
}
/**
* Determine if the scrollTo element is visible within the given containerRef prop.
* Currently only supports vertical elements but could be extended to support horizontal
......@@ -78,12 +89,14 @@ export class ScrollTo extends Component {
* Scroll to the element if it is set to be scolled and is not visible
*/
scrollToElement() {
const { scrollTo } = this.props;
const { offsetTop, scrollTo } = this.props;
if (!scrollTo) return;
if (!this.elementToScrollTo()) return;
if (this.elementIsVisible()) return;
this.elementToScrollTo().scrollIntoView({ block: 'center' });
if (!this.scrollabelContainer()) return;
const scrollBy = this.elementToScrollTo().offsetTop
- (this.containerBoundingRect().height / 2) + offsetTop;
this.scrollabelContainer().scrollTo(0, scrollBy);
}
/**
......@@ -95,9 +108,9 @@ export class ScrollTo extends Component {
if (!scrollTo) return children;
return (
<RootRef rootRef={this.scrollToRef}>
<div ref={this.scrollToRef}>
{children}
</RootRef>
</div>
);
}
}
......
......@@ -2,7 +2,6 @@ import React, { Component } from 'react';
import PropTypes from 'prop-types';
import Button from '@material-ui/core/Button';
import Chip from '@material-ui/core/Chip';
import RootRef from '@material-ui/core/RootRef';
import Typography from '@material-ui/core/Typography';
import CompanionWindow from '../containers/CompanionWindow';
import SearchPanelControls from '../containers/SearchPanelControls';
......@@ -32,7 +31,6 @@ export class SearchPanel extends Component {
} = this.props;
return (
<RootRef rootRef={this.containerRef}>
<CompanionWindow
ariaLabel={t('searchTitle')}
title={(
......@@ -56,6 +54,7 @@ export class SearchPanel extends Component {
windowId={windowId}
id={id}
titleControls={<SearchPanelControls companionWindowId={id} windowId={windowId} />}
ref={this.containerRef}
>
<SearchResults
containerRef={this.containerRef}
......@@ -72,7 +71,6 @@ export class SearchPanel extends Component {
))
}
</CompanionWindow>
</RootRef>
);
}
}
......
......@@ -49,6 +49,12 @@ export class SidebarIndexList extends Component {
const onClick = () => { setCanvas(windowId, canvas.id); }; // eslint-disable-line require-jsdoc, max-len
return (
<ScrollTo
containerRef={containerRef}
key={`${canvas.id}-${variant}`}
offsetTop={96} // offset for the height of the form above
scrollTo={selectedCanvasIds.includes(canvas.id)}
>
<MenuItem
key={canvas.id}
className={classes.listItem}
......@@ -57,16 +63,10 @@ export class SidebarIndexList extends Component {
button
component="li"
selected={selectedCanvasIds.includes(canvas.id)}
>
<ScrollTo
containerRef={containerRef}
key={`${canvas.id}-${variant}`}
offsetTop={96} // offset for the height of the form above
scrollTo={selectedCanvasIds.includes(canvas.id)}
>
<Item label={canvas.label} canvas={canvases[canvasIndex]} />
</ScrollTo>
</MenuItem>
</ScrollTo>
);
})
}
......
......@@ -66,6 +66,12 @@ export class SidebarIndexTableOfContents extends Component {
}
return (
nodes.map(node => (
<ScrollTo
containerRef={containerRef}
key={`${node.id}-scroll`}
offsetTop={96} // offset for the height of the form above
scrollTo={nodeIdToScrollTo === node.id}
>
<TreeItem
key={node.id}
nodeId={node.id}
......@@ -77,12 +83,6 @@ export class SidebarIndexTableOfContents extends Component {
selected: classes.selected,
}}
label={(
<ScrollTo
containerRef={containerRef}
key={`${node.id}-scroll`}
offsetTop={96} // offset for the height of the form above
scrollTo={nodeIdToScrollTo === node.id}
>
<div
className={clsx({
[classes.visibleNode]: visibleNodeIds.indexOf(node.id) !== -1,
......@@ -90,7 +90,6 @@ export class SidebarIndexTableOfContents extends Component {
>
{node.label}
</div>
</ScrollTo>
)}
onClick={() => this.selectTreeItem(node)}
onKeyDown={e => this.handleKeyPressed(e, node)}
......@@ -102,6 +101,7 @@ export class SidebarIndexTableOfContents extends Component {
nodeIdToScrollTo,
) : null}
</TreeItem>
</ScrollTo>
))
);
}
......
......@@ -4,7 +4,6 @@ import Tabs from '@material-ui/core/Tabs';
import Tab from '@material-ui/core/Tab';
import Tooltip from '@material-ui/core/Tooltip';
import Button from '@material-ui/core/Button';
import RootRef from '@material-ui/core/RootRef';
import ItemListIcon from '@material-ui/icons/ReorderSharp';
import TocIcon from '@material-ui/icons/SortSharp';
import ThumbnailListIcon from '@material-ui/icons/ViewListSharp';
......@@ -91,11 +90,12 @@ export class WindowSideBarCanvasPanel extends Component {
}
return (
<RootRef rootRef={this.containerRef}>
<CompanionWindow
title={t('canvasIndex')}
id={id}
windowId={windowId}
ref={this.containerRef}
otherRef={this.containerRef}
titleControls={(
<>
{
......@@ -153,7 +153,6 @@ export class WindowSideBarCanvasPanel extends Component {
{listComponent}
</div>
</CompanionWindow>
</RootRef>
);
}
}
......
......@@ -4,6 +4,7 @@ import { withTranslation } from 'react-i18next';
import { withStyles } from '@material-ui/core';
import { withSize } from 'react-sizeme';
import { withPlugins } from '../extend/withPlugins';
import { withRef } from '../extend/withRef';
import * as actions from '../state/actions';
import { getCompanionWindow, getThemeDirection, getWindowConfig } from '../state/selectors';
import { CompanionWindow } from '../components/CompanionWindow';
......@@ -130,6 +131,7 @@ const styles = theme => ({
});
const enhance = compose(
withRef(),
withTranslation(),
withStyles(styles),
withSize(),
......
import React, { forwardRef } from 'react';
/** */
export const withRef = () => (Component) => {
const WithRefs = forwardRef((props, ref) => (
<Component innerRef={ref} {...props} />
));
return WithRefs;
};
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment