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

borders somehow screw up measure #1151

Closed
MrLoh opened this issue Oct 24, 2018 · 11 comments
Closed

borders somehow screw up measure #1151

MrLoh opened this issue Oct 24, 2018 · 11 comments
Milestone

Comments

@MrLoh
Copy link

MrLoh commented Oct 24, 2018

The problem
There is a weird issue when measuring the position of elements. Somehow borders of surrounding elements are not taken into account in the measurement. Probably because it uses the offset values and not getBoundingClientRect

How to reproduce
I set up this code sandbox which reproduces the issue by showing the discrepancies of the measurements using measure vs. getBoundingClientRect

https://codesandbox.io/s/8klwx0zxml?module=%2Fsrc%2FApp.js

Expected behavior
The measurement should take into account the borders of parent elements

Environment

  • React Native for Web: 0.9.4
  • React: 16.5.2
  • Browser: latest Chrome & Safari

Additional context
This seems to be the code that causes the problem.

const getRect = node => {
const height = node.offsetHeight;
const width = node.offsetWidth;
let left = node.offsetLeft;
let top = node.offsetTop;
node = node.offsetParent;
while (node && node.nodeType === 1 /* Node.ELEMENT_NODE */) {
left += node.offsetLeft - node.scrollLeft;
top += node.offsetTop - node.scrollTop;
node = node.offsetParent;
}
return { height, left, top, width };
};

@necolas
Copy link
Owner

necolas commented Nov 8, 2018

Did you compare this to the values react native returns by using snack.expo.io?

@necolas necolas added the needs: more information Issue is missing actionable information label Nov 8, 2018
@MrLoh
Copy link
Author

MrLoh commented Nov 9, 2018

@necolas It seems that react native cannot measure this at all. I am very confused.
https://snack.expo.io/@mrloh/mad-popcorn

@MrLoh
Copy link
Author

MrLoh commented Nov 9, 2018

sorry, just forgot that I can only measure in onLayout in react-native. I updated the snack and can confirm that react-native measures correctly, the same way that getBoundingClientRect does. So this is indeed a bug here.

@necolas
Copy link
Owner

necolas commented Nov 9, 2018

React Native supports the instance methods too. Please could you check one more thing for me? Apply a scale transform to the View and see if RN reports the data the same way as getBoundingClientRect

@MrLoh
Copy link
Author

MrLoh commented Nov 9, 2018

@necolas no problem, I checked (see updated examples) and it seems like they do. There are some discrepancies in complex cases. If I nest two scaled items, then only the top coordinate exactly matches, the left coordinate somehow get's shifted. I drew some red lines at the measured positions that show you that the scaled left coordinate is actually not correct on either react-native measure or getBoundingClientRect, but oh well, using the getBoundingClientRect coordinates comes much closer to the react native implementation than the current implementation and I think the use case of nested scaled items is extremely strange and should not happen in well written code anyways.

Beware that x and y coordinates get measured differently for getBoundingClientRect. If you take the difference between the parent x-y-positions and the element x-y-positions you will get the same results as the react native measure return value. You can also fix the scale issue with getBoundingClientRect here.

There has been some recent discussion around ReactDOM.findDOMNode it was mentioned as discouraged in the 16.6 release article I believe, but I think this is one of the rare exception cases. See @gaearon's comment here facebook/react#9217

Hope this helps to fix the issue. Thank you a lot for the great library, I'm amazed at the feature parity with react native, that you have achieved, it's really cool to share my component library between react native and web. Maybe I can open source our react native (web) component library some day.

@necolas
Copy link
Owner

necolas commented Nov 9, 2018

Thanks for the details! I'm aware of the findDOMNode deprecation and will look to remove that by the time all the concurrent React stuff ships too.

@MrLoh
Copy link
Author

MrLoh commented Nov 12, 2018

@necolas findDOMNode is not deprecated as stated in the thread, just discouraged for everyday use cases. But I think it'd be fine to use in this case. Better than reimplementing the measuring algorithm.

@necolas
Copy link
Owner

necolas commented Nov 12, 2018

IIRC it's going to be deprecated in strict mode

@MrLoh
Copy link
Author

MrLoh commented Nov 12, 2018

You're right https://reactjs.org/blog/2018/10/23/react-v-16-6.html#deprecations-in-strictmode. Doesn't quite fit with what is stated in the discussion in the issue. But whatever. Well, not so sure how to get a reference on which one could call getBoundingClientRect then.

@necolas
Copy link
Owner

necolas commented Jan 1, 2019

This should be fixed in the future 0.10 release. You can try the above patch/branch that includes the change. From what I can tell, it produces the same values as RN including when transforms are applied. I'll look into whether the values RN produces for native can be extended to include the rest of the data returned by getBoundingClientRect.

@necolas necolas closed this as completed Jan 1, 2019
necolas added a commit that referenced this issue Jan 1, 2019
@MrLoh
Copy link
Author

MrLoh commented Jan 14, 2019

Works for me in 0.10-alpha.2 🎉 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants