From 680f439f736e199576844cd2d3cc3854e95201af Mon Sep 17 00:00:00 2001
From: "Grigorii K. Shartsev" <me@shgk.me>
Date: Fri, 1 Mar 2024 18:59:03 +0100
Subject: [PATCH] fix(settings): emails actions a11y and design

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
---
 .../PersonalInfo/EmailSection/Email.vue       | 147 +++++++-------
 .../EmailSection/EmailSection.vue             |  12 +-
 .../PersonalInfo/shared/FederationControl.vue | 113 ++++++++++-
 .../shared/FederationControlActions.vue       | 181 ------------------
 cypress/e2e/settings/personal-info.cy.ts      |   6 +-
 5 files changed, 194 insertions(+), 265 deletions(-)
 delete mode 100644 apps/settings/src/components/PersonalInfo/shared/FederationControlActions.vue

diff --git a/apps/settings/src/components/PersonalInfo/EmailSection/Email.vue b/apps/settings/src/components/PersonalInfo/EmailSection/Email.vue
index 99917bb4f7e..e1e0bc625ba 100644
--- a/apps/settings/src/components/PersonalInfo/EmailSection/Email.vue
+++ b/apps/settings/src/components/PersonalInfo/EmailSection/Email.vue
@@ -2,6 +2,7 @@
 	- @copyright 2021, Christopher Ng <chrng8@gmail.com>
 	-
 	- @author Christopher Ng <chrng8@gmail.com>
+	- @author Grigorii K. Shartsev <me@shgk.me>
 	-
 	- @license GNU AGPL version 3 or any later version
 	-
@@ -22,55 +23,35 @@
 
 <template>
 	<div>
-		<div class="email">
-			<NcInputField :id="inputIdWithDefault"
-				ref="email"
-				autocapitalize="none"
-				autocomplete="email"
-				:error="hasError || !!helperText"
-				:helper-text="helperText || undefined"
-				:label="inputPlaceholder"
-				:placeholder="inputPlaceholder"
-				spellcheck="false"
-				:success="isSuccess"
-				type="email"
-				:value.sync="emailAddress" />
+		<div class="email" :class="{ 'email--additional': !primary }">
+			<div v-if="!primary" class="email__label-container">
+				<label :for="inputIdWithDefault">{{ inputPlaceholder }}</label>
+				<FederationControl v-if="!federationDisabled && !primary"
+					:readable="propertyReadable"
+					:additional="true"
+					:additional-value="email"
+					:disabled="federationDisabled"
+					:handle-additional-scope-change="saveAdditionalEmailScope"
+					:scope.sync="localScope"
+					@update:scope="onScopeChange" />
+			</div>
+			<div class="email__input-container">
+				<NcTextField :id="inputIdWithDefault"
+					ref="email"
+					class="email__input"
+					autocapitalize="none"
+					autocomplete="email"
+					:error="hasError || !!helperText"
+					:helper-text="helperTextWithNonConfirmed"
+					label-outside
+					:placeholder="inputPlaceholder"
+					spellcheck="false"
+					:success="isSuccess"
+					type="email"
+					:value.sync="emailAddress" />
 
-			<div class="email__actions">
-				<NcActions :aria-label="actionsLabel" @close="showFederationSettings = false">
-					<template v-if="showFederationSettings">
-						<NcActionButton @click="showFederationSettings = false">
-							<template #icon>
-								<NcIconSvgWrapper :path="mdiArrowLeft" />
-							</template>
-							{{ t('settings', 'Back') }}
-						</NcActionButton>
-						<FederationControlActions :readable="propertyReadable"
-							:additional="true"
-							:additional-value="email"
-							:disabled="federationDisabled"
-							:handle-additional-scope-change="saveAdditionalEmailScope"
-							:scope.sync="localScope"
-							@update:scope="onScopeChange" />
-					</template>
-					<template v-else>
-						<NcActionButton v-if="!federationDisabled && !primary"
-							@click="showFederationSettings = true">
-							<template #icon>
-								<NcIconSvgWrapper :path="mdiLock" />
-							</template>
-							{{ t('settings', 'Change scope level of {property}', { property: propertyReadable.toLocaleLowerCase() }) }}
-						</NcActionButton>
-						<NcActionCaption v-if="!isConfirmedAddress"
-							:name="t('settings', 'This address is not confirmed')" />
-						<NcActionButton close-after-click
-							:disabled="deleteDisabled"
-							@click="deleteEmail">
-							<template #icon>
-								<NcIconSvgWrapper :path="mdiTrashCan" />
-							</template>
-							{{ deleteEmailLabel }}
-						</NcActionButton>
+				<div class="email__actions">
+					<NcActions :aria-label="actionsLabel">
 						<NcActionButton v-if="!primary || !isNotificationEmail"
 							close-after-click
 							:disabled="!isConfirmedAddress"
