From e47fa9a4eb67bb7460a2355004430a083aca04f0 Mon Sep 17 00:00:00 2001
From: t11r <1674104+t11r@users.noreply.github.com>
Date: Sun, 1 Oct 2023 22:40:38 +0200
Subject: [PATCH] Fix button not closing popup, reduce redundancy

Since v-click-outside trumps touch listeners, we need to move the directive to a common parent element and change the DOM tree accordingly. Otherwise, the button does not close the popup on touch. Extend associated e2e tests.

The pagination buttons' DOM is now identical for large and small screens, so move them to a separate component.
---
 src/components/AppHeader.vue         | 401 +++++++++------------------
 src/components/PaginationButtons.vue |  67 +++++
 src/styles/sections/header.scss      |  44 +--
 tests/e2e/pagination.spec.js         |  19 +-
 4 files changed, 239 insertions(+), 292 deletions(-)
 create mode 100644 src/components/PaginationButtons.vue

diff --git a/src/components/AppHeader.vue b/src/components/AppHeader.vue
index 06a98613..142b2d11 100644
--- a/src/components/AppHeader.vue
+++ b/src/components/AppHeader.vue
@@ -28,72 +28,13 @@
 				</button>
 			</div>
 
-			<div class="tify-header-button-group -pagination">
-				<button
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isFirstPage"
-					:title="$translate('First page')"
-					@click="$store.goToFirstPage()"
-				>
-					<icon-page-first />
-				</button>
-
-				<button
-					v-if="$store.manifest.structures"
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isFirstPage"
-					:title="$translate('Previous section')"
-					@click="$store.goToPreviousSection()"
-				>
-					<icon-skip-previous />
-				</button>
-
-				<button
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isFirstPage"
-					:title="$translate('Previous page')"
-					@click="$store.goToPreviousPage()"
-				>
-					<icon-chevron-left />
-				</button>
-
-				<button
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isLastPage"
-					:title="$translate('Next page')"
-					@click="$store.goToNextPage()"
-				>
-					<icon-chevron-right />
-				</button>
-
-				<button
-					v-if="$store.manifest.structures"
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isLastSection"
-					:title="$translate('Next section')"
-					@click="$store.goToNextSection()"
-				>
-					<icon-skip-next />
-				</button>
-
-				<button
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isLastPage"
-					:title="$translate('Last page')"
-					@click="$store.goToLastPage()"
-				>
-					<icon-page-last />
-				</button>
-			</div>
+			<pagination-buttons />
 		</div>
 
-		<div class="tify-header-column -toggle">
+		<div
+			v-click-outside="closeControlsPopup"
+			class="tify-header-column -controls"
+		>
 			<div
 				ref="switchViewSmall"
 				class="tify-header-button-group -toggle"
@@ -105,212 +46,146 @@
 					:aria-label="$translate('View')"
 					class="tify-header-button"
 					:title="$translate('View')"
-					@click.stop="toggleControlsPopup"
+					@click="toggleControlsPopup"
 				>
 					<icon-dots-grid />
 				</button>
 			</div>
