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.

Sign up for GitHub

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

Jump to bottom

Flex gap detection does not work when inside display:none iframe #2590

Open
chris13524 opened this issue Aug 17, 2020 · 6 comments · May be fixed by #2593
Open

Flex gap detection does not work when inside display:none iframe #2590

chris13524 opened this issue Aug 17, 2020 · 6 comments · May be fixed by #2593

Comments

@chris13524
Copy link
Contributor

chris13524 commented Aug 17, 2020 •
edited

I believe this is because scrollHeight does not get computed by the browser until an actual layout is performed. And since the iframe is display:none, no layout happens.

Possible solutions:

  • convert to async test and wait for resize event (triggered when display:none is removed) before testing

Test case:

<iframe src="modernizr-flexgap-issue-iframe.html" style="display:none;"></iframe>

modernizr-flexgap-issue-iframe.html:

<script>

// adapted from: https://github.com/Modernizr/Modernizr/blob/master/feature-detects/css/flexgap.js

// create flex container with row-gap set
var flex = document.createElement('div');
flex.style.display = 'flex';
flex.style.flexDirection = 'column';
flex.style.rowGap = '1px';

// create two, elements inside it
flex.appendChild(document.createElement('div'));
flex.appendChild(document.createElement('div'));

// append to the DOM (needed to obtain scrollHeight)
document.documentElement.appendChild(flex);
var isSupported = flex.scrollHeight === 1; // flex container should be 1px high from the row-gap
flex.parentNode.removeChild(flex);

console.log("flexGap isSupported: " + isSupported);

</script>
@chris13524
Copy link
Contributor Author

Using window.addEventListener('resize', ...) works. However this does not work in a "normal" situation due to the lack of a resize event. Would need some code to detect we are in a display:none situation first, and then add the resize event listener. Possibly by using getComputedStyle() and seeing that everything is blank.

Would converting this to an async test be acceptable for backwards compatibility @rejas?

@shadeed
Copy link

shadeed commented Aug 19, 2020

Hello Chris, I was looking for a way to detect the support for flexbox gap, but I didn't find the CSS class in the official modernizr build. Is this feature still not released yet?

Sorry that my question is not related to the issue.

@Markel
Copy link
Contributor

Markel commented Aug 19, 2020

Hi @shadeed the feature is already in the codebase under the css folder (or click here) and should be available in the last version available on Github. The problem resides on the fact that the web is outdated (I think it works with v3.6.0), it is a problem that has been for a time now. Apart from building the library from source code there is no other solution I can give right now 😔.

Note that the last version of Github has the bug about which this issue is

@rejas
Copy link
Member

rejas commented Aug 19, 2020

Would converting this to an async test be acceptable for backwards compatibility @rejas?

I guess that would be acceptable. But maybe @Markel and @patrickkettner have some opinion on this too?

@Markel
Copy link
Contributor

Markel commented Aug 19, 2020 •
edited

I honestly don't have a clue whether converting a test to async causes compatibility problems @rejas 😅

If it doesn't I would go for it, I think async should be the future (I actually have written my few tests asynchronously)

If it does... I mean we haven't released v4 yet so we have that possibility 🤷. Maybe we should create a v4 branch and maybe merge #2108 there (seeing that the PR is becoming outdated) so we have things more organized 🤔?

Edit: PR number corrected

@Markel Markel mentioned this issue Aug 19, 2020
Merged
@chris13524 chris13524 linked a pull request Aug 25, 2020 that will close this issue
Open
@chris13524
Copy link
Contributor Author

Just submitted a PR #2593 to fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support display:none iframes in flexgap test chris13524/Modernizr
4 participants
@rejas @chris13524 @shadeed @Markel