tag selector focus edits, fix windowing breakage (#3984)

* tag selector focus edits

- no tabstop on the tag selector scrollable area
- change tag selector's role from default "grid" to "group".
"grid" is not quite correct semantically and leads to
voiceover suggesting irrelevant commands.
- move all keyboard handling logic to tagSelectorList.jsx.
Tabbing through tag selector is now handled in ZoteroPane,
so the only logic left there is arrow navigation
between tags, and there's no reason to not have it
together with the tags list.
- a workaround to deal with focused tags when windowing
kicks in. When a tag is focused, record its index.
Each time tags are re-rendered, if the saved index is not
among rendered tags, refocus it, otherwise, move focus
to the tags list.

chrome/content/zotero/components/tagSelector/tagSelectorList.jsx

@@ -46,6 +46,12 @@ class TagList extends React.PureComponent {
 		this.collectionRef = React.createRef();
 		this.scrollToTopOnNextUpdate = false;
 		this.prevTagCount = 0;
+		this.focusedTagIndex = null;
+		this.lastFocusedTagIndex = null;
+		this.resolveTagsRenderedPromise = null;
+		this.state = {
+			scrollToCell: null
+		};
 	}
 	
 	componentDidUpdate(prevProps) {
@@ -159,7 +165,7 @@ class TagList extends React.PureComponent {
 		
 		const { onDragOver, onDragExit, onDrop } = this.props.dragObserver;
 		
-		var className = 'tag-selector-item';
+		var className = 'tag-selector-item keyboard-clickable';
 		if (tag.selected) {
 			className += ' selected';
 		}
@@ -177,10 +183,13 @@ class TagList extends React.PureComponent {
 			className,
 			onClick: ev => !tag.disabled && this.props.onSelect(tag.name, ev),
 			onContextMenu: ev => this.props.onTagContext(tag, ev),
-			onKeyDown: ev => this.props.onKeyDown(ev),
 			onDragOver,
 			onDragExit,
 			onDrop,
+			onFocus: (_) => {
+				this.lastFocusedTagIndex = this.focusedTagIndex;
+				this.focusedTagIndex = index;
+			}
 		};
 		
 		props.style = {
@@ -214,6 +223,84 @@ class TagList extends React.PureComponent {
 			</div>
 		);
 	};
+
+	// Try to refocus a focused tag that was removed due to windowing
+	refocusTag() {
+		let tagsList = document.querySelector('.tag-selector-list');
+		let tagsNodes = [...tagsList.querySelectorAll(".tag-selector-item")];
+		let tagToFocus = this.props.tags[this.focusedTagIndex];
+		let nodeToFocus = tagsNodes.find(node => node.textContent == tagToFocus.name);
+		if (nodeToFocus) {
+			nodeToFocus.focus();
+		}
+	}
+
+	waitForSectionRender() {
+		return new Promise((resolve, _) => {
+			this.resolveTagsRenderedPromise = resolve;
+		});
+	}
+
+	handleSectionRendered = ({ indices }) => {
+		let tagsList = document.querySelector('.tag-selector-list');
+		// <Collection> sets role="grid" which is not semantically correct
+		tagsList.setAttribute("role", "group");
+
+		if (this.focusedTagIndex === null) return;
+		// If the focused tag does not changed, the scrollToCell won't change
+		// either, so the <Collection> won't scroll to the desired tag if we don't reset it.
+		// E.g. second arrowLeft keypress when first tag is focused won't scroll to it.
+		if (this.lastFocusedTagIndex === this.focusedTagIndex) {
+			this.setState({ scrollToCell: null });
+		}
+		// Check if the tag that is supposed to be focused is within the rendered tags range.
+		// If it is, make sure it is focused. If it is not - focus the tags list.
+		if (indices.includes(this.focusedTagIndex)) {
+			this.refocusTag();
+			if (this.resolveTagsRenderedPromise) {
+				this.resolveTagsRenderedPromise();
+			}
+		}
+		else {
+			tagsList.focus();
+		}
+	};
+
+	handleBlur = (event) => {
+		// If the focus leaves the tags list, clear the last focused tag index
+		let tagsList = document.querySelector('.tag-selector-list');
+		if (!tagsList.contains(event.relatedTarget)) {
+			this.focusedTagIndex = null;
+			this.lastFocusedTagIndex = null;
+		}
+	};
+
+	async handleKeyDown(e) {
+		if (!["ArrowRight", "ArrowLeft"].includes(e.key)) return;
+		// If the windowing kicks in, the node of the initially-focused tag may not
+		// exist, so first we may need to scroll to it.
+		if (!document.activeElement.classList.contains("tag-selector-item")) {
+			this.setState({ scrollToCell: this.focusedTagIndex });
+			// Even after the <Collection> re-renders, the new tag nodes may not be rendered yet.
+			// So we have to wait for handleSectionRendered to run before proceeding.
+			await this.waitForSectionRender();
+		}
+		// Sanity check to make sure that now a tag node is focused
+		if (!document.activeElement.classList.contains("tag-selector-item")) return;
+		// Handle arrow navigation
+		let nextTag = (node) => {
+			if (e.key == "ArrowRight") return node.nextElementSibling;
+			return node.previousElementSibling;
+		};
+		let nextOne = nextTag(document.activeElement);
+		// Skip disabled tags
+		while (nextOne && nextOne.classList.contains("disabled")) {
+			nextOne = nextTag(nextOne);
+		}
+		if (nextOne) {
+			nextOne.focus();
+		}
+	}
 	
 	render() {
 		Zotero.debug("Rendering tag list");
@@ -252,12 +339,14 @@ class TagList extends React.PureComponent {
 					width={this.props.width}
 					height={this.props.height - filterBarHeight}
 					aria-label={document.querySelector("#zotero-tag-selector").getAttribute("label") || ""}
+					onSectionRendered={this.handleSectionRendered}
+					scrollToCell={Number.isInteger(this.state.scrollToCell) ? this.state.scrollToCell : undefined}
 				/>
 			);
 		}
 		
 		return (
-			<div className="tag-selector-list-container">
+			<div className="tag-selector-list-container" onBlur={this.handleBlur} onKeyDown={this.handleKeyDown.bind(this)}>
 				{tagList}
 			</div>
 		);

chrome/content/zotero/containers/tagSelectorContainer.jsx

@@ -580,34 +580,6 @@ Zotero.TagSelector = class TagSelectorContainer extends React.PureComponent {
 		}
 	}
 
-	handleKeyDown = (e) => {
-		if (["ArrowRight", "ArrowLeft"].includes(e.key)) {
-			let nextTag = (node) => {
-				if (e.key == "ArrowRight") return node.nextElementSibling;
-				return node.previousElementSibling;
-			};
-			let nextOne = nextTag(e.target);
-			// Skip disabled tags
-			while (nextOne && nextOne.classList.contains("disabled")) {
-				nextOne = nextTag(nextOne);
-			}
-			if (nextOne) {
-				nextOne.focus();
-			}
-		}
-		else if (e.key == "Tab" && !e.shiftKey) {
-			this.focusTextbox();
-			e.preventDefault();
-		}
-		else if (e.key == "Tab" && e.shiftKey) {
-			document.querySelector('.tag-selector-list').focus();
-			e.preventDefault();
-		}
-		if ([" ", "Enter"].includes(e.key)) {
-			e.target.click();
-		}
-	}
-
 	handleSearch = (searchString) => {
 		this.setState({searchString});
 	}

chrome/content/zotero/zoteroPane.js

@@ -419,9 +419,12 @@ var ZoteroPane = new function()
 						if (tagContainer.getAttribute('collapsed') == "true") {
 							return document.getElementById('zotero-tb-add');
 						}
-						// If tag selector is collapsed, go to "New item" button, otherwise
-						// default to focusing on tag selector
-						return tagContainer.querySelector(".tag-selector-list");
+						// If tag selector is collapsed, go to "New item" button, otherwise focus tag selector
+						let firstNonDisabledTag = tagSelector.querySelector('.tag-selector-item:not(.disabled)');
+						if (firstNonDisabledTag) {
+							return firstNonDisabledTag;
+						}
+						return tagSelector.querySelector(".search-input");
 					},
 					Escape: clearCollectionSearch
 				}