-		</div>
-
-		<div
-			:id="$store.getId('controls')"
-			v-click-outside="closeControlsPopup"
-			class="tify-header-column -controls"
-			:class="{ '-visible': controlsVisible }"
-		>
-			<div class="tify-header-button-group -view">
-				<!-- NOTE: This button is hidden on large containers -->
-				<button
-					v-if="$store.manifest"
-					type="button"
-					class="tify-header-button -scan"
-					:class="{ '-active': $store.options.view === 'scan' }"
-					:aria-controls="$store.getId('scan')"
-					:aria-expanded="$store.options.view === 'scan' ? 'true' : 'false'"
-					@click="toggleView('scan')"
-				>
-					<icon-image />
-					{{ $translate('Scan') }}
-				</button>
-
-				<button
-					v-if="fulltextEnabled"
-					type="button"
-					class="tify-header-button"
-					:class="{ '-active': $store.options.view === 'fulltext' }"
-					:aria-controls="$store.getId('fulltext')"
-					:aria-expanded="$store.options.view === 'fulltext' ? 'true' : 'false'"
-					@click="toggleView('fulltext')"
-				>
-					<icon-text-long />
-					{{ $translate('Fulltext') }}
-				</button>
-
-				<button
-					v-if="$store.manifest"
-					type="button"
-					class="tify-header-button"
-					:class="{ '-active': $store.options.view === 'thumbnails' }"
-					:aria-controls="$store.getId('thumbnails')"
-					:aria-expanded="$store.options.view === 'thumbnails' ? 'true' : 'false'"
-					@click="toggleView('thumbnails')"
-				>
-					<icon-view-module />
-					{{ $translate('Pages') }}
-				</button>
-
-				<button
-					v-if="tocEnabled"
-					type="button"
-					class="tify-header-button"
-					:class="{ '-active': $store.options.view === 'toc' }"
-					:aria-controls="$store.getId('toc')"
-					:aria-expanded="$store.options.view === 'toc' ? 'true' : 'false'"
-					@click="toggleView('toc')"
-				>
-					<icon-table-of-contents />
-					{{ $translate('Contents') }}
-				</button>
-
-				<button
-					type="button"
-					class="tify-header-button"
-					:class="{ '-active': $store.options.view === 'info' }"
-					:aria-controls="$store.getId('info')"
-					:aria-expanded="$store.options.view === 'info' ? 'true' : 'false'"
-					@click="toggleView('info')"
-				>
-					<icon-information-variant />
-					{{ $translate('Info') }}
-				</button>
-
-				<button
-					v-if="$store.manifest"
-					type="button"
-					class="tify-header-button"
-					:class="{ '-active': $store.options.view === 'export' }"
-					:aria-controls="$store.getId('export')"
-					:aria-expanded="$store.options.view === 'export' ? 'true' : 'false'"
-					@click="toggleView('export')"
-				>
-					<icon-download-outline />
-					{{ $translate('Export') }}
-				</button>
-
-				<button
-					v-if="$store.collection"
-					type="button"
-					class="tify-header-button"
-					:class="{ '-active': $store.options.view === 'collection' }"
-					:aria-controls="$store.getId('collection')"
-					:aria-expanded="$store.options === 'collection' ? 'true' : 'false'"
-					@click="toggleView('collection')"
-				>
-					<icon-list-box-outline />
-					{{ $translate('Collection') }}
-				</button>
-			</div>
-
-			<div v-if="fullscreenSupported" class="tify-header-button-group -view">
-				<button
-					type="button"
-					class="tify-header-button -icon-only"
-					:class="{ '-active': $store.options.view === 'help' }"
-					:aria-controls="$store.getId('help')"
-					:aria-expanded="$store.options.view === 'help' ? 'true' : 'false'"
-					:title="$translate('Help')"
-					@click="toggleView('help')"
-				>
-					<icon-help-circle-outline />
-					{{ $translate('Help') }}
-				</button>
-				<button
-					v-if="!fullscreenActive"
-					type="button"
-					class="tify-header-button -icon-only"
-					:title="$translate('Fullscreen')"
-					@click="toggleFullscreen"
-				>
-					<icon-fullscreen />
-					{{ $translate('Fullscreen') }}
-				</button>
-				<button
-					v-else
-					type="button"
-					class="tify-header-button -icon-only"
-					:title="$translate('Exit fullscreen')"
-					@click="toggleFullscreen"
-				>
-					<icon-fullscreen-exit />
-					{{ $translate('Exit fullscreen') }}
-				</button>
-			</div>
 
 			<div
-				v-if="$store.manifest"
-				class="tify-header-button-group -popup"
+				:id="$store.getId('controls')"
+				class="tify-header-popup"
+				:class="{ '-visible': controlsVisible }"
 			>
-				<button
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isFirstPage"
-					:title="$translate('First page')"
-					@click="$store.goToFirstPage()"
-				>
-					<icon-page-first />
-				</button>
-
-				<button
-					v-if="$store.manifest.structures"
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isFirstPage"
-					:title="$translate('Previous section')"
-					@click="$store.goToPreviousSection()"
-				>
-					<icon-skip-previous />
-				</button>
-
-				<button
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isFirstPage"
-					:title="$translate('Previous page')"
-					@click="$store.goToPreviousPage()"
-				>
-					<icon-chevron-left />
-				</button>
-
-				<button
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isLastPage"
-					:title="$translate('Next page')"
-					@click="$store.goToNextPage()"
-				>
-					<icon-chevron-right />
-				</button>
-
-				<button
-					v-if="$store.manifest.structures"
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isLastSection"
-					:title="$translate('Next section')"
-					@click="$store.goToNextSection()"
-				>
-					<icon-skip-next />
-				</button>
-
-				<button
-					type="button"
-					class="tify-header-button"
-					:disabled="$store.isCustomPageView || $store.isLastPage"
-					:title="$translate('Last page')"
-					@click="$store.goToLastPage()"
-				>
-					<icon-page-last />
-				</button>
+				<div class="tify-header-button-group -view">
+					<!-- NOTE: This button is hidden on large containers -->
+					<button
+						v-if="$store.manifest"
+						type="button"
+						class="tify-header-button -scan"
+						:class="{ '-active': $store.options.view === 'scan' }"
+						:aria-controls="$store.getId('scan')"
+						:aria-expanded="$store.options.view === 'scan' ? 'true' : 'false'"
+						@click="toggleView('scan')"
+					>
+						<icon-image />
+						{{ $translate('Scan') }}
+					</button>
+
+					<button
+						v-if="fulltextEnabled"
+						type="button"
+						class="tify-header-button"
+						:class="{ '-active': $store.options.view === 'fulltext' }"
+						:aria-controls="$store.getId('fulltext')"
+						:aria-expanded="$store.options.view === 'fulltext' ? 'true' : 'false'"
+						@click="toggleView('fulltext')"
+					>
+						<icon-text-long />
+						{{ $translate('Fulltext') }}
+					</button>
+
+					<button
+						v-if="$store.manifest"
+						type="button"
+						class="tify-header-button"
+						:class="{ '-active': $store.options.view === 'thumbnails' }"
+						:aria-controls="$store.getId('thumbnails')"
+						:aria-expanded="$store.options.view === 'thumbnails' ? 'true' : 'false'"
+						@click="toggleView('thumbnails')"
+					>
+						<icon-view-module />
+						{{ $translate('Pages') }}
+					</button>
+
+					<button
+						v-if="tocEnabled"
+						type="button"
+						class="tify-header-button"
+						:class="{ '-active': $store.options.view === 'toc' }"
+						:aria-controls="$store.getId('toc')"
+						:aria-expanded="$store.options.view === 'toc' ? 'true' : 'false'"
+						@click="toggleView('toc')"
+					>
+						<icon-table-of-contents />
+						{{ $translate('Contents') }}
+					</button>
+
+					<button
+						type="button"
+						class="tify-header-button"
+						:class="{ '-active': $store.options.view === 'info' }"
+						:aria-controls="$store.getId('info')"
+						:aria-expanded="$store.options.view === 'info' ? 'true' : 'false'"
+						@click="toggleView('info')"
+					>
+						<icon-information-variant />
+						{{ $translate('Info') }}
+					</button>
+
+					<button
+						v-if="$store.manifest"
+						type="button"
+						class="tify-header-button"
+						:class="{ '-active': $store.options.view === 'export' }"
+						:aria-controls="$store.getId('export')"
+						:aria-expanded="$store.options.view === 'export' ? 'true' : 'false'"
+						@click="toggleView('export')"
+					>
+						<icon-download-outline />
+						{{ $translate('Export') }}
+					</button>
+
+					<button
+						v-if="$store.collection"
+						type="button"
+						class="tify-header-button"
+						:class="{ '-active': $store.options.view === 'collection' }"
+						:aria-controls="$store.getId('collection')"
+						:aria-expanded="$store.options === 'collection' ? 'true' : 'false'"
+						@click="toggleView('collection')"
+					>
+						<icon-list-box-outline />
+						{{ $translate('Collection') }}
+					</button>
+				</div>
+
+				<div v-if="fullscreenSupported" class="tify-header-button-group -view">
+					<button
+						type="button"
+						class="tify-header-button -icon-only"
+						:class="{ '-active': $store.options.view === 'help' }"
+						:aria-controls="$store.getId('help')"
+						:aria-expanded="$store.options.view === 'help' ? 'true' : 'false'"
+						:title="$translate('Help')"
+						@click="toggleView('help')"
+					>
+						<icon-help-circle-outline />
+						{{ $translate('Help') }}
+					</button>
+					<button
+						v-if="!fullscreenActive"
+						type="button"
+						class="tify-header-button -icon-only"
+						:title="$translate('Fullscreen')"
+						@click="toggleFullscreen"
+					>
+						<icon-fullscreen />
+						{{ $translate('Fullscreen') }}
+					</button>
+					<button
+						v-else
+						type="button"
+						class="tify-header-button -icon-only"
+						:title="$translate('Exit fullscreen')"
+						@click="toggleFullscreen"
+					>
+						<icon-fullscreen-exit />
+						{{ $translate('Exit fullscreen') }}
+					</button>
+				</div>
+
+				<pagination-buttons v-if="$store.manifest" />
 			</div>
 		</div>
 	</header>
diff --git a/src/components/PaginationButtons.vue b/src/components/PaginationButtons.vue
new file mode 100644
index 00000000..24ee99c3
--- /dev/null
+++ b/src/components/PaginationButtons.vue
@@ -0,0 +1,67 @@
+<template>
+	<div class="tify-header-button-group -pagination">
+		<button
+			type="button"
+			class="tify-header-button"
+			:disabled="$store.isCustomPageView || $store.isFirstPage"
+			:title="$translate('First page')"
+			@click="$store.goToFirstPage()"
+		>
+			<icon-page-first />
+		</button>
+
+		<button
+			v-if="$store.manifest.structures"
+			type="button"
+			class="tify-header-button"
+			:disabled="$store.isCustomPageView || $store.isFirstPage"
+			:title="$translate('Previous section')"
+			@click="$store.goToPreviousSection()"
+		>
+			<icon-skip-previous />
+		</button>
+
+		<button
+			type="button"
+			class="tify-header-button"
+			:disabled="$store.isCustomPageView || $store.isFirstPage"
+			:title="$translate('Previous page')"
+			@click="$store.goToPreviousPage()"
+		>
+			<icon-chevron-left />
+		</button>
+
+		<button
+			type="button"
+			class="tify-header-button"
+			:disabled="$store.isCustomPageView || $store.isLastPage"
+			:title="$translate('Next page')"
+			@click="$store.goToNextPage()"
+		>
+			<icon-chevron-right />
+		</button>
+
+		<button
+			v-if="$store.manifest.structures"
+			type="button"
+			class="tify-header-button"
+			:disabled="$store.isCustomPageView || $store.isLastSection"
+			:title="$translate('Next section')"
+			@click="$store.goToNextSection()"
+		>
+			<icon-skip-next />
+		</button>
+
+		<button
+			type="button"
+			class="tify-header-button"
+			:disabled="$store.isCustomPageView || $store.isLastPage"
+			:title="$translate('Last page')"
+			@click="$store.goToLastPage()"
+		>
+			<icon-page-last />
+		</button>
+	</div>
+</template>
+
+<script />
diff --git a/src/styles/sections/header.scss b/src/styles/sections/header.scss
index 6d6c7564..43a06ad7 100644
--- a/src/styles/sections/header.scss
+++ b/src/styles/sections/header.scss
@@ -30,20 +30,26 @@
 		.tify.-small & {
 			display: none;
 		}
+
+		.tify-header-popup & {
+			box-shadow: 0 -1px $border-color;
+			display: none;
+			margin: calc(#{$br} * 2 - 1px) 0 0;
+			padding: $br 0 0;
+
+			.tify.-small & {
+				display: flex;
+			}
+		}
 	}
 
-	&.-popup {
-		box-shadow: 0 -1px $border-color;
+	&.-toggle {
 		display: none;
-		margin: calc(#{$br} * 2 - 1px) 0 0;
-		padding: $br 0 0;
 
-		.tify.-small & {
+		.tify.-large & {
 			display: flex;
 		}
-	}
 
-	&.-toggle {
 		.tify-header-column:not(:nth-child(2)) & {
 			border-left: 1px solid $border-color;
 
@@ -173,27 +179,21 @@
 	&:first-child {
 		flex: 1;
 	}
+}
 
-	&.-toggle {
+.tify-header-popup {
+	display: flex;
+
+	.tify.-large & {
+		@include dropdown(right);
+		box-shadow: none;
 		display: none;
+		top: g(2);
 
-		.tify.-large & {
+		&.-visible {
 			display: block;
 		}
 	}
-
-	&.-controls {
-		.tify.-large & {
-			@include dropdown(right);
-			box-shadow: none;
-			display: none;
-			top: g(2);
-
-			&.-visible {
-				display: block;
-			}
-		}
-	}
 }
 
 .tify-header-title {
diff --git a/tests/e2e/pagination.spec.js b/tests/e2e/pagination.spec.js
index 67c92c12..3687d0ae 100644
--- a/tests/e2e/pagination.spec.js
+++ b/tests/e2e/pagination.spec.js
@@ -77,19 +77,24 @@ describe('Pagination', () => {
 		cy.contains('.-current.-highlighted', '69 : -');
 	});
 
-	it('changes the page on a small touchscreen', () => {
+	it('changes the page on small touchscreens', () => {
 		cy.viewport(375, 667);
 		cy.visit(`/?manifest=${Cypress.env('iiifApiUrl')}/manifest/gdz-HANS_DE_7_w042081`);
 
 		cy.get('[title="View"]').click();
-		cy.get('.tify-header-button-group.-popup [title="Last page"]').click();
-		cy.get('.tify-header-button-group.-popup [title="Next page"]').should('be.disabled');
-		cy.get('.tify-header-button-group.-popup [title="Next section"]').should('be.disabled');
-		cy.get('.tify-header-button-group.-popup [title="Last page"]').should('be.disabled');
+		cy.contains('Pages').should('be.visible');
+		cy.get('[title="View"]').click();
+		cy.contains('Pages').should('not.be.visible');
+
+		cy.get('[title="View"]').click();
+		cy.get('.tify-header-popup [title="Last page"]').click();
+		cy.get('.tify-header-popup [title="Next page"]').should('be.disabled');
+		cy.get('.tify-header-popup [title="Next section"]').should('be.disabled');
+		cy.get('.tify-header-popup [title="Last page"]').should('be.disabled');
 
-		cy.get('.tify-header-button-group.-popup [title="Previous section"]').click();
+		cy.get('.tify-header-popup [title="Previous section"]').click();
 		cy.contains(currentPage, '67 : -');
 		cy.get('.tify-header').click();
-		cy.get('.tify-header-column.-controls').should('not.be.visible');
+		cy.get('.tify-header-popup').should('not.be.visible');
 	});
 });