greedy-nav is not reliable on small devices

[migupry]

Environment

• Minimal Mistakes version: 4.20.1
• Ruby gem or remote theme version: not sure/used remote theme starter
• Jekyll version: not sure/used remote theme starter
• Git repository URL: https://github.com/migupry/mmbasic and https://github.com/migupry/mmlogo
• GitHub Pages hosted (if yes provide URL to site): Yes: https://www.miguel.dev.br/mmbasic/ and https://www.miguel.dev.br/mmlogo/
• Operating system: Android

Expected behavior

Greedy nav is supposed to always show the same amount of visible-links and hidden-links, and it doesn't seem the case.
I've created two new repositories using the remote theme starter to demonstrate issues I've been having consistently using minimal mistakes. One of them (mmbasic) I've made no changes, and the other (mmlogo) I've added the bio-photo as the logo just to show another example which seem to be easier to reproduce the bug.

This is how the pages will load most of the time on my Android device, which is the expected behaviour:
mmbasic:
https://imgur.com/z5jEdB8
mmlogo:
https://imgur.com/ZAJCwVq

Steps to reproduce the behavior

This is how the pages are rendered sometimes, when opening the page for the first time:
mmbasic:
https://imgur.com/2BbpxzD (Notice "Categories" is gone)
https://imgur.com/0h1o70I (All links are visible, "Posts" is hidden because of overflow and "Categories" is hidden behind the title)
mmlogo:
https://imgur.com/n1jpuVV ("Posts" is hidden behind the title)

To reproduce it, simply open one of the pages on a mobile (android) device: https://www.miguel.dev.br/mmbasic/ or https://www.miguel.dev.br/mmlogo/

• On Chrome mobile browser, this is mostly unreproduceable with page reloads, or even by clicking on the page links. It can only happen the first time you enter the page. So to reproduce, you need to go to another page, and come back, or close and reopen the tab. Some times you need to do this 5-10 times to reproduce, but sometimes, 1 or 2 times is enough.
• This issue seems to be more easily reproduceable in the Logo version, than the basic version, and also seems to bother more in Instagram and Telegram's in-app browsers. I would guess it has to do with how those browsers handle cache, but i'm not sure. So sending this page's links via messaging on one of this apps, and opening on the same app, particularly the "mmlogo" version seem to be the easiest way to reproduce this. But I have to enforce this is not an issue exclusive to those browsers!

Other

• I've tried messing with "jquery.greedy-navigation.js", and I've found the issue was mostly gone when I've changed the availableSpace = $vlinks.width() - 10; line to a calculation of the window's width decreased by some "rem" (font-size) value, which was obviously dumb and only viable with my particular setup of "logo + title".
• As I've said on the previous item, by using that hack the issue was mostly gone, but not gone for good. It doesn't seem to have much effect for Instagram in-app browser. Most of the time, when opening the page via the instagram in-app browser, the navbar was messy (although it would get fixed with a reload). Since I was putting a page on production, and our main source of exposure for the page was instagram, I had to find a quick and dirty fix: I've ended up removing most of "greedynav" logic, duplicating the "visible" and "hidden" links on "masthead.html" and controlling the "display" attribute of "visible" and "hidden" links via media-queries. Dirty and dumb, but hey, at least it was reliable!

I'm about to get my hands dirty again trying to implement https://github.com/gijsroge/priority-navigation or one of the other alternatives listed there on one of my two projects that are using Minimal Mistakes to get a better fix for myself, and depending on my results I'll share here. Thought I would open this issue before though.

Needless to say, I'm in love with Minimal Mistakes, it's a really great job!

[mmistakes]

I believe the issue is JavaScript related. The script that determines how wide the nav space is changes depending on when it tries to figure out that value.

When you have a logo it could determine the width differently depending on if the image loads before or after.

This is why it’s inconsistent. Perhaps someone better with JavaScript has a fix. I forked the script from the greedy nav repo so not sure how to improve it.

[migupry]

Thank you for your answer! I'm not at all an expert JavaScript programmer, but I'll be testing other "priority+ navigation" implementations, to see if I can get any good results with them, or maybe get some insights on how to improve the current greedy nav implementation. Or maybe none of those, and just learn a bunch in the process.

[migupry]

Hey, I come with good news!

After 2-3 days of intense testing and searching, I'm pretty sure I've managed to identify and solve the bug!
Actually, there are two separate bugs, that seem to be caused by race conditions.

1st bug:

Behavior:
On pages both with and without logos, though rarely, the "visible-link" "ul" object takes more space than it should, making some items on the navbar appear completely or partially hidden.
The cause:
On the _base.scss file, it is set both "-webkit-transition" and "transition" attributes for a bunch of elements, and importantly to our case, the "a" elements and the "img" elements.
On the Navbar there are 3 types of "a", which are affected by transitions and are relevant to this bug: "site-logo", "site-title" and ".visible-links -> li -> a" (which are the navbar visible section links) also, the "img" object under "site-logo".
On the greedy navigation script, jquery.greedy-navigation.js, the line availableSpace = $vlinks.width() - 10; measures vlinks (the ".visible-links" "ul" object) width, and this calculation is crucial to determine how many items may fit on the navbar. After some debugging, I've found out that the measured size of vlinks was not consistent, which was... strange. I thought this had to do with the script, but nothing seemed to be wrong there.
Then finally it clicked to me! It was a race condition. Most of the times, the measurement is done after the transition on the "a" (and img) objects width is done, but yeah, most of the times is not always. Sometimes, the vlinks object's width is measured before or during the transition, and thus getting the wrong value.
The fix:
We could wait for the transition to occur in order to execute the greedy navigation script logic (relevant related discussion here), but... I mean... Is it worth it? We would have to add a considerable amount of complexity to the code in order to control the transition's execution with js/jquery.
My suggestion is simply: "transitions on navbar -> bad":
diff between my fix and original code:
_navigation.scss

187,188d186 // .greedy-nav a
<     -webkit-transition: none;
<     transition: none;
202,206d199 // .greedy-nav img
<   }
<   
<   img{
<     -webkit-transition: none;
<     transition: none;

note: .visible-links a::before elements also have a transition, but they don't seem to influence the code, so I've ignored it.
My conscience says it would be better to disable all transitions on the navbar, since it would make harder for an user customization on the template or even, who knows, an update, to bring this bug back again. But this should work for now.

2nd bug:

Behavior:
On pages with logos, not so rarely, the "visible-link" "ul" object takes more space than it should, making some items on the navbar be completely or partially hidden. The same effect as the first bug, but more frequently, and limited to pages with logos.
The cause:
Although we've target both the .site-logo element and the img element inside of it on the first solution, this is not enough to fix this. After some testing, I've noticed yet another race condition: similar as before, the vlinks measurement can occur before the .site-logo img is even loaded. If the image happens to take a little more time to load, since the widths are not fixed, the vlinksobject is measured without it, and when the image is finally loaded, all the calculation with the wrong values is already done.
The fix:
We need to wait for the images on the navbar to load before doing the relevant calculation. As discussed here, the best way to make this happen without much hassle seems to be using David DeSandro’s imagesLoaded library since "it has no dependencies, is pretty small, and works with jQuery".
So I've included imagesloaded.pkgd.js (the non packaged version display an error and doesn't work out of the box) on /assets/js/plugins, added it into package.json and assured the image is loaded before running the relevant logic.
diff between my fix and original code: (before correcting indentation)
jquery.greedy-navigation.js

18d17 // before "Get initial state" line
<   $('nav.greedy-nav').imagesLoaded( function() {
74c73 // the second to last line
<   })
---
> 

The way this is implemented makes sure it works even if the the user adds another image on the navbar, for some reason.

---

After this 2 fixes, the bugs are not reproducible anymore.

If it is relevant, I could make a pull request, just tell me.

[mmistakes]

Thanks for digging into this issue, great work uncovering the issues.
The only thing that gives me pause is the 2nd bug and including more JavaScript to combat it. While only around 5kb minified, it still has a lot going on in it.

