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

Precision problem with fractional height #811

Closed
robsch opened this issue Apr 28, 2020 · 8 comments
Closed

Precision problem with fractional height #811

robsch opened this issue Apr 28, 2020 · 8 comments
Labels

Comments

@robsch
Copy link

robsch commented Apr 28, 2020

Using version v4.2.10 in Firefox I get a problem if the height of an element has a fractional part (like 750.67px). I'm using taggedElement, but I don't know if it is relevant. It seems to me that the problem does not exist in Chrome. In Firefox (75, desktop, Linux) this results in a too small iframe, what makes the content scrollable. However, what is a problem/bug in Firefox, no scrollbar appears. But it is scrollable! Very little of course (0.67px). And this causes a poor "scroll experience". I currently see this in the responsive design mode of Firefox since I'm currently evaluating the library.

The problem is caused by the usage of parseInt() and/or by Firefox using fractional height.

I know this is a weak bug report. But you hopefully can use it if I tell you how I solve it. At least the behaviour is not there if I do the following.

In iframeResizer.js in line 162 I add one pixel to the height:

function processMsg() {
  var data = msg.substr(msgIdLen).split(':')
  var height = data[1] ? parseInt(data[1], 10) + 1: 0; // <------------------ Here
  var iframe = settings[data[0]] && settings[data[0]].iframe;
  var compStyle = getComputedStyle(iframe)

  return {
    iframe: iframe,
    id: data[0],
    height: height + getPaddingEnds(compStyle) + getBorderEnds(compStyle),
    width: data[2],
    type: data[3]
  }
}

Can you consider that problem? Is it the same as in #727? Is there a workaround by configuring the lib?

@davidjbradshaw
Copy link
Owner

Rather than add one, I think it might be better to use parseFloat and Math.ceil to fix this, so we only bump up 1px when we really need to. Is this only an issue where the fraction is greater than 0.5px? In which case maybe Math.round would be better.

If you want to have a play around, I'd be happy to look at a PR to address this.

@robsch
Copy link
Author

robsch commented Apr 28, 2020

Using different things from Github, using it for years ... but never done a PR. I really should have a look for it.

But I don't know if using parseFloat with Math.ceil or Math.round is the solution. If so you would change it on your own, right? What are your concerns?

@davidjbradshaw
Copy link
Owner

My concern is we should not add 1px if it is not needed. as that might break existing sites using this code. What we want is 58px => 58 and 58.6px => 59.

Yes it’s about time you did a PR, I have limited free time to investigate this.

@davidjbradshaw
Copy link
Owner

@robsch
Copy link
Author

robsch commented May 4, 2020

That's what I also though: should the calculation take place at the sender or the receiver? I cannot estimate that.

@robsch
Copy link
Author

robsch commented May 4, 2020

I have created this fiddle, without iframe-resizer, to see the effect when the iframe content is fractional: https://jsfiddle.net/robsch/tp5fgwuy/

@robsch
Copy link
Author

robsch commented May 4, 2020

Okay, I think it was my fault. iframe-resizer does not have a real issue. Although the precision and rounding fix could be considered and implemented, it does not create a serious wrong behaviour. IMO the worst that might happen is that less than 1 pixel in height gets lost.

I have integrated iframe-resizer into the fiddle: https://jsfiddle.net/robsch/tp5fgwuy/197/ and the issue is there. But the reason is overflow-y: auto of the surrounding div of the tagged element. This is not a problem of iframe-resizer. I thought it, but I was wrong. I added this style in the fiddle to reproduce my code that has the issue. If that gets removed, all works as expected.

So sorry for the inconvenience. You may want to consider the rounding, but this is up to you. Thank you for taking the time anyway!

@davidjbradshaw
Copy link
Owner

Ok thanks for the update.

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

No branches or pull requests

2 participants