Fix menu visibility

The crumb for the menu was shown like any other crumb when calling
"_showCrumb", but it was also shown when other crumbs were hidden
without taking into account the available width. This caused several
related problems, like the breadcrumbs taking too much space when the
menu was sometimes shown after the rest of the crumbs were adjusted to
the available width, or the menu being shown instead of the last crumb
even if there was room for it when the available width was increased.

Now the menu is always hidden before starting the resizing of the crumbs
to ensure that whether it was previously shown or not does not affect
the result. In a similar way, the menu will no longer be shown by
"_showCrumb", as it is not a regular crumb that has to be shown simply
if there is enough room. The menu is now shown as soon as any other
crumb is hidden; this ensures that the menu width will be taken into
account in further width checks. As when _updateMenu" is called it no
longer needs to take care of showing the menu this fixes the issue
revealed when fixing the test setup in the previous commit.

Finally, this implicitly fixes the failure in the breadcrumbs tests when
run on Firefox, as it was caused by the menu interfering in the
calculations of the other crumbs when increasing the width.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

2 files changed, 100 insertions, 10 deletions

diff --git a/apps/files/js/breadcrumb.js b/apps/files/js/breadcrumb.js
index e4951958ad9..3e43de39354 100644
--- a/apps/files/js/breadcrumb.js
+++ b/apps/files/js/breadcrumb.js
@@ -36,6 +36,7 @@
 		this.$menu = $('<div class="popovermenu menu-center"><ul></ul></div>');
 
 		this.crumbSelector = '.crumb:not(.hidden):not(.crumbhome):not(.crumbmenu)';