An since not everyone will have a header logo I feel bad including unnecessary JavaScript if it's not needed. Not sure if there's a better way to handle that and not overcomplicate things.

That said I'm all for your fix on the first bug. Could you submit a PR and then myself and others could test it out.

Thanks again!

[migupry]

Hey no problem! I'm just trying to give back something as this theme is being so useful to me.
Yeah, it makes sense... It sure is adding a lot of unecessary JS. I'll try to work something out without the external lib.
I'll come back after some testing.

[migupry]

I've come up with something based on imagesloaded and relevant discussions here and here.

This doesn't add any dependencies and simply doesn't do anything if the page doesn't have a logo.

If it does have a logo, it checks if it's loaded before continuing:

• It the logo is loaded, it proceeds with the script as usual.
• If it's not loaded, it adds eventListeners or attachEvents to wait for the image to load, then it clears the eventListeners/attachEvents before continuing with the script.

I've tested with Chrome/Firefox mobile browsers, as well as Instagram and Telegram's in-app browsers and it worked 100% of the time, even if the logo has a broken src.

It should work with legacy browsers as well.

$ diff assets/js/plugins/jquery.greedy-navigation.js ~/minimal-mistakes/assets/js/plugins/jquery.greedy-navigation.js 
72c72
<   var logoImg = $('nav.greedy-nav .site-logo img')[0];
---
>   check();
74,102d73
<   // check if page has a logo
<   if(typeof(logoImg) != "undefined"){
<     function checkClearEventListener(){ // function that clears EventListeners and calls check()
<       logoImg.removeEventListener( 'load', checkClearEventListener, false );
<       logoImg.removeEventListener( 'error', checkClearEventListener, false );
<       check();
<     }
<     function checkClearAttachEvent(){ // function that clears AttachEvents and calls check()
<       logoImg.detachEvent('onload', checkClearAttachEvent );
<       logoImg.detachEvent('onerror', checkClearAttachEvent );
<       check();
<     }
<     // check if logo is not loaded
<     if(!(logoImg.complete || logoImg.naturalWidth !== 0)){
<       // if browser is compatible with EventListeners use those to wait for the logo to load
<       if (logoImg.addEventListener) {
<         logoImg.addEventListener( 'load', checkClearEventListener, false );
<         logoImg.addEventListener( 'error', checkClearEventListener, false );
<       }
<       // if browser is only compatible with attachEvents (IE<=8) use those to wait for the logo to load
<       else if (logoImg.attachEvent) {
<         logoImg.attachEvent('onload', checkClearAttachEvent );
<         logoImg.attachEvent('onerror', checkClearAttachEvent );
<       } else logoImg.onload(check()); // if browser isn't compatible with neither EventListeners nor attachEvents, use onload attribute
<       // if logo is already loaded just check
<     } else check();
<   // if page does not have a logo just check
<   } else check();
<

[migupry]

After thinking a little bit more, I've realized I wasn't using jQuery enough, and the code was overcomplicated because of that.

I've come up with this simpler version, using more jQuery (since it is already a dependency), that works the same, but is significantly cleaner:

diff assets/js/plugins/jquery.greedy-navigation.js ~/minimal-mistakes/assets/js/plugins/jquery.greedy-navigation.js 
72c72
<   var logoImg = $('nav.greedy-nav .site-logo img');
---
>   check();
74,84d73
<   // check if page has a logo
<   if(logoImg.length !== 0){
<     // check if logo is not loaded
<     if(!(logoImg[0].complete || logoImg[0].naturalWidth !== 0)){
<       // if logo is not loaded wait for logo to load or fail to check
<       logoImg.one("load error", check);
<     // if logo is already loaded just check
<     } else check();
<   // if page does not have a logo just check
<   } else check();
<   

[mmistakes]

Nice work.
When you're ready open a pull request and I can mark it as a draft (WIP) so myself and others can play around with it and provide feedback (if any).

[migupry]

Thank you.
Ok! I'll do that. I'm just doing some final testing on devices I have at my disposal at the moment.

