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.

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

Fire 'onLayout' when elements are resized #848

Closed
wants to merge 11 commits into from
76 changes: 62 additions & 14 deletions packages/react-native-web/src/modules/applyLayout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,29 +9,73 @@

import { canUseDOM } from 'fbjs/lib/ExecutionEnvironment';
import debounce from 'debounce';
import findNodeHandle from '../../exports/findNodeHandle';

const emptyObject = {};
const registry = {};

let id = 1;
const guid = () => `r-${id++}`;

let resizeObserver;
if (canUseDOM) {
const triggerAll = () => {
Object.keys(registry).forEach(key => {
const instance = registry[key];
instance._handleLayout();
if (typeof window.ResizeObserver !== 'undefined') {
resizeObserver = new window.ResizeObserver(entries => {
entries.forEach(({ target }) => {
target._handleLayout && target._handleLayout();
});
});
};
} else {
if (process.env.NODE_ENV !== 'production') {
console.warn(
'onLayout relies on ResizeObserver which is not supported by your browser. ' +
'Please include a polyfill. https://github.com/WICG/ResizeObserver/issues/3. ' +
'Falling back to window.onresize.'
);
}

window.addEventListener('resize', debounce(triggerAll, 16), false);
const triggerAll = () => {
Object.keys(registry).forEach(key => {
const instance = registry[key];
instance._handleLayout();
});
};

window.addEventListener('resize', debounce(triggerAll, 16), false);
}
}

const observe = instance => {
if (resizeObserver) {
const node = findNodeHandle(instance);
node._handleLayout = debounce(instance._handleLayout.bind(instance));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm applying this locally and noticed this. It would be good if we could avoid binding a new function to the DOM node and instead use a pointer (like _onLayoutId) to the instance's method. Did you try that? Also wondering why this function is debounced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, node._onLayoutId and look up the instance in the registry should work.

RE: debouncing

As I said above

Since the measure method you implemented takes this into account already, I decided to use ResizeObserver only to trigger instance._handleLayout calls. For this reason I am debouncing this function now because it triggers a new measurement which can be expensive (correct me if I am wrong here or think that we can or should not debounce).

Measurements will be triggered, whether the user callback is debunced/throttled or not. If we decide to get rid of the debouncing I think that, for the sake of consistency, we should also remove the debouncing here https://github.com/necolas/react-native-web/pull/848/files#diff-fa9d0b1cb86f4e38c30f6685ec719be4R44

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not debouncing measurements but the frequency with which onresize is called

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but as a consequence of that measurements (which happen in _handleLayout) will be delayed to when resize ends.

Also wondering why this function is debounced

That was to keep the behavior consistent otherwise, on resize, ResizeObserver would fire continuously and we'd invoke _handleLayout (measure) without delays.

Let me know if I am missing something.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see. I wonder if it's helpful to debounce at all. I suspect most viewport resizing is discrete - orientation change, etc - rather than the kind of continuous resizing developers might do. And maybe debouncing is better left to app developers who know when they want to use it? There's also the fact that measurement uses rAF, which needs to change too.

I'm working on a branch for FlatList that I'll push up so you can see how this patch interacts with it. I do want to get this in, it's just surfacing a few existing issues related to layout measurement in the lib :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe debouncing is better left to app developers who know when they want to use it?

Yes, that would be ideal. With the current implementation though they could only debounce onLayout (the fn prop), measurements would happen regardless. https://github.com/giuseppeg/react-native-web/blob/9db10f12c4dd23c46adb3f33f86932fd57f1f33a/packages/react-native-web/src/modules/applyLayout/index.js#L124-L132

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. But probably ok. What I'll do is create a feature branch for this block of work, apply a version of this patch, and then we can work out these other details from there. Thanks for taking the time to understand (and help me understand) the details and for being patient!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! Sounds good, I pushed the change you suggested in #848 (comment) I didn't bother rebasing but I can do that if you want.

resizeObserver.observe(node);
} else {
const id = guid();
instance._onLayoutId = id;
registry[id] = instance;
instance._handleLayout();
}
};

const unobserve = instance => {
if (resizeObserver) {
const node = findNodeHandle(instance);
delete node._handleLayout;
resizeObserver.unobserve(node);
} else {
delete registry[instance._onLayoutId];
delete instance._onLayoutId;
}
};

const safeOverride = (original, next) => {
if (original) {
return function prototypeOverride() {
original.call(this);
next.call(this);
/* eslint-disable prefer-rest-params */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I think that it is unnecessary to use the rest operator in this case, this version should perform better than its transpiled equivalent.

original.call(this, arguments);
next.call(this, arguments);
/* eslint-enable prefer-rest-params */
};
}
return next;
Expand All @@ -47,24 +91,28 @@ const applyLayout = Component => {
function componentDidMount() {
this._layoutState = emptyObject;
this._isMounted = true;
this._onLayoutId = guid();
registry[this._onLayoutId] = this;
this._handleLayout();
observe(this);
}
);

Component.prototype.componentDidUpdate = safeOverride(
componentDidUpdate,
function componentDidUpdate() {
this._handleLayout();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually removing this might break backwards compatibility. Happy to fix this if you think it is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

function componentDidUpdate(prevProps) {
if (this.props.onLayout && !prevProps.onLayout) {
observe(this);
} else if (!this.props.onLayout && prevProps.onLayout) {
unobserve(this);
} else if (!resizeObserver) {
this._handleLayout();
}
}
);

Component.prototype.componentWillUnmount = safeOverride(
componentWillUnmount,
function componentWillUnmount() {
this._isMounted = false;
delete registry[this._onLayoutId];
unobserve(this);
}
);

Expand Down
21 changes: 19 additions & 2 deletions website/storybook/1-components/Text/TextScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* @flow
*/

import { View } from 'react-native';
import OnLayoutExample from './examples/onLayout';
import PropChildren from './examples/PropChildren';
import PropNumberOfLines from './examples/PropNumberOfLines';
import PropOnPress from './examples/PropOnPress';
Expand Down Expand Up @@ -132,13 +134,28 @@ const TextScreen = () => (
<DocItem
name="onLayout"
typeInfo="?function"
description={
description={[
<AppText>
Invoked on mount and layout changes with{' '}
<Code>{'{ nativeEvent: { layout: { x, y, width, height } } }'}</Code>, where{' '}
<Code>x</Code> and <Code>y</Code> are the offsets from the parent node.
</AppText>,
<AppText>
NOTE: Behind the hood React Native for Web uses <Code>ResizeObserver</Code> and doesn't
polyfill it when not supported.
</AppText>
}
]}
example={{
render: () => (
<React.Fragment>
<OnLayoutExample style={{ padding: 10 }} />
<OnLayoutExample />
<View>
<OnLayoutExample />
</View>
</React.Fragment>
)
}}
/>

<DocItem
Expand Down
34 changes: 34 additions & 0 deletions website/storybook/1-components/Text/examples/onLayout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* @flow
*/

import React from 'react';
import { Text, TextPropTypes, StyleSheet } from 'react-native';

export default class OnLayoutExample extends React.Component {
static propTypes = {
style: TextPropTypes.style
};

state = {
layoutInfo: {}
};

onLayout = ({ nativeEvent }) => {
this.setState({ layoutInfo: nativeEvent.layout });
};

render() {
return (
<Text onLayout={this.onLayout} style={[styles.root, this.props.style]}>
{JSON.stringify(this.state.layoutInfo)}
</Text>
);
}
}

const styles = StyleSheet.create({
root: {
backgroundColor: '#eee'
}
});
21 changes: 19 additions & 2 deletions website/storybook/1-components/View/ViewScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* @flow
*/

import { View } from 'react-native';
import OnLayoutExample from './examples/onLayout';
import PropPointerEvents from './examples/PropPointerEvents';
import transformExamples from './examples/transforms';
import ZIndexExample from './examples/ZIndex';
Expand Down Expand Up @@ -125,13 +127,28 @@ const ViewScreen = () => (
<DocItem
name="onLayout"
typeInfo="?function"
description={
description={[
<AppText>
Invoked on mount and layout changes with{' '}
<Code>{'{ nativeEvent: { layout: { x, y, width, height } } }'}</Code>, where{' '}
<Code>x</Code> and <Code>y</Code> are the offsets from the parent node.
</AppText>,
<AppText>
NOTE: Behind the hood React Native for Web uses <Code>ResizeObserver</Code> and doesn't
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you include ResizeObserver and a link to the polyfill in the list of recommended polyfills in the "Getting started" docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

polyfill it when not supported.
</AppText>
}
]}
example={{
render: () => (
<React.Fragment>
<OnLayoutExample style={{ padding: 10 }} />
<OnLayoutExample />
<View>
<OnLayoutExample />
</View>
</React.Fragment>
)
}}
/>

<DocItem
Expand Down
34 changes: 34 additions & 0 deletions website/storybook/1-components/View/examples/onLayout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* @flow
*/

import React from 'react';
import { Text, View, ViewPropTypes, StyleSheet } from 'react-native';

export default class OnLayoutExample extends React.Component {
static propTypes = {
style: ViewPropTypes.style
};

state = {
layoutInfo: {}
};

onLayout = ({ nativeEvent }) => {
this.setState({ layoutInfo: nativeEvent.layout });
};

render() {
return (
<View onLayout={this.onLayout} style={[styles.root, this.props.style]}>
<Text>{JSON.stringify(this.state.layoutInfo)}</Text>
</View>
);
}
}

const styles = StyleSheet.create({
root: {
backgroundColor: '#eee'
}
});