foundation / foundation-sites

The most advanced responsive front-end framework in the world. Quickly create prototypes and production code for sites that work on any kind of device.
https://get.foundation
MIT License
29.66k stars 5.48k forks source link

[Off-canvas] async loading of f6 js makes media queries unreliable, was: menu for small shows on all screens #8659

Closed COLABORATI closed 5 years ago

COLABORATI commented 8 years ago

I am using the zurb site template with the unfortunate and premature foundation 6 release.

The off-canvas javascript seems to have some problems, currently I am not sure what exactly is wrong, however I have a small site with an off-canvas menu that does work in development environment (npm start) but always shows the mobile version when compiled for production (npm run build).

The generated (minified) javascript code contains a few Cannot call a class as a function strings at the top - so there seems to be some js problem.

I already disabled the uncss destroyer in the gulpfile, but this still happens.

There is no error while compiling with gulp.

I am not able to build a working production version. The dev build, however, works. No error message on build. It sucks.

COLABORATI commented 8 years ago

I now setup a fresh foundation 6 zurb template just to see how the generated production javascript looks, and there are many Cannot call a class as a function strings on top of the built app.js. I did not do a deep dive into babel rabbitholes but having this multiple times on top of a minified javascript does not look healthy. If this is completely ok, it would be great to have an explanation or link, if not then the current f6 zurb template does not generate a working production build.

COLABORATI commented 8 years ago

A little correction, but that does not make anything better: it is not like always the small screen version is showing up on large screens, it is more some kind of randomness involved - after a few reloads the large screen menu appears, but then after another click on a link in that menu for some mystical reason the small screen top bar is shown again. Yes, I am deleting the browser cache (setting 'disable cache' in chromium inspector), also I did try it in several browsers.

The off-canvas code is based on http://zurb.com/building-blocks/top-bar-with-off-canvas .

No errors in console or while compiling with gulp.

COLABORATI commented 8 years ago

I re-checked the actual html and scss and everything is ok, I can not see any problems. Again, in development mode everything is perfect, but the production build generates a bad result. Yes, the uncss step is already disabled.

I changed the title to better emphasize what this issue is about. Actually, these are two issues, one is that the off-canvas js code does not seem to survive a production gulp build.

The second issue here which throws larger questions about F6 is the fact that a production build produces a broken version of something that works in development mode.

Now I understand very good why a good testing infrastructure is so important - and what it means that there is no such thing within F6.

colin-marshall commented 8 years ago

@COLABORATI sorry for the issues you are having. The goal for 6.3 is to have unit tests for all the components, so although they're currently missing, unit tests are at the top of the priority list. I don't know if there were any plans for unit tests in the templates, but I will keep that in mind once we get the unit tests for the components completed.

Please provide me with an example of the markdown you are using so I can reproduce.

COLABORATI commented 8 years ago

OK, this is something that can drive anybody crazy, unbelievable.

I commented out the things that happen during a production build in the gulpfile. Then I reactivated, run build, tested, and so on, many times in micro-steps. Checked break-point usage, all css classes, etc.

Maybe it is not something that has to do with code minifiers, I am not sure but here is one more observation: there is one more thing that changes on the transition from dev to prod - instead of using the browsersync server one starts to use F5 (no reference to Foundation 5 here, it is about the function key) to refresh. You know what - the randomness in showing offcanvas title-bar vs. non-offcanvas top-bar menus seems to be triggered only by the F5 key! Does that give you any hint? Does that make any sense?

When using only mouse or fingers for navigation, the menu does not change. Starting to use F5 for refreshing the page it starts to switch between the two menus, but no pattern here, not "every second time" or anything like that, it seems to be random, but with a tendency to showing title-bar more often than top-bar.

I still have commented out source maps, but this now happens with minified and with unminified js and css code, I tested without browsersync. Crazy.

I am using the example mentioned above with some additions that should not affect the visibility (menu items, styling of colors etc.), also I added the FOUC prevention mentioned in the responsive menu docs, but that does not seem to matter (same with or without). I could not reproduce that strange menu flipping with the raw example, so I will try to re-build an example with this problem from a fresh zurb-template step-by-step and will watch at which point that strange behavior starts, but I need to switch to other tasks, so it will take some time.

