From 1cb3db4aaa7508012d2e85a8c864fe8737397c82 Mon Sep 17 00:00:00 2001
From: Matthew Tylee Atkinson <matatk@gmail.com>
Date: Fri, 7 Dec 2018 10:31:29 +0000
Subject: [PATCH] Tidy up getLandmarks() to make the structure neater

---
 src/code/landmarksFinder.js | 83 ++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 43 deletions(-)

diff --git a/src/code/landmarksFinder.js b/src/code/landmarksFinder.js
index 279d88a6..84b7c426 100644
--- a/src/code/landmarksFinder.js
+++ b/src/code/landmarksFinder.js
@@ -88,13 +88,11 @@ export default function LandmarksFinder(win, doc) {
 	// Keeping track of landmark navigation
 	//
 
-	let currentlySelectedIndex
-
-	// If we find a <main> or role="main" element...
-	let mainElementIndex
+	let currentlySelectedIndex  // the landmark currently having focus/border
+	let mainElementIndex        // if we find a <main> or role="main" element
 
 	// Keep a reference to the currently-selected element in case the page
-	// changes and the landmarks are updated.
+	// changes and the landmark is still there, but has moved within the list.
 	let currentlySelectedElement
 
 	function updateSelectedIndexAndReturnElementInfo(index) {
@@ -116,53 +114,52 @@ export default function LandmarksFinder(win, doc) {
 
 	// Recursive function for building list of landmarks from a root element
 	function getLandmarks(element, depth, parentLandmark) {
-		for (const elementChild of element.children) {
-			if (isVisuallyHidden(elementChild)) continue
+		if (isVisuallyHidden(element)) return
 
-			// Support HTML5 elements' native roles
-			let role = getRoleFromTagNameAndContainment(elementChild)
+		// Support HTML5 elements' native roles
+		let role = getRoleFromTagNameAndContainment(element)
 
-			// Elements with explicitly-set rolees
-			if (elementChild.getAttribute) {
-				const tempRole = elementChild.getAttribute('role')
-				if (tempRole) {
-					role = tempRole
-				}
+		// Elements with explicitly-set rolees
+		if (element.getAttribute) {
+			const tempRole = element.getAttribute('role')
+			if (tempRole) {
+				role = tempRole
 			}
+		}
 
-			// The element may or may not have a label
-			const label = getARIAProvidedLabel(elementChild)
+		// The element may or may not have a label
+		const label = getARIAProvidedLabel(element)
 
-			// Add the element if it should be considered a landmark
-			if (role && isLandmark(role, label)) {
-				if (parentLandmark && parentLandmark.contains(elementChild)) {
-					depth = depth + 1
-				}
+		// Add the element if it should be considered a landmark
+		if (role && isLandmark(role, label)) {
+			if (parentLandmark && parentLandmark.contains(element)) {
+				depth = depth + 1
+			}
 
-				landmarks.push({
-					'depth': depth,
-					'role': role,
-					'label': label,
-					'element': elementChild,
-					'selector': createSelector(elementChild)
-				})
-
-				// Was this element selected before we were called (i.e.
-				// before the page was dynamically updated)?
-				if (currentlySelectedElement === elementChild) {
-					currentlySelectedIndex = landmarks.length - 1
-				}
+			landmarks.push({
+				'depth': depth,
+				'role': role,
+				'label': label,
+				'element': element,
+				'selector': createSelector(element)
+			})
 
-				// If this is the first main landmark (there should only
-				// be one), store a reference to it.
-				if (mainElementIndex < 0 && role === 'main') {
-					mainElementIndex = landmarks.length - 1
-				}
+			// Was this element selected before we were called (i.e.
+			// before the page was dynamically updated)?
+			if (currentlySelectedElement === element) {
+				currentlySelectedIndex = landmarks.length - 1
+			}
 
-				parentLandmark = elementChild
+			// If this is the first main landmark (there should only
+			// be one), store a reference to it.
+			if (mainElementIndex < 0 && role === 'main') {
+				mainElementIndex = landmarks.length - 1
 			}
 
-			// Recursively traverse the tree structure of the child node
+			parentLandmark = element
+		}
+
+		for (const elementChild of element.children) {
 			getLandmarks(elementChild, depth, parentLandmark)
 		}
 	}
@@ -192,7 +189,7 @@ export default function LandmarksFinder(win, doc) {
 			return label !== null
 		}
 
-		// Is the role (which may have been explicitly set) a valid landmark type?
+		// Is the role (which may've been explicitly set) a valid landmark type?
 		return regionTypes.includes(role)
 	}