@@ -81,8 +62,16 @@
 							</template>
 							{{ setNotificationMailLabel }}
 						</NcActionButton>
-					</template>
-				</NcActions>
+						<NcActionButton close-after-click
+							:disabled="deleteDisabled"
+							@click="deleteEmail">
+							<template #icon>
+								<NcIconSvgWrapper :path="mdiTrashCan" />
+							</template>
+							{{ deleteEmailLabel }}
+						</NcActionButton>
+					</NcActions>
+				</div>
 			</div>
 		</div>
 
@@ -95,13 +84,14 @@
 <script>
 import NcActions from '@nextcloud/vue/dist/Components/NcActions.js'
 import NcActionButton from '@nextcloud/vue/dist/Components/NcActionButton.js'
-import NcActionCaption from '@nextcloud/vue/dist/Components/NcActionCaption.js'
 import NcIconSvgWrapper from '@nextcloud/vue/dist/Components/NcIconSvgWrapper.js'
-import NcInputField from '@nextcloud/vue/dist/Components/NcInputField.js'
+import NcTextField from '@nextcloud/vue/dist/Components/NcTextField.js'
+
 import debounce from 'debounce'
 
 import { mdiArrowLeft, mdiLock, mdiStar, mdiStarOutline, mdiTrashCan } from '@mdi/js'
-import FederationControlActions from '../shared/FederationControlActions.vue'
+
+import FederationControl from '../shared/FederationControl.vue'
 import { handleError } from '../../../utils/handlers.js'
 
 import { ACCOUNT_PROPERTY_READABLE_ENUM, VERIFICATION_ENUM } from '../../../constants/AccountPropertyConstants.js'