Man, I just wanted to add some mobile navigation to a little side project and it totally destroyed my saturday - please make that whole thing testable, this is hell. Also, please, more usable examples out of the box for standard use cases. There is currently one example for a complete offcanvas menu (and it has fouc trouble, see my other ticket) - there should be 10 examples at least with many variations.

Thanks for your attention.

colin-marshall commented 8 years ago

@COLABORATI thanks for looking into it further, please let me know what you find when you have time to look at it some more. And rest assured, more examples in the docs, better docs, and visual tests are also priorities for 6.3.

I responded to your FOUC issue and am awaiting reply.

As for that Cannot call a class as a function that appears repeated at the top of the minified file, that is done by babel for every module. It's my understanding that it's repeated because our goal was to not use any browser polyfills in the transpiled JS. In order for those not to repeat, we would have to use babel-runtime, which uses browser polyfills. We wanted the JS to be transpiled to pure ES5.

COLABORATI commented 8 years ago

Ok, today I sprinkled a few console.log into some foundation.*.js files. I basically put a logger after every variable assignment or initialization, the output is easy understandable when you know the code (I am sure there are more elegant methods).

Would you like to please take a look at Line 30 in foundation.util.mediaQuery.js

there is this and it does not make any sense to me:

var extractedStyles = $('.foundation-mq').css('font-family');

This is the console output when everything looks ok (correct menu shown):

mediaQuery initializing with these queries: 
app.js:10450 mediaQuery key: small value: only screen and (min-width: 0em)
app.js:10450 mediaQuery key: medium value: only screen and (min-width: 40em)
app.js:10450 mediaQuery key: large value: only screen and (min-width: 64em)
app.js:10450 mediaQuery key: xlarge value: only screen and (min-width: 75em)
app.js:10450 mediaQuery key: xxlarge value: only screen and (min-width: 90em)
app.js:10455 mediaQuery this._getCurrentSize: large
app.js:13641 responsiveToggle targetID: MenuTop
app.js:10471 mediaQuery query: only screen and (min-width: 40em)
app.js:10475 mediaQuery matches!
app.js:13688 responsiveToggle _update: Desktop Screen detected!
app.js:13689 responsiveToggle this.options.hideFor: medium
app.js:10471 mediaQuery query: only screen and (min-width: 0em)
app.js:10475 mediaQuery matches!

And this is what console output looks like when the wrong menu is shown after a manual refresh:

mediaQuery initializing with these queries: 
app.js:10450 mediaQuery key: Times New Roman value: only screen and (min-width: null)
app.js:10455 mediaQuery this._getCurrentSize: undefined
app.js:13641 responsiveToggle targetID: MenuTop
app.js:10471 mediaQuery query: null
app.js:13679 responsiveToggle _update: Mobile Screen detected!
app.js:13680 responsiveToggle this.options.hideFor: medium
app.js:10471 mediaQuery query: null

That $('.foundation-mq').css('font-family') selector does not look right to me. What should this be?

Thanks for your attention!

COLABORATI commented 8 years ago

BTW I have no clue why that output for the selector is changing between requests.

COLABORATI commented 8 years ago

OK, so this $('.foundation-mq').css('font-family'); seems to be the place for transporting the defined breakpoints - but it is a bad hack. For some reason the browser seems to add Times New Roman to that selector - do not ask my why, maybe this depends on some user settings, it just seems to do so from time to time. Of course the assumption about that string is wrong then and so the whole media query system for foundation 6 breaks.

Looks like it might be good to investigate for a better way to transport that important breakpoint string. I guess it would be better to generate some other kind of fake css, anything that will not be touched by any browser under any circumstances. Fonts might change from system to system and also anything related to fonts is exposed to user settings and also some plugins do weird stuff with fonts - I do not know about the inner life of why browsers might think they have to add something to a "font-famiily" css, but obviously that happens and it looks like it would be good to avoid anything remotely related to font-* completely for this kind of persistence mechanism. My guess is that any fake css structure inside .foundation-mq wold be ok, as long it has nothing to do with fonts.