[migupry]

After some more intense work and study I have more things to share:

I was testing my code on a 320px wide Iphone device I have access to, and I couldn't understand why in some test cases the first navbar item was being shown partially hidden under the title. I knew this wasn't a race condition issue anymore, because, unlike before, this behavior happend 100% of the times. I also knew this wasn't related to the code I've added, because using the unmodified version of the script I've also got the same results.

This got me to do yet another deep dive into the script, and then I had to face something that had me boggled since I've started analyzing the greedy-nav script: The way it calculates the available space for links on the navbar. The relevant line is availableSpace = $vlinks.width() - 10;. So what this does is to presume that the available space for items in the navbar is the totality of the width of visible links element minus 10px. I had in my mind this was strange because 10 pixels is a static value, that couldn't take into account the dynamic nature of css with it's media queries and so on. That's why my very first approach was trying to use (incorrectly) em/rem values to calculate the available width.

The width of the visible links element decreased by 10 pixels is an educated guess of the available space, but it is not precise. The developers of greedy-nav are certainly not to blame, as doing precise calculations adds a considerable amount of complexity, and in my opinion, defeats the purpose of it as a versatile and lightweight priority+ navbar implementation.

If you want to calculate the precise available space for navItems, you need to take the navbar inner width and decrease from it the width of every visible element (except for the links). But how different the precise measurement and the approximation could be, you may ask? On a page without a logo, I've measured a 10px difference on a 360px width Android device, and a remarkable 30px difference on the mentioned 320px width Iphone device. Thats not at all insignificant as it represents +9% of the Iphone device width and can easily result in consistent glitches on some devices in some pages (as in my test case).

As we're talking about precision (and good use of space), there's 2 another issues.

• While there's no hidden item behind the "hamburger menu toggle", you don't need to take the toggle's width into account, but as soon as there is at least 1 hidden item, you need to account for it. Handling this maximizes the use of navbar space and adds precision.
• Minimal mistakes has 4 CSS width breakpoints (<768px, <1024px, < 1280px, >= 1280px), and under each of this breakpoints the size of navbar items change. Since in the original script the calculation of breakpoints for the navbar (a list of points when the items should go into the 'hamburger menu') only happens once the script loads, the navbar start getting inconsistent once you resize your window and reach another CSS width breakpoint (thus resulting in the navbar items changing width). This could occur when you rotate a large mobile or tablet screen, or when you maximize/restore a window on Desktop. Recalculating the navbar breakpoints once (and only once) you reach another CSS width breakpoint is also necessary if we're aiming for precision

So I've implemented in the script all those things I've mentioned. I've done my best, doing performance tests all along, to make this the less performance greedy as I could. This of course inevitably added time complexity to the code, since I've added some grabby calculations (particularly elements widths measurements). That being said, at least in my test cases the added complexity is not at all performance critical, nor can it be perceived by the user, or outside of performance testing. This is tested both with and without the logo and the search toggle. I'm proud to say this adds what I call "pixel perfect responsivity". :)