@@ -452,20 +455,14 @@ var ZoteroPane = new function()
 				},
 				'tag-selector-item': {
 					Tab: () => tagSelector.querySelector(".search-input"),
-					ShiftTab: () => tagSelector.querySelector(".tag-selector-list"),
+					ShiftTab: () => document.getElementById("collection-tree"),
 				},
 				'tag-selector-actions': {
 					Tab: () => document.getElementById('zotero-tb-add'),
 					ShiftTab: () => tagSelector.querySelector(".search-input")
 				},
 				'tag-selector-list': {
-					Tab: () => {
-						let firstNonDisabledTag = tagSelector.querySelector('.tag-selector-item:not(.disabled)');
-						if (firstNonDisabledTag) {
-							return firstNonDisabledTag;
-						}
-						return tagSelector.querySelector(".search-input");
-					},
+					Tab: () => tagSelector.querySelector(".search-input"),
 					ShiftTab: () => document.getElementById("collection-tree"),
 				}
 			};

test/tests/zoteroPaneTest.js

@@ -1534,7 +1534,6 @@ describe("ZoteroPane", function() {
 			"tag-selector-actions",
 			"search-input",
 			"tag-selector-item",
-			"tag-selector-list",
 			"collection-tree",
 			"zotero-collections-search",
 			"zotero-tb-collection-add",