Use iframe height and width attributes instead offset height and width #88
Conversation
|
@deansimcox thanks for the explanation. I will look into this more ASAP! |
src/reframe.js
Outdated
| @@ -18,8 +18,8 @@ export default function reframe(target, cName) { | |||
| if (hasClass) return; | |||
|
|
|||
| // general targeted <element> sizes | |||
| const h = frame.offsetHeight; | |||
| const w = frame.offsetWidth; | |||
| const h = parseInt(frame.getAttribute('height')); | |||
There was a problem hiding this comment.
I think a ternary might solve both, the issue you're having and your other note.
In example:
const h = frame.getAttribute('height') !== 'undefined' ?
parseInt(frame.getAttribute('height')) :
frame.offsetHeight^ I'm not 100% that that is correct but what about something like that? 😁
|
@deansimcox what an awesome and thoughtful CR! Please make the minor tweaks and then lets ship this! 🚢 |
…eight and width. Version bumped to 2.1.19
|
No worries @yowainwright and thanks for the feedback! I just pushed another commit - I decided to use an if block to check that both height and width are not null. |
|
@deansimcox thank you! Why did you choose |
|
@yowainwright It just felt better to store |
There was a problem hiding this comment.
@deansimcox thanks!
I pasted some sample code. That's what I was thinking.
I see what you were saying about repeating getAttribute 2x—even 3x when we consider '%'.
| @@ -17,9 +17,22 @@ export default function reframe(target, cName) { | |||
| const hasClass = frame.className.split(' ').indexOf(c) !== -1; | |||
| if (hasClass) return; | |||
|
|
|||
| // get height width attributes | |||
There was a problem hiding this comment.
// try to use `offsetSize` if the size attribute is not defined or of it uses a percentage
const shouldUseSzAttr = (a) => !frame.getAttribute(a) || frame.getAttribute(a).toString('%')
const h = shouldUseSzAttr('height') ? frame.offsetHeight : parseInt(frame.getAttribute('height'))
const w = shouldUseSzAttr('width') ? frame.offsetWidth : parseInt(frame.getAttribute('width'))
|
@deansimcox thank you for your thoughtful insight. It is because of your insight that I'm going to make some small tweaks besides just your change. I'm going to merge your PR. There will be a slight tweak very soon after! 🙌 🙏 |
This pull request solves an issue with Firefox that I've been having, but I'm guessing it could affect other browsers.
The issue is caused by the iframe having a height of "0" at the time when reframe.js is run. I initially solved this by running reframe on the onload event for the iframe, but this causes the iframe to essentially load twice when reframe does it's DOM manipulation.
This code change will make reframe use the height and with attributes on the iframe instead of getting the offsetHeight and offsetWidth.