COLABORATI commented 8 years ago

One more thing:

I guess that the underlying problem has something to do with the asynchronous nature of browser javascript and how you are handling it with the foundation.*.js. A Page refresh seems to be too fast for the init functions in the foundation.util.mediaQuery.js. Just guessing. I don't know. I am fed up now, I will implement some css only offcanvas that works without javascript - I would strongly recommend to do the same for Foundation 6.

COLABORATI commented 8 years ago

I was on the right track in my last session, but I had to stop just one step before finding the solution. Today I looked into this strange error again, styles, javascript and html everything ok, but I did this:

<script async src="{{root}}assets/js/app.js"></script>

The error described above occurs only when the script is included with the async attribute!

Is it expected to assume that async inclusion naturally will break a css framework? Even if yes, I still believe it would be ok to put a little warning about this in the docs, e.g.:

"Beware of including the Foundation Javascript source with the async attribute - this will produce strange errors in media query handling!"

COLABORATI commented 8 years ago

just changed the issue title to better reflect what this is about behind the scenes...

COLABORATI commented 8 years ago

Hi again, I would like to know if the the Zurb team is planning anything regarding this problem? Thanks!

COLABORATI commented 7 years ago

meanwhile, did this change? Async loading would be a great feature to use in a mobile framework.

kball commented 7 years ago

The core issue here I believe is actually related to the way Foundation modules (and utils) initialize - much of that is currently done "inline" as the JS is loaded... the refactoring targeted in https://github.com/zurb/foundation-sites/issues/9438 for v6.4 should address that; Better async behavior should be one of the goals of that project.

vita10gy commented 6 years ago

I think I have this going on too. Is this still indeed an open/known issue even though #9965 was merged?

The menu works with an empty cache, but with a loaded cache the mobile menu always shows.

Removing defer or async from the script seems to solve that issue.

DanielRuf commented 6 years ago

Please let us know which Foundation version you use.

vita10gy commented 6 years ago

6.4.3

Installed via composer. I'm compiling the scss myself but using the full JS in /vendor/zurb/foundation/dist/js as is. The issue seems to exist on the min and non-minned version.

Edit: Oops, actually that's actually not strictly true, the JS the page needs is all made into one large JS file, So not strictly "as is", but I'm not building those from sources.

DanielRuf commented 6 years ago

Not sure how this is related to the webpack PR.

DanielRuf commented 6 years ago

Can you create a PR which reproduces it and which tests this with the latest RC.

vita10gy commented 6 years ago

@kball Mentioned "the refactoring targeted in #9438 for v6.4 should address that; Better async behavior should be one of the goals of that project."

And it looks like that was addressed with the webpack PR, though I may have misinterpreted.

I'll see what I can do about recreating it on something I can share.

Is it your understanding that async/defer should be fine to use and I should find another tree to bark up, or is it a known issue async/defer should be avoided? I don't know what else to try, but I kind of feel like this is a me problem somewhere. I can't be the only person trying to async/defer foundation with every site scanner under the sun telling people to do that, if nothing else.

But then I found this, and it's exactly what's happening, and still open, so maybe it's not a me problem. :)

vita10gy commented 6 years ago

I think what's happening is basically MediaQuery's _init function just assumes that when it does

  var extractedStyles = __WEBPACK_IMPORTED_MODULE_0_jquery___default()('.foundation-mq').css('font-family');
    var namedQueries;

that the ('.foundation-mq').css('font-family'); has the css file loaded/parsed by then for that to mean anything to it, and that doesn't seem always the case. There should probably be some form of waiting for that to be true before it barrels on with nothing in extractedStyles and never fixing the mistake.

DanielRuf commented 5 years ago

So far async scripts are not controllable as the browser has the full control.

We do not want to reinvent something like requirejs and async should not be used for a complete framework. Only for small chunks of code like sharing buttons.

Closing as we will not directly address this in Foundation Sites.