+		this.hiddenCrumbSelector = '.crumb.hidden:not(.crumbhome):not(.crumbmenu)';
 		options = options || {};
 		if (options.onClick) {
 			this.onClick = options.onClick;
@@ -269,19 +270,19 @@
  		 * Get the crumb to show
  		 */
  		_getCrumbElement: function() {
-			var hidden = this.$el.find('.crumb.hidden').length;
+			var hidden = this.$el.find(this.hiddenCrumbSelector).length;
 			var shown = this.$el.find(this.crumbSelector).length;
 			// Get the outer one with priority to the highest
 			var elmt = (1 - shown % 2) * (hidden - 1);
-			return this.$el.find('.crumb.hidden:eq('+elmt+')');
+			return this.$el.find(this.hiddenCrumbSelector + ':eq('+elmt+')');
 		},
 
  		/**
  		 * Show the middle crumb
  		 */
  		_showCrumb: function() {
-			if(this.$el.find('.crumb.hidden').length === 1) {
-				this.$el.find('.crumb.hidden').removeClass('hidden');
+			if(this.$el.find(this.hiddenCrumbSelector).length === 1) {
+				this.$el.find(this.hiddenCrumbSelector).removeClass('hidden');
 			}
 			this._getCrumbElement().removeClass('hidden');
  		},
@@ -298,9 +299,7 @@
 		 * Update the popovermenu
 		 */
 		_updateMenu: function() {
-			var menuItems = this.$el.find('.crumb.hidden');
-			// Hide the crumb menu if no elements
-			this.$el.find('.crumbmenu').toggleClass('hidden', menuItems.length === 0);
+			var menuItems = this.$el.find(this.hiddenCrumbSelector);
 
 			this.$menu.find('li').addClass('in-breadcrumb');
 			for (var i = 0; i < menuItems.length; i++) {
@@ -316,12 +315,19 @@
 				return;
 			}
 
+			// Always hide the menu to ensure that it does not interfere with
+			// the width calculations; otherwise, the result could be different
+			// depending on whether the menu was previously being shown or not.
+			this.$el.find('.crumbmenu').addClass('hidden');
+
 			// Show the crumbs to compress the siblings before hidding again the
 			// crumbs. This is needed when the siblings expand to fill all the
 			// available width, as in that case their old width would limit the
 			// available width for the crumbs.
-			while (this.$el.find('.crumb.hidden').length > 0
-				&& this.getTotalWidth() < this.$el.parent().width()) {
+			// Note that the crumbs shown always overflow the parent width
+			// (except, of course, when they all fit in).
+			while (this.$el.find(this.hiddenCrumbSelector).length > 0
+				&& this.getTotalWidth() <= this.$el.parent().width()) {
 				this._showCrumb();
 			}
 
@@ -339,11 +345,20 @@
 			// AND if there are crumbs left to hide
 			while (this.getTotalWidth() > availableWidth
 				&& this.$el.find(this.crumbSelector).length > 0) {
+				// As soon as one of the crumbs is hidden the menu will be
+				// shown. This is needed for proper results in further width
+				// checks.
+				// Note that the menu is not shown only when all the crumbs were
+				// being shown and they all fit the available space; if any of
+				// the crumbs was not being shown then those shown would
+				// overflow the available width, so at least one will be hidden
+				// and thus the menu will be shown.
+				this.$el.find('.crumbmenu').removeClass('hidden');
 				this._hideCrumb();
 			}
 			// If container is bigger than content + element to be shown
 			// AND if there is at least one hidden crumb
-			while (this.$el.find('.crumb.hidden').length > 0
+			while (this.$el.find(this.hiddenCrumbSelector).length > 0
 				&& this.getTotalWidth() + this._getCrumbElement().outerWidth(true) < availableWidth) {
 				this._showCrumb();
 			}
diff --git a/apps/files/tests/js/breadcrumbSpec.js b/apps/files/tests/js/breadcrumbSpec.js
index 3d154f12edd..ebc70f35a78 100644
--- a/apps/files/tests/js/breadcrumbSpec.js
+++ b/apps/files/tests/js/breadcrumbSpec.js
@@ -529,6 +529,45 @@ describe('OCA.Files.BreadCrumb tests', function() {
 			expect($crumbs.eq(6).hasClass('hidden')).toEqual(false);
 			expect($crumbs.eq(7).hasClass('hidden')).toEqual(false);
 		});
+		it('Updates the breadcrumbs when reducing available width taking into account the menu width', function() {
+			var $crumbs;
+
+			// enough space
+			$('#controls').width(1800);
+			bc._resize();
+
+			$crumbs = bc.$el.find('.crumb');
+
+			// Menu is hidden
+			expect($crumbs.eq(0).hasClass('hidden')).toEqual(true);
+			expect($crumbs.eq(1).hasClass('hidden')).toEqual(false);
+
+			expect($crumbs.eq(2).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(3).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(4).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(5).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(6).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(7).hasClass('hidden')).toEqual(false);
+
+			// simulate decrease
+			// 650 is enough for all the crumbs except the third and fourth
+			// ones, but not enough for the menu and all the crumbs except the
+			// third and fourth ones; the second one has to be hidden too.
+			$('#controls').width(650);
+			bc._resize();
+
+			// Second, third and fourth crumb are hidden and everything else is
+			// visible
+			expect($crumbs.eq(0).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(1).hasClass('hidden')).toEqual(false);
+
+			expect($crumbs.eq(2).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(3).hasClass('hidden')).toEqual(true);
+			expect($crumbs.eq(4).hasClass('hidden')).toEqual(true);
+			expect($crumbs.eq(5).hasClass('hidden')).toEqual(true);
+			expect($crumbs.eq(6).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(7).hasClass('hidden')).toEqual(false);
+		});
 		it('Updates the breadcrumbs when increasing available width', function() {
 			var $crumbs;
 
@@ -564,6 +603,42 @@ describe('OCA.Files.BreadCrumb tests', function() {
 			expect($crumbs.eq(6).hasClass('hidden')).toEqual(false);
 			expect($crumbs.eq(7).hasClass('hidden')).toEqual(false);
 		});
+		it('Updates the breadcrumbs when increasing available width taking into account the menu width', function() {
+			var $crumbs;
+
+			// limited space
+			$('#controls').width(850);
+			bc._resize();
+
+			$crumbs = bc.$el.find('.crumb');
+
+			// Third and fourth crumb are hidden and everything else is visible
+			expect($crumbs.eq(0).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(1).hasClass('hidden')).toEqual(false);
+
+			expect($crumbs.eq(2).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(3).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(4).hasClass('hidden')).toEqual(true);
+			expect($crumbs.eq(5).hasClass('hidden')).toEqual(true);
+			expect($crumbs.eq(6).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(7).hasClass('hidden')).toEqual(false);
+
+			// simulate increase
+			// 1030 is enough for all the crumbs if the menu is hidden.
+			$('#controls').width(1030);
+			bc._resize();
+
+			// Menu is hidden and everything else is visible
+			expect($crumbs.eq(0).hasClass('hidden')).toEqual(true);
+			expect($crumbs.eq(1).hasClass('hidden')).toEqual(false);
+
+			expect($crumbs.eq(2).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(3).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(4).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(5).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(6).hasClass('hidden')).toEqual(false);
+			expect($crumbs.eq(7).hasClass('hidden')).toEqual(false);
+		});
 		cit('Updates the breadcrumbs when increasing available width with an expanding sibling', function() {
 			var $crumbs;