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

Possibly returning null from getBox #8

Closed
migueloller opened this issue Mar 20, 2019 · 3 comments · Fixed by #9
Closed

Possibly returning null from getBox #8

migueloller opened this issue Mar 20, 2019 · 3 comments · Fixed by #9
Labels
help wanted Extra attention is needed question Further information is requested

Comments

@migueloller
Copy link

Hi Alex!

Thanks for making this library! Super helpful for all things DOM layout.

There's a pesky issue when measuring an element that has display: none. getComputedStyle will return the used values if the element has display: none and the computed values otherwise. What this means is that calling getBox on an element with display: none will result in an error as the used values could be a length (e.g., em, rem, etc.) or a keyword like auto.

Would you be open to making the signature of getBox (el: Element) => ?BoxModel? I thought of just wrapping the method in a try/catch in our codebase but we measure layout a large amount of times and a try/catch block would slow it down a lot.

There's more info on the quirks of getComputedStyle's resolved values here.

If this is something you're open to, I'd happily open a PR.

@alexreardon
Copy link
Owner

alexreardon commented Mar 29, 2019

Interesting! Perhaps if it is display:none we return 0 for all values? rather than null?

🤔

@alexreardon alexreardon added help wanted Extra attention is needed question Further information is requested labels Mar 29, 2019
@migueloller
Copy link
Author

migueloller commented Mar 29, 2019

That's a possibility. I do wonder if that's the only case and is the "correct" source of truth for something like this. I tried finding an exhaustive list of what would cause this but had no luck finding one. Perhaps more research is required, or a dive into the spec.

If display: none is the only case for this, returning 0 for all values would definitely work, though! As people report other instances of similar issues, support for those could be added.

@alexreardon
Copy link
Owner

I think returning 0 when we detect that there is no px prefix is a reasonable way forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants