Move checkboxes to their own column

The selection column is not only a visual column, but also a real column
of the file list table. Unlike other columns whose width is reduced in
space constrained screens the selection column must stay the same so the
tapping area is large enough to be easily usable

The selection column does not appear in the search results table, so its
contents have to be explicitly aligned with those of the main table
based on whether the main table has a selection column or not (using the
"has-selection" CSS class in the same way as the "has-favorite" CSS
class was being used when there was a column for favorite actions).

In the tests the ":visible" selector can no longer be used. That
selector matches elements with a width or height that is greater than
zero, but the dimensions calculated in the unit tests are not reliable;
the width of the link was zero before these changes, and now moving the
checkbox to its own column causes the height of the link to become zero
too, so it no longer matches the ":visible" selector even if it is not
hidden. As hidding and showing the link is based on its "display" CSS
property its value is the one checked now.

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

6 files changed, 69 insertions, 62 deletions

diff --git a/apps/files/css/files.scss b/apps/files/css/files.scss
index 80a2b44ac11..65519ae2e16 100644
--- a/apps/files/css/files.scss
+++ b/apps/files/css/files.scss
@@ -438,46 +438,27 @@ table td.filename .uploadtext {
 	opacity: .5;
 }
 
+table td.selection {
+	padding: 0;
+}
+
 /* File checkboxes */
-#fileList tr td.filename>.selectCheckBox + label:before {
+#fileList tr td.selection>.selectCheckBox + label:before {
 	opacity: 0.3;
-	position: absolute;
-	bottom: 4px;
-	right: 0;
-	z-index: 10;
 }
 
 /* Show checkbox with full opacity when hovering, checked, or selected */
-#fileList tr:hover td.filename>.selectCheckBox + label:before,
-#fileList tr:focus td.filename>.selectCheckBox + label:before,
-#fileList tr td.filename>.selectCheckBox:checked + label:before,
-#fileList tr.selected td.filename>.selectCheckBox + label:before {
+#fileList tr:hover td.selection>.selectCheckBox + label:before,
+#fileList tr:focus td.selection>.selectCheckBox + label:before,
+#fileList tr td.selection>.selectCheckBox:checked + label:before,
+#fileList tr.selected td.selection>.selectCheckBox + label:before {
 	opacity: 1;
 }
 
 /* Use label to have bigger clickable size for checkbox */
-#fileList tr td.filename>.selectCheckBox + label,
-.select-all + label {
-	background-position: 30px 30px;
-	height: 50px;
-	position: absolute;
-	width: 50px;
-	z-index: 5;
-}
-#fileList tr td.filename>.selectCheckBox {
-	/* sometimes checkbox height is bigger (KDE/Qt), so setting to absolute
-	 * to prevent it to increase the height */
-	position: absolute;
-	z-index: 10;
-}
+#fileList tr td.selection>.selectCheckBox + label,
 .select-all + label {
-	top: 0;
-}
-.select-all + label:before {
-	position: absolute;
-	top: 18px;
-	left: 18px;
-	z-index: 10;
+	padding: 16px;
 }
 
 #fileList tr td.filename {
diff --git a/apps/files/css/mobile.scss b/apps/files/css/mobile.scss
index eefc92c816b..e7b75910fa9 100644
--- a/apps/files/css/mobile.scss
+++ b/apps/files/css/mobile.scss
@@ -24,10 +24,6 @@ table td.date {
 table td {
 	padding: 0;
 }
-/* and accordingly fix left margin of file list summary on mobile */
-.summary .info {
-	margin-left: 105px;
-}
 
 /* remove shift for multiselect bar to account for missing navigation */
 table.multiselect thead {
diff --git a/apps/files/js/filelist.js b/apps/files/js/filelist.js
index 2c97bf737b3..0d45c29b25a 100644
--- a/apps/files/js/filelist.js
+++ b/apps/files/js/filelist.js
@@ -332,7 +332,7 @@
 
 			this.$fileList.on('click','td.filename>a.name, td.filesize, td.date', _.bind(this._onClickFile, this));
 
-			this.$fileList.on('change', 'td.filename>.selectCheckBox', _.bind(this._onClickFileCheckbox, this));
+			this.$fileList.on('change', 'td.selection>.selectCheckBox', _.bind(this._onClickFileCheckbox, this));
 			this.$el.on('show', _.bind(this._onShow, this));
 			this.$el.on('urlChanged', _.bind(this._onUrlChanged, this));
 			this.$el.find('.select-all').click(_.bind(this._onClickSelectAll, this));
@@ -593,7 +593,7 @@
 		 * @param {bool} state true to select, false to deselect
 		 */
 		_selectFileEl: function($tr, state, showDetailsView) {
-			var $checkbox = $tr.find('td.filename>.selectCheckBox');
+			var $checkbox = $tr.find('td.selection>.selectCheckBox');
 			var oldData = !!this._selectedFiles[$tr.data('id')];
 			var data;
 			$checkbox.prop('checked', state);
@@ -649,7 +649,7 @@
 				else {
 					this._lastChecked = $tr;
 				}
-				var $checkbox = $tr.find('td.filename>.selectCheckBox');
+				var $checkbox = $tr.find('td.selection>.selectCheckBox');
 				this._selectFileEl($tr, !$checkbox.prop('checked'));
 				this.updateSelectionSummary();
 			} else {
@@ -704,7 +704,7 @@
 		 */
 		_onClickSelectAll: function(e) {
 			var checked = $(e.target).prop('checked');
-			this.$fileList.find('td.filename>.selectCheckBox').prop('checked', checked)
+			this.$fileList.find('td.selection>.selectCheckBox').prop('checked', checked)
 				.closest('tr').toggleClass('selected', checked);
 			this._selectedFiles = {};
 			this._selectionSummary.clear();
@@ -1063,6 +1063,13 @@
 
 			this.$fileList.empty();
 
+			if (this._allowSelection) {
+				// The results table, which has no selection column, checks
+				// whether the main table has a selection column or not in order
+				// to align its contents with those of the main table.
+				this.$el.addClass('has-selection');
+			}
+
 			// clear "Select all" checkbox
 			this.$el.find('.select-all').prop('checked', false);
 
@@ -1192,6 +1199,20 @@
 				path = this.getCurrentDirectory();
 			}
 
+			// selection td
+			if (this._allowSelection) {
+				td = $('<td class="selection"></td>');
+
+				td.append(
+					'<input id="select-' + this.id + '-' + fileData.id +
+					'" type="checkbox" class="selectCheckBox checkbox"/><label for="select-' + this.id + '-' + fileData.id + '">' +
+					'<span class="hidden-visually">' + t('files', 'Select') + '</span>' +
+					'</label>'
+				);
+
+				tr.append(td);
+			}
+
 			// filename td
 			td = $('<td class="filename"></td>');
 
@@ -1203,14 +1224,6 @@
 			else {
 				linkUrl = this.getDownloadUrl(name, path, type === 'dir');
 			}
-			if (this._allowSelection) {
-				td.append(
-					'<input id="select-' + this.id + '-' + fileData.id +
-					'" type="checkbox" class="selectCheckBox checkbox"/><label for="select-' + this.id + '-' + fileData.id + '">' +
-					'<span class="hidden-visually">' + t('files', 'Select') + '</span>' +
-					'</label>'
-				);
-			}
 			var linkElem = $('<a></a>').attr({
 				"class": "name",
 				"href": linkUrl
@@ -2613,6 +2626,13 @@
 		 */
 		_createSummary: function() {
 			var $tr = $('<tr class="summary"></tr>');
+
+			if (this._allowSelection) {
+				// Dummy column for selection, as all rows must have the same
+				// number of columns.
+				$tr.append('<td></td>');
+			}
+
 			this.$el.find('tfoot').append($tr);
 
 			return new OCA.Files.FileSummary($tr, {config: this._filesConfig});
diff --git a/apps/files/templates/list.php b/apps/files/templates/list.php
index 46bd9351e39..483bdcd2e38 100644
--- a/apps/files/templates/list.php
+++ b/apps/files/templates/list.php
@@ -41,12 +41,14 @@
 <table id="filestable" data-allow-public-upload="<?php p($_['publicUploadEnabled'])?>" data-preview-x="32" data-preview-y="32">
 	<thead>
 		<tr>
+			<th id="headerSelection" class="hidden column-selection">
+				<input type="checkbox" id="select_all_files" class="select-all checkbox"/>
+				<label for="select_all_files">
+					<span class="hidden-visually"><?php p($l->t('Select all'))?></span>
+				</label>
+			</th>
 			<th id='headerName' class="hidden column-name">
 				<div id="headerName-container">
-					<input type="checkbox" id="select_all_files" class="select-all checkbox"/>
-					<label for="select_all_files">
-						<span class="hidden-visually"><?php p($l->t('Select all'))?></span>
-					</label>
 					<a class="name sort columntitle" data-sort="name"><span><?php p($l->t( 'Name' )); ?></span><span class="sort-indicator"></span></a>
 					<span id="selectedActionsList" class="selectedActions">
 						<a href="" class="copy-move">
diff --git a/apps/files/tests/js/filelistSpec.js b/apps/files/tests/js/filelistSpec.js
index 8530e330d49..1f8d38c59ce 100644
--- a/apps/files/tests/js/filelistSpec.js
+++ b/apps/files/tests/js/filelistSpec.js
@@ -712,8 +712,14 @@ describe('OCA.Files.FileList tests', function() {
 				fileList.add(testFiles[i], {silent: true});
 			}
 
+			$tr = fileList.findFileEl('One.txt');
+			expect($tr.find('a.name').css('display')).not.toEqual('none');
+
 			// trigger rename prompt
 			fileList.rename('One.txt');
+
+			expect($tr.find('a.name').css('display')).toEqual('none');
+
 			$input = fileList.$fileList.find('input.filename');
 			$input.val('Two.jpg');
 
@@ -735,12 +741,12 @@ describe('OCA.Files.FileList tests', function() {
 			$tr = fileList.findFileEl('One.txt');
 			expect($tr.length).toEqual(1);
 			expect($tr.find('a .nametext').text().trim()).toEqual('One.txt');
-			expect($tr.find('a.name').is(':visible')).toEqual(true);
+			expect($tr.find('a.name').css('display')).not.toEqual('none');
 
 			$tr = fileList.findFileEl('Two.jpg');
 			expect($tr.length).toEqual(1);
 			expect($tr.find('a .nametext').text().trim()).toEqual('Two.jpg');
-			expect($tr.find('a.name').is(':visible')).toEqual(true);
+			expect($tr.find('a.name').css('display')).not.toEqual('none');
 
 			// input and form are gone
 			expect(fileList.$fileList.find('input.filename').length).toEqual(0);
@@ -1741,7 +1747,7 @@ describe('OCA.Files.FileList tests', function() {
 		it('Selects a file when clicking its checkbox', function() {
 			var $tr = fileList.findFileEl('One.txt');
 			expect($tr.find('input:checkbox').prop('checked')).toEqual(false);
-			$tr.find('td.filename input:checkbox').click();
+			$tr.find('td.selection input:checkbox').click();
 
 			expect($tr.find('input:checkbox').prop('checked')).toEqual(true);
 		});
@@ -1779,7 +1785,7 @@ describe('OCA.Files.FileList tests', function() {
 			var $tr = fileList.findFileEl('One.txt');
 			var $tr2 = fileList.findFileEl('Three.pdf');
 			var e;
-			$tr.find('td.filename input:checkbox').click();
+			$tr.find('td.selection input:checkbox').click();
 			e = new $.Event('click');
 			e.shiftKey = true;
 			$tr2.find('td.filename .name').trigger(e);
@@ -1797,7 +1803,7 @@ describe('OCA.Files.FileList tests', function() {
 			var $tr = fileList.findFileEl('One.txt');
 			var $tr2 = fileList.findFileEl('Three.pdf');
 			var e;
-			$tr2.find('td.filename input:checkbox').click();
+			$tr2.find('td.selection input:checkbox').click();
 			e = new $.Event('click');
 			e.shiftKey = true;
 			$tr.find('td.filename .name').trigger(e);
@@ -1813,13 +1819,13 @@ describe('OCA.Files.FileList tests', function() {
 		});
 		it('Selecting all files will automatically check "select all" checkbox', function() {
 			expect($('.select-all').prop('checked')).toEqual(false);
-			$('#fileList tr td.filename input:checkbox').click();
+			$('#fileList tr td.selection input:checkbox').click();
 			expect($('.select-all').prop('checked')).toEqual(true);
 		});
 		it('Selecting all files on the first visible page will not automatically check "select all" checkbox', function() {
 			fileList.setFiles(generateFiles(0, 41));
 			expect($('.select-all').prop('checked')).toEqual(false);
-			$('#fileList tr td.filename input:checkbox').click();
+			$('#fileList tr td.selection input:checkbox').click();
 			expect($('.select-all').prop('checked')).toEqual(false);
 		});
 		it('Selecting all files also selects hidden files when invisible', function() {
@@ -1831,7 +1837,7 @@ describe('OCA.Files.FileList tests', function() {
 				size: 150
 			}));
 			$('.select-all').click();
-			expect($tr.find('td.filename input:checkbox').prop('checked')).toEqual(true);
+			expect($tr.find('td.selection input:checkbox').prop('checked')).toEqual(true);
 			expect(_.pluck(fileList.getSelectedFiles(), 'name')).toContain('.hidden');
 		});
 		it('Clicking "select all" will select/deselect all files', function() {
diff --git a/apps/files_trashbin/templates/index.php b/apps/files_trashbin/templates/index.php
index c3f0304d408..b3ebca2867b 100644
--- a/apps/files_trashbin/templates/index.php
+++ b/apps/files_trashbin/templates/index.php
@@ -21,12 +21,14 @@
 <table id="filestable">
 	<thead>
 		<tr>
+			<th id="headerSelection" class="hidden column-selection">
+				<input type="checkbox" id="select_all_trash" class="select-all checkbox"/>
+				<label for="select_all_trash">
+					<span class="hidden-visually"><?php p($l->t('Select all'))?></span>
+				</label>
+			</th>
 			<th id='headerName' class="hidden column-name">
 				<div id="headerName-container">
-					<input type="checkbox" id="select_all_trash" class="select-all checkbox"/>
-					<label for="select_all_trash">
-						<span class="hidden-visually"><?php p($l->t('Select all'))?></span>
-					</label>
 					<a class="name sort columntitle" data-sort="name"><span><?php p($l->t( 'Name' )); ?></span><span class="sort-indicator"></span></a>
 					<span id="selectedActionsList" class='selectedActions'>
 						<a href="" class="undelete">