@@ -121,10 +111,9 @@ export default {
 	components: {
 		NcActions,
 		NcActionButton,
-		NcActionCaption,
 		NcIconSvgWrapper,
-		NcInputField,
-		FederationControlActions,
+		NcTextField,
+		FederationControl,
 	},
 
 	props: {
@@ -213,6 +202,20 @@ export default {
 			return this.primary || this.localVerificationState === VERIFICATION_ENUM.VERIFIED
 		},
 
+		isNotConfirmedHelperText() {
+			if (!this.isConfirmedAddress) {
+				return t('settings', 'This address is not confirmed')
+			}
+			return ''
+		},
+
+		helperTextWithNonConfirmed() {
+			if (this.helperText || this.hasError || this.isSuccess) {
+				return this.helperText || ''
+			}
+			return this.isNotConfirmedHelperText
+		},
+
 		setNotificationMailLabel() {
 			if (this.isNotificationEmail) {
 				return t('settings', 'Unset as primary email')
@@ -259,7 +262,8 @@ export default {
 
 	methods: {
 		debounceEmailChange: debounce(async function(email) {
-			this.helperText = this.$refs.email?.$refs.input?.validationMessage || null
+			// TODO: provide method to get native input in NcTextField
+			this.helperText = this.$refs.email.$refs.inputField.$refs.input.validationMessage || null
 			if (this.helperText !== null) {
 				return
 			}
@@ -403,16 +407,29 @@ export default {
 
 <style lang="scss" scoped>
 .email {
-	display: flex;
-	flex-direction: row;
-	align-items: start;
-	gap: 4px;
+	&__label-container {
+		height: var(--default-clickable-area);
+		display: flex;
+		flex-direction: row;
+		align-items: center;
+		gap: calc(var(--default-grid-baseline) * 2);
+	}
+
+	&__input-container {
+		position: relative;
+	}
+
+	&__input {
+		// TODO: provide a way to hide status icon or combine it with trailing button in NcInputField
+		:deep(.input-field__icon--trailing) {
+			display: none;
+		}
+	}
 
 	&__actions {
-		display: flex;
-		gap: 0 2px;
-		margin-right: 5px;
-		margin-top: 6px;
+		position: absolute;
+		inset-block-start: 0;
+		inset-inline-end: 0;
 	}
 }
 </style>
diff --git a/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue b/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue
index 16a866c6ee9..55b68fa5933 100644
--- a/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue
+++ b/apps/settings/src/components/PersonalInfo/EmailSection/EmailSection.vue
@@ -2,6 +2,7 @@
 	- @copyright 2021, Christopher Ng <chrng8@gmail.com>
 	-
 	- @author Christopher Ng <chrng8@gmail.com>
+	- @author Grigorii K. Shartsev <me@shgk.me>
 	-
 	- @license GNU AGPL version 3 or any later version
 	-
@@ -21,7 +22,7 @@
 -->
 
 <template>
-	<section>
+	<section class="section-emails">
 		<HeaderBar :input-id="inputId"
 			:readable="primaryEmail.readable"
 			:is-editable="true"
@@ -45,10 +46,10 @@
 		</span>
 
 		<template v-if="additionalEmails.length">
-			<em class="additional-emails-label">{{ t('settings', 'Additional emails') }}</em>
 			<!-- TODO use unique key for additional email when uniqueness can be guaranteed, see https://github.com/nextcloud/server/issues/26866 -->
 			<Email v-for="(additionalEmail, index) in additionalEmails"
 				:key="additionalEmail.key"
+				class="section-emails__additional-email"
 				:index="index"
 				:scope.sync="additionalEmail.scope"
 				:email.sync="additionalEmail.value"
@@ -196,12 +197,11 @@ export default {
 </script>
 
 <style lang="scss" scoped>
-section {
+.section-emails {
 	padding: 10px 10px;
 
-	.additional-emails-label {
-		display: block;
-		margin-top: 16px;
+	&__additional-email {
+		margin-top: calc(var(--default-grid-baseline) * 3);
 	}
 }
 </style>
diff --git a/apps/settings/src/components/PersonalInfo/shared/FederationControl.vue b/apps/settings/src/components/PersonalInfo/shared/FederationControl.vue
index 64f603932b6..b8dd7f18737 100644
--- a/apps/settings/src/components/PersonalInfo/shared/FederationControl.vue
+++ b/apps/settings/src/components/PersonalInfo/shared/FederationControl.vue
@@ -3,6 +3,7 @@
 	-
 	- @author Christopher Ng <chrng8@gmail.com>
 	- @author Ferdinand Thiessen <opensource@fthiessen.de>
+	- @author Grigorii K. Shartsev <me@shgk.me>
 	-
 	- @license GNU AGPL version 3 or any later version
 	-
@@ -24,39 +25,60 @@
 <template>
 	<NcActions ref="federationActions"
 		class="federation-actions"
-		:class="{ 'federation-actions--additional': additional }"
 		:aria-label="ariaLabel"
 		:disabled="disabled">
 		<template #icon>
 			<NcIconSvgWrapper :path="scopeIcon" />
 		</template>
-		<FederationControlActions :additional="additional"
-			:additional-value="additionalValue"
-			:handle-additional-scope-change="handleAdditionalScopeChange"
-			:readable="readable"
-			:scope="scope"
-			@update:scope="onUpdateScope" />
+
+		<NcActionButton v-for="federationScope in federationScopes"
+			:key="federationScope.name"
+			:close-after-click="true"
+			:disabled="!supportedScopes.includes(federationScope.name)"
+			:name="federationScope.displayName"
+			type="radio"
+			:value="federationScope.name"
+			:model-value="scope"
+			@update:modelValue="changeScope">
+			<template #icon>
+				<NcIconSvgWrapper :path="federationScope.icon" />
+			</template>
+			{{ supportedScopes.includes(federationScope.name) ? federationScope.tooltip : federationScope.tooltipDisabled }}
+		</NcActionButton>
 	</NcActions>
 </template>
 
 <script>
 import NcActions from '@nextcloud/vue/dist/Components/NcActions.js'
+import NcActionButton from '@nextcloud/vue/dist/Components/NcActionButton.js'
 import NcIconSvgWrapper from '@nextcloud/vue/dist/Components/NcIconSvgWrapper.js'
+import { loadState } from '@nextcloud/initial-state'
+
 import {
 	ACCOUNT_PROPERTY_READABLE_ENUM,
 	ACCOUNT_SETTING_PROPERTY_READABLE_ENUM,
 	PROFILE_READABLE_ENUM,
+	PROPERTY_READABLE_KEYS_ENUM,
+	PROPERTY_READABLE_SUPPORTED_SCOPES_ENUM,
 	SCOPE_PROPERTY_ENUM,
+	SCOPE_ENUM,
+	UNPUBLISHED_READABLE_PROPERTIES,
 } from '../../../constants/AccountPropertyConstants.js'
-import FederationControlActions from './FederationControlActions.vue'
+import { savePrimaryAccountPropertyScope } from '../../../service/PersonalInfo/PersonalInfoService.js'
+import { handleError } from '../../../utils/handlers.js'
+
+const {
+	federationEnabled,
+	lookupServerUploadEnabled,
+} = loadState('settings', 'accountParameters', {})
 
 export default {
 	name: 'FederationControl',
 
 	components: {
 		NcActions,
+		NcActionButton,
 		NcIconSvgWrapper,
-		FederationControlActions,
 	},
 
 	props: {
@@ -87,9 +109,12 @@ export default {
 		},
 	},
 
+	emits: ['update:scope'],
+
 	data() {
 		return {
 			readableLowerCase: this.readable.toLocaleLowerCase(),
+			initialScope: this.scope,
 		}
 	},
 
@@ -105,14 +130,82 @@ export default {
 		scopeIcon() {
 			return SCOPE_PROPERTY_ENUM[this.scope].icon
 		},
+
+		federationScopes() {
+			return Object.values(SCOPE_PROPERTY_ENUM)
+		},
+
+		supportedScopes() {
+			const scopes = PROPERTY_READABLE_SUPPORTED_SCOPES_ENUM[this.readable]
+
+			if (UNPUBLISHED_READABLE_PROPERTIES.includes(this.readable)) {
+				return scopes
+			}
+
+			if (federationEnabled) {
+				scopes.push(SCOPE_ENUM.FEDERATED)
+			}
+
+			if (lookupServerUploadEnabled) {
+				scopes.push(SCOPE_ENUM.PUBLISHED)
+			}
+
+			return scopes
+		},
 	},
 
 	methods: {
-		onUpdateScope(scope) {
+		async changeScope(scope) {
 			this.$emit('update:scope', scope)
+
+			if (!this.additional) {
+				await this.updatePrimaryScope(scope)
+			} else {
+				await this.updateAdditionalScope(scope)
+			}
+
 			// TODO: provide focus method from NcActions
 			this.$refs.federationActions.$refs.menuButton.$el.focus()
 		},
+
+		async updatePrimaryScope(scope) {
+			try {
+				const responseData = await savePrimaryAccountPropertyScope(PROPERTY_READABLE_KEYS_ENUM[this.readable], scope)
+				this.handleResponse({
+					scope,
+					status: responseData.ocs?.meta?.status,
+				})
+			} catch (e) {
+				this.handleResponse({
+					errorMessage: t('settings', 'Unable to update federation scope of the primary {property}', { property: this.readableLowerCase }),
+					error: e,
+				})
+			}
+		},
+
+		async updateAdditionalScope(scope) {
+			try {
+				const responseData = await this.handleAdditionalScopeChange(this.additionalValue, scope)
+				this.handleResponse({
+					scope,
+					status: responseData.ocs?.meta?.status,
+				})
+			} catch (e) {
+				this.handleResponse({
+					errorMessage: t('settings', 'Unable to update federation scope of additional {property}', { property: this.readableLowerCase }),
+					error: e,
+				})
+			}
+		},
+
+		handleResponse({ scope, status, errorMessage, error }) {
+			if (status === 'ok') {
+				this.initialScope = scope
+			} else {
+				this.$emit('update:scope', this.initialScope)
+				handleError(error, errorMessage)
+			}
+		},
 	},
 }
 </script>
diff --git a/apps/settings/src/components/PersonalInfo/shared/FederationControlActions.vue b/apps/settings/src/components/PersonalInfo/shared/FederationControlActions.vue
deleted file mode 100644
index d37d7fa2fba..00000000000
--- a/apps/settings/src/components/PersonalInfo/shared/FederationControlActions.vue
+++ /dev/null
@@ -1,181 +0,0 @@
-<!--
-	- @copyright 2021, Christopher Ng <chrng8@gmail.com>
-	-
-	- @author Christopher Ng <chrng8@gmail.com>
-	- @author Ferdinand Thiessen <opensource@fthiessen.de>
-	-
-	- @license GNU AGPL version 3 or any later version
-	-
-	- This program is free software: you can redistribute it and/or modify
-	- it under the terms of the GNU Affero General Public License as
-	- published by the Free Software Foundation, either version 3 of the
-	- License, or (at your option) any later version.
-	-
-	- This program is distributed in the hope that it will be useful,
-	- but WITHOUT ANY WARRANTY; without even the implied warranty of
-	- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-	- GNU Affero General Public License for more details.
-	-
-	- You should have received a copy of the GNU Affero General Public License
-	- along with this program. If not, see <http://www.gnu.org/licenses/>.
-	-
--->
-
-<template>
-	<Fragment>
-		<NcActionButton v-for="federationScope in federationScopes"
-			:key="federationScope.name"
-			:close-after-click="true"
-			:disabled="!supportedScopes.includes(federationScope.name)"
-			:name="federationScope.displayName"
-			type="radio"
-			:value="federationScope.name"
-			:model-value="scope"
-			@update:modelValue="changeScope">
-			<template #icon>
-				<NcIconSvgWrapper :path="federationScope.icon" />
-			</template>
-			{{ supportedScopes.includes(federationScope.name) ? federationScope.tooltip : federationScope.tooltipDisabled }}
-		</NcActionButton>
-	</Fragment>
-</template>
-
-<script>
-import NcActionButton from '@nextcloud/vue/dist/Components/NcActionButton.js'
-import NcIconSvgWrapper from '@nextcloud/vue/dist/Components/NcIconSvgWrapper.js'
-import { loadState } from '@nextcloud/initial-state'
-import { Fragment } from 'vue-frag'
-
-import {
-	ACCOUNT_PROPERTY_READABLE_ENUM,
-	ACCOUNT_SETTING_PROPERTY_READABLE_ENUM,
-	PROFILE_READABLE_ENUM,
-	PROPERTY_READABLE_KEYS_ENUM,
-	PROPERTY_READABLE_SUPPORTED_SCOPES_ENUM,
-	SCOPE_ENUM, SCOPE_PROPERTY_ENUM,
-	UNPUBLISHED_READABLE_PROPERTIES,
-} from '../../../constants/AccountPropertyConstants.js'
-import { savePrimaryAccountPropertyScope } from '../../../service/PersonalInfo/PersonalInfoService.js'
-import { handleError } from '../../../utils/handlers.js'
-
-const {
-	federationEnabled,
-	lookupServerUploadEnabled,
-} = loadState('settings', 'accountParameters', {})
-
-export default {
-	name: 'FederationControlActions',
-
-	components: {
-		Fragment,
-		NcActionButton,
-		NcIconSvgWrapper,
-	},
-
-	props: {
-		readable: {
-			type: String,
-			required: true,
-			validator: (value) => Object.values(ACCOUNT_PROPERTY_READABLE_ENUM).includes(value) || Object.values(ACCOUNT_SETTING_PROPERTY_READABLE_ENUM).includes(value) || value === PROFILE_READABLE_ENUM.PROFILE_VISIBILITY,
-		},
-		additional: {
-			type: Boolean,
-			default: false,
-		},
-		additionalValue: {
-			type: String,
-			default: '',
-		},
-		handleAdditionalScopeChange: {
-			type: Function,
-			default: null,
-		},
-		scope: {
-			type: String,
-			required: true,
-		},
-	},
-
-	data() {
-		return {
-			readableLowerCase: this.readable.toLocaleLowerCase(),
-			initialScope: this.scope,
-		}
-	},
-
-	computed: {
-		federationScopes() {
-			return Object.values(SCOPE_PROPERTY_ENUM)
-		},
-
-		supportedScopes() {
-			const scopes = PROPERTY_READABLE_SUPPORTED_SCOPES_ENUM[this.readable]
-
-			if (UNPUBLISHED_READABLE_PROPERTIES.includes(this.readable)) {
-				return scopes
-			}
-
-			if (federationEnabled) {
-				scopes.push(SCOPE_ENUM.FEDERATED)
-			}
-
-			if (lookupServerUploadEnabled) {
-				scopes.push(SCOPE_ENUM.PUBLISHED)
-			}
-
-			return scopes
-		},
-	},
-
-	methods: {
-		async changeScope(scope) {
-			this.$emit('update:scope', scope)
-
-			if (!this.additional) {
-				await this.updatePrimaryScope(scope)
-			} else {
-				await this.updateAdditionalScope(scope)
-			}
-		},
-
-		async updatePrimaryScope(scope) {
-			try {
-				const responseData = await savePrimaryAccountPropertyScope(PROPERTY_READABLE_KEYS_ENUM[this.readable], scope)
-				this.handleResponse({
-					scope,
-					status: responseData.ocs?.meta?.status,
-				})
-			} catch (e) {
-				this.handleResponse({
-					errorMessage: t('settings', 'Unable to update federation scope of the primary {property}', { property: this.readableLowerCase }),
-					error: e,
-				})
-			}
-		},
-
-		async updateAdditionalScope(scope) {
-			try {
-				const responseData = await this.handleAdditionalScopeChange(this.additionalValue, scope)
-				this.handleResponse({
-					scope,
-					status: responseData.ocs?.meta?.status,
-				})
-			} catch (e) {
-				this.handleResponse({
-					errorMessage: t('settings', 'Unable to update federation scope of additional {property}', { property: this.readableLowerCase }),
-					error: e,
-				})
-			}
-		},
-
-		handleResponse({ scope, status, errorMessage, error }) {
-			if (status === 'ok') {
-				this.initialScope = scope
-			} else {
-				this.$emit('update:scope', this.initialScope)
-				handleError(error, errorMessage)
-			}
-		},
-	},
-}
-</script>
diff --git a/cypress/e2e/settings/personal-info.cy.ts b/cypress/e2e/settings/personal-info.cy.ts
index d64cadbe614..a7564d5125e 100644
--- a/cypress/e2e/settings/personal-info.cy.ts
+++ b/cypress/e2e/settings/personal-info.cy.ts
@@ -57,9 +57,9 @@ const validateActiveVisibility = (property: string, active: Visibility) => {
 		.and('match', new RegExp(`current scope is ${active}`, 'i'))
 	getVisibilityButton(property)
 		.click()
-	cy.get('ul[role="dialog"')
+	cy.get('ul[role="menu"]')
 		.contains('button', active)
-		.should('have.attr', 'aria-pressed', 'true')
+		.should('have.attr', 'aria-checked', 'true')
 
 	// close menu
 	getVisibilityButton(property)
@@ -74,7 +74,7 @@ const validateActiveVisibility = (property: string, active: Visibility) => {
 const setActiveVisibility = (property: string, active: Visibility) => {
 	getVisibilityButton(property)
 		.click()
-	cy.get('ul[role="dialog"')
+	cy.get('ul[role="menu"]')
 		.contains('button', active)
 		.click({ force: true })
 	handlePasswordConfirmation(user.password)