Make reading element dimensions work regardless of transforms

[javiergonzalezGenially]

When using the carousel inside a container with a scale transform applied the carousel doesn't work properly because it uses to measure nodes getBoundingClientRect, which returns a rect which is already scaled. The proposed fix (tested working in the playground) uses instead offsetLeft/offsetWidth etc, which are unscaled.

[davidjerleke]

Hi @javiergonzalezGenially,

A friendly reminder: Please start a new discussion before starting any work to avoid doing unnecessary work.

Thank you for your contribution! I don't think this approach will work with a RTL setup? It needs to be modified to work with that. Basically we need to create our own artificial offsetRight value.

Best, David

[javiergonzalezGenially]

Sorry, I didn't know a discussion was necessary.

I just tried adding dir="rtl" to the node: <div class="embla" dir="rtl"> and adding the option direction: 'rtl' and it seems to work correctly. Is there anything else I need to check?

[davidjerleke]

Thanks @javiergonzalezGenially,

One more thing I have in mind at the time of writing: Have you checked if all tests in embla-carousel work as expected? Or do we need to update them?

Best, David

[davidjerleke]

Sorry, I didn't know a discussion was necessary.

@javiergonzalezGenially I have to update the contribution guidelines but the reason is actually there for you. Now this PR/initiative is great and is actually on my TODO, but sometimes devs do significant amount of work on something that isn't aligned with the Embla vision without starting a discussion. So they practically waste their time which could be avoided with a discussion 🙂.

[javiergonzalezGenially]

@davidjerleke just made a commit that fixed the unit tests :)

[davidjerleke]

Thanks a ton @javiergonzalezGenially. I will test this as soon as possible. Thank you very much for contributing ⭐!

[javiergonzalezGenially]

Thanks to you for creating the lib! I don't want to seem too demanding, but if you could release a rc with the fix at some point would be really appreciated ;)

[davidjerleke]

@javiergonzalezGenially sure. I will release an RC after I’ve tested this and I’ve reviewed all code. I think this is a great feature to test with RC before the stable v8 release.

[davidjerleke]

@javiergonzalezGenially I will have to change some stuff on this PR before we can merge this because I found cases where the measurements fail. Especially when setting containScroll: false.

It seems to be caused by the fact that getBoundingClientRect().left returns the left distance relative to the document while node.offsetLeft gives us the left distance relative to its parent. The same goes for the top values.

Best, David

[javiergonzalezGenially]

@davidjerleke sure, maybe you can use https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetParent to get the relative parent element and sum all the offsets until that offsetParent returns null?

[davidjerleke]

@javiergonzalezGenially thanks for the idea! I’m looking into possible solutions. I have to take bundle size, performance, readability and complexity into account so will try to find a good solution.

[javiergonzalezGenially]

Sure, I wasn't talking of anything complex, just something like:

function getDocumentOffset(element) {
  let top = 0, left = 0;
  let curElement = element;
  while (curElement) {
    top += curElement.offsetTop;
    left += curElement.offsetLeft;
    curElement = curElement.offsetParent;
  }
  return { offsetTop: top, offsetLeft: left, offsetWidth: element.offsetWidth, offsetHeight: element.offsetHeight };
}

After that it should be a matter of changing getNodeRect to do

const { offsetTop, offsetLeft, offsetWidth, offsetHeight } = getDocumentOffset(node)

instead of

const { offsetTop, offsetLeft, offsetWidth, offsetHeight } = node

[davidjerleke]

@javiergonzalezGenially thanks for the code snippet. I probably wasn’t clear: I didn’t mean that your suggestion was complex, I just wanted to explain that I want to consider more than one solution, what I will be looking into and their pros and cons.

Sorry if I wasn’t clear 🙂.

Cheers!

[javiergonzalezGenially]

Ah! sure, no problem :)

[davidjerleke]

@javiergonzalezGenially I think this is ready to merge. I will let you know when the experimental RC is out which will be v8.0.0-rc15.

[davidjerleke]

Relates to:

• 346

[davidjerleke]

• Closes: #637.

[davidjerleke]

@javiergonzalezGenially I've released v8.0.0-rc15 which includes this PR.

[javiergonzalezGenially]

Thanks a lot!!