diff jquery.greedy-navigation.js ~/minimal-mistakes/assets/js/plugins/jquery.greedy-navigation.js 
12,34d11
<   var $nav = $("nav.greedy-nav");
<   var $logo = $('nav.greedy-nav .site-logo');
<   var $logoImg = $('nav.greedy-nav .site-logo img');
<   var $title = $("nav.greedy-nav .site-title");
<   var $search = $('nav.greedy-nav button.search__toggle');
<   
<   var numOfItems, totalSpace, closingTime, breakWidths;
< 
<   // This function measures both hidden and visible links and sets the navbar breakpoints
<   // This is called the first time the script runs and everytime the "check()" function detects a change of window width that reached a different CSS width breakpoint, which affects the size of navbar Items 
<   // Please note that "CSS width breakpoints" (which are only 4) !== "navbar breakpoints" (which are as many as the number of items on the navbar)
<   function measureLinks(){
<     numOfItems = 0;
<     totalSpace = 0;
<     closingTime = 1000;
<     breakWidths = [];
< 
<     // Adds the width of a navItem in order to create breakpoints for the navbar
<     function addWidth(i, w) {
<       totalSpace += w;
<       numOfItems += 1;
<       breakWidths.push(totalSpace);
<     }
36,49c13,16
<     // Measures the width of hidden links by making a temporary clone of them and positioning under visible links
<     function hiddenWidth(obj){
<       var clone = obj.clone();
<       clone.css("visibility","hidden");
<       $vlinks.append(clone);
<       addWidth(0, clone.outerWidth());
<       clone.remove();
<     }
<     // Measure both visible and hidden links widths
<     $vlinks.children().outerWidth(addWidth);
<     $hlinks.children().each(function(){hiddenWidth($(this))});
<   }
<   // Get initial state
<   measureLinks();
---
>   var numOfItems = 0;
>   var totalSpace = 0;
>   var closingTime = 1000;
>   var breakWidths = [];
51,53c18,23
<   var winWidth = $( window ).width();
<   // Set the last measured CSS width breakpoint: 0: <768px, 1: <1024px, 2: < 1280px, 3: >= 1280px.
<   var lastBreakpoint = winWidth < 768 ? 0 : winWidth < 1024 ? 1 : winWidth < 1280 ? 2 : 3;
---
>   // Get initial state
>   $vlinks.children().outerWidth(function(i, w) {
>     totalSpace += w;
>     numOfItems += 1;
>     breakWidths.push(totalSpace);
>   });
59,66d28
<     winWidth = $( window ).width();
<     // Set the current CSS width breakpoint: 0: <768px, 1: <1024px, 2: < 1280px, 3: >= 1280px.
<     var curBreakpoint = winWidth < 768 ? 0 : winWidth < 1024 ? 1 : winWidth < 1280 ? 2 : 3;
<     // If current breakpoint is different from last measured breakpoint, measureLinks again
<     if(curBreakpoint !== lastBreakpoint) measureLinks();
<     // Set the last measured CSS width breakpoint with the current breakpoint
<     lastBreakpoint = curBreakpoint;
< 
67a30
>     availableSpace = $vlinks.width() - 10;
69,74d31
<     // Decrease the width of visible elements from the nav innerWidth to find out the available space for navItems
<     availableSpace = /* nav */ $nav.innerWidth()
<                    - /* logo */ ($logo.length !== 0 ? $logo.outerWidth(true) : 0)
<                    - /* title */ $title.outerWidth(true)
<                    - /* search */ ($search.length !== 0 ? $search.outerWidth(true) : 0)
<                    - /* toggle */ (numOfVisibleItems !== breakWidths.length ? $btn.outerWidth(true) : 0);
82,83c39,40
<       // There is more than enough space. If only one element is hidden, add the toggle width to the available space
<     } else if (availableSpace + (numOfVisibleItems === breakWidths.length - 1?$btn.outerWidth(true):0) > breakWidths[numOfVisibleItems]) {
---
>       // There is more than enough space
>     } else if (availableSpace > breakWidths[numOfVisibleItems]) {
115,126c72,74
<   // check if page has a logo
<   if($logoImg.length !== 0){
<     // check if logo is not loaded
<     if(!($logoImg[0].complete || $logoImg[0].naturalWidth !== 0)){
<       // if logo is not loaded wait for logo to load or fail to check
<       $logoImg.one("load error", check);
<     // if logo is already loaded just check
<     } else check();
<   // if page does not have a logo just check
<   } else check();
<   
< });
---
>   check();
> 
> });

[mmistakes]

All sounds good to me. Eager to test this out in a PR.

[migupry]

I've opened the Pull request 2674.

[stale]

This issue has been automatically marked as stale because it has not had recent activity.

If this is a bug and you can still reproduce this error on the master branch, please reply with any additional information you have about it in order to keep the issue open.

If this is a feature request, please consider whether it can be accomplished in another way. If it cannot, please elaborate on why it is core to this project and why you feel more than 80% of users would find this beneficial.

This issue will automatically be closed in 7 days if no further activity occurs. Thank you for all your contributions.