feat(toggle): Show all landmarks keyboard shortcut (#245)

Naughty! This really should’ve been split up into smaller PRs, though I think I am starting to get into this new commit style :-). Anyway, this does a couple of things:

* Adds help page support for indicating to the user when a keyboard shortcut is not set (in support of #165).
* Implements a keyboard shortcut that toggles the display of all landmarks (in support of #120).

In support of this, it required splitting the ElementFocuser into an ElementFocuser and a BorderDrawer. A lot of API naming cleanup and code tidying was done too.

A further PR will add UI to the pop-up that mouse users can use to access this feature.

src/assemble/manifest.common.json

@@ -28,6 +28,9 @@
   ],
 
   "commands": {
+    "toggle-all-landmarks": {
+      "description": "__MSG_toggleAllShortcutDescription__"
+    },
     "main-landmark": {
       "suggested_key": {
         "default": "Alt+Shift+M"

src/assemble/manifest.firefox.json

@@ -34,5 +34,13 @@
     }
   },
 
-  "devtools_page": "devtools.html"
+  "devtools_page": "devtools.html",
+
+  "commands": {
+    "toggle-all-landmarks": {
+      "suggested_key": {
+        "default": "Alt+Shift+S"
+      }
+    }
+  }
 }

src/assemble/messages.common.json

@@ -26,6 +26,10 @@
     "message": "Move to and highlight the main landmark"
   },
 
+  "toggleAllShortcutDescription": {
+    "message": "Toggle the display of all landmarks on the page"
+  },
+
   "noMainLandmarkFound": {
     "message": "No main landmark found"
   },
@@ -92,7 +96,7 @@
   },
 
   "prefsResetAll": {
-    "message": "Reset everything to defaults"
+    "message": "Reset settings to defaults"
   },
 
 

src/code/_background.js

@@ -5,7 +5,7 @@ import { defaultInterfaceSettings } from './defaults'
 import disconnectingPortErrorCheck from './disconnectingPortErrorCheck'
 import Logger from './logger'
 
-const logger = new Logger()
+const logger = new Logger(window)
 
 const contentConnections = {}
 const devtoolsConnections = {}
@@ -296,6 +296,7 @@ browser.commands.onCommand.addListener(function(command) {
 		case 'next-landmark':
 		case 'prev-landmark':
 		case 'main-landmark':
+		case 'toggle-all-landmarks':
 			sendToActiveContentScriptIfExists({ name: command })
 			break
 		default:

src/code/_content.js

@@ -3,11 +3,15 @@ import LandmarksFinder from './landmarksFinder'
 import ElementFocuser from './elementFocuser'
 import PauseHandler from './pauseHandler'
 import Logger from './logger'
+import BorderDrawer from './borderDrawer'
+import ContrastChecker from './contrastChecker'
 
-const logger = new Logger()
-const lf = new LandmarksFinder(window, document)
-const ef = new ElementFocuser(document)
-const ph = new PauseHandler(logger)
+const logger = new Logger(window)
+const landmarksFinder = new LandmarksFinder(window, document)
+const contrastChecker = new ContrastChecker()
+const borderDrawer = new BorderDrawer(window, document, contrastChecker)
+const elementFocuser = new ElementFocuser(document, borderDrawer)
+const pauseHandler = new PauseHandler(logger)
 
 const outOfDateTime = 2000
 let observer = null
@@ -23,36 +27,52 @@ function messageHandler(message, sendingPort) {
 		case 'get-landmarks':
 			// A GUI is requesting the list of landmarks on the page
 			handleOutdatedResults()
-			sendingPort.postMessage({ name: 'landmarks', data: lf.filter() })
+			sendingPort.postMessage({
+				name: 'landmarks',
+				data: landmarksFinder.allDepthsRolesLabelsSelectors()
+			})
 			break
 		case 'focus-landmark':
 			// Triggered by clicking on an item in a GUI, or indirectly via one
 			// of the keyboard shortcuts (if landmarks are present)
 			handleOutdatedResults()
-			checkFocusElement(
-				() => lf.getLandmarkElementRoleLabel(message.index))
+			checkFocusElement(() =>
+				landmarksFinder.getLandmarkElementRoleLabel(message.index))
 			break
 		case 'next-landmark':
 			// Triggered by keyboard shortcut
 			handleOutdatedResults()
-			checkFocusElement(lf.getNextLandmarkElementRoleLabel)
+			checkFocusElement(landmarksFinder.getNextLandmarkElementRoleLabel)
 			break
 		case 'prev-landmark':
 			// Triggered by keyboard shortcut
 			handleOutdatedResults()
-			checkFocusElement(lf.getPreviousLandmarkElementRoleLabel)
+			checkFocusElement(
+				landmarksFinder.getPreviousLandmarkElementRoleLabel)
 			break
 		case 'main-landmark': {
 			// Triggered by keyboard shortcut
 			handleOutdatedResults()
-			const mainElementInfo = lf.getMainElementRoleLabel()
+			const mainElementInfo = landmarksFinder.getMainElementRoleLabel()
 			if (mainElementInfo) {
-				ef.focusElement(mainElementInfo)
+				elementFocuser.focusElement(mainElementInfo)
 			} else {
 				alert(browser.i18n.getMessage('noMainLandmarkFound') + '.')
 			}
 			break
 		}
+		case 'toggle-all-landmarks':
+			// Triggered by keyboard shortcut
+			handleOutdatedResults()
+			if (elementFocuser.isManagingBorders()) {
+				elementFocuser.manageBorders(false)
+				borderDrawer.addBorderToElements(
+					landmarksFinder.allElementsRolesLabels())
+			} else {
+				borderDrawer.removeAllBorders()
+				elementFocuser.manageBorders(true)
+			}
+			break
 		case 'trigger-refresh':
 			// On sites that use single-page style techniques to transition
 			// (such as YouTube and GitHub) we monitor in the background script
@@ -61,7 +81,8 @@ function messageHandler(message, sendingPort) {
 			// this happens, we should treat it as a new page, and fetch
 			// landmarks again when asked.
 			logger.log('Landmarks: refresh triggered')
-			ef.removeBorderOnCurrentlySelectedElement()
+			elementFocuser.clear()
+			borderDrawer.removeAllBorders()
 			findLandmarksAndUpdateBackgroundScript()
 			break
 		default:
@@ -70,19 +91,19 @@ function messageHandler(message, sendingPort) {
 }
 
 function handleOutdatedResults() {
-	if (ph.getPauseTime() > outOfDateTime) {
-		logger.log(`Landmarks may be out of date (pause: ${ph.getPauseTime()}); scanning now...`)
+	if (pauseHandler.getPauseTime() > outOfDateTime) {
+		logger.log(`Landmarks may be out of date (pause: ${pauseHandler.getPauseTime()}); scanning now...`)
 		findLandmarksAndUpdateBackgroundScript()
 	}
 }
 
 function checkFocusElement(callbackReturningElementInfo) {
-	if (lf.getNumberOfLandmarks() === 0) {
+	if (landmarksFinder.getNumberOfLandmarks() === 0) {
 		alert(browser.i18n.getMessage('noLandmarksFound') + '.')
 		return
 	}
 
-	ef.focusElement(callbackReturningElementInfo())
+	elementFocuser.focusElement(callbackReturningElementInfo())
 }
 
 
@@ -92,9 +113,17 @@ function checkFocusElement(callbackReturningElementInfo) {
 
 function findLandmarksAndUpdateBackgroundScript() {
 	logger.timeStamp('findLandmarksAndUpdateBackgroundScript()')
-	lf.find()
-	port.postMessage({ name: 'landmarks', data: lf.filter() })
-	ef.checkFocusedElement()
+	landmarksFinder.find()
+	port.postMessage({
+		name: 'landmarks',
+		data: landmarksFinder.allDepthsRolesLabelsSelectors()
+	})
+	elementFocuser.refreshFocusedElement()
+	borderDrawer.refreshBorders()
+	if (!elementFocuser.isManagingBorders()) {
+		borderDrawer.addBorderToElements(
+			landmarksFinder.allElementsRolesLabels())
+	}
 }
 
 
@@ -140,8 +169,9 @@ function setUpMutationObserver() {
 	observer = new MutationObserver((mutations) => {
 		// Guard against being innundated by mutation events
 		// (which happens in e.g. Google Docs)
-		ph.run(
-			ef.didJustMakeChanges,  // ignore mutations if Landmarks caused them
+		pauseHandler.run(
+			// Ignore mutations if Landmarks caused them
+			borderDrawer.hasMadeDOMChanges,
 			function() {
 				if (shouldRefreshLandmarkss(mutations)) {
 					logger.log('Scan due to mutation')

src/code/_gui.js

@@ -229,7 +229,8 @@ if (INTERFACE === 'sidebar') {
 
 	browser.storage.onChanged.addListener(function(changes) {
 		// What if the sidebar is open and the user changes their preference?
-		if (changes.hasOwnProperty('interface')) {
+		if (changes.hasOwnProperty('interface')
+			&& changes.interface.newValue !== changes.interface.oldValue) {
 			switch (changes.interface.newValue) {
 				case 'sidebar': removeNote()
 					break

src/code/_help.js

@@ -1,8 +1,8 @@
 import disconnectingPortErrorCheck from './disconnectingPortErrorCheck'
 
 let port
+let shortcutNotSet = false
 
-// FIXME global
 const shortcutTableRows = [
 	{
 		element: 'tr',
@@ -13,37 +13,50 @@ const shortcutTableRows = [
 	}
 ]
 
-// FIXME global
 const keyboardShortcutsLink = {
-	element: 'p', contains: [{
-		element: 'a',
-		class: 'config',
-		href: '#',
-		content: 'Add or change shortcuts',
-		listen: {
-			event: 'click',
-			handler: () => port.postMessage({
-				name: 'splash-open-configure-shortcuts'
-			})
+	element: 'a',
+	class: 'configAction',
+	tabindex: 0,
+	content: 'Add or change shortcuts',
+	listen: [{
+		event: 'click',
+		handler: () => port.postMessage({
+			name: 'splash-open-configure-shortcuts'
+		})
+	}, {
+		event: 'keydown',
+		handler: (event) => {
+			if (event.key === 'Enter') {
+				port.postMessage({
+					name: 'splash-open-configure-shortcuts'
+				})
+			}
 		}
 	}]
 }
 
-// FIXME global
 const settingsLink = {
 	element: 'a',
-	class: 'config',
-	href: '#',
+	class: 'configAction',
+	tabindex: 0,
 	content: 'Change preferences (opens in new tab)',
-	listen: {
+	listen: [{
 		event: 'click',
-		handler: (event) => {
+		handler: () => {
 			port.postMessage({
 				name: 'splash-open-settings'
 			})
-			event.preventDefault()  // until it opens in the same tab
 		}
-	}
+	}, {
+		event: 'keydown',
+		handler: (event) => {
+			if (event.key === 'Enter') {
+				port.postMessage({
+					name: 'splash-open-configure-shortcuts'
+				})
+			}
+		}
+	}]
 }
 
 function makeHTML(structure, root) {
@@ -58,8 +71,8 @@ function makeHTML(structure, root) {
 			case 'class':
 				newElement.classList.add(structure[key])
 				break
-			case 'href':
-				newElement.href = structure[key]
+			case 'tabindex':
+				newElement.setAttribute('tabindex', String(structure[key]))
 				break
 			case 'text':
 				root.appendChild(document.createTextNode(structure[key]))
@@ -68,8 +81,10 @@ function makeHTML(structure, root) {
 				newElement.appendChild(document.createTextNode(structure[key]))
 				break
 			case 'listen':
-				newElement.addEventListener(
-					structure[key].event, structure[key].handler)
+				for (const eventHandler of structure[key]) {
+					newElement.addEventListener(
+						eventHandler.event, eventHandler.handler)
+				}
 				break
 			case 'contains':
 				for (const contained of structure[key]) {
@@ -114,9 +129,10 @@ function addCommandRowAndReportIfMissing(command) {
 			}
 		}
 	} else {
-		shortcutCellElement = { element: 'td', class: 'error', contains: [
+		shortcutCellElement = { element: 'td', class: 'errorItem', contains: [
 			{ text: 'Not set up' }
 		]}
+		shortcutNotSet = true
 	}
 
 	shortcutTableRows.push({
@@ -169,6 +185,20 @@ function messageHandler(message) {  // also sendingPort
 
 	makeHTML({ element: 'table', contains: shortcutTableRows },
 		document.getElementById('keyboard-shortcuts-table'))
+
+	if (shortcutNotSet) {
+		document.querySelector('#section-keyboard-navigation summary')
+			.classList.add('errorItem')
+
+		document.querySelector('[data-link="shortcuts"] a')
+			.classList.add('errorAction')
+
+		for (const warning of document.querySelectorAll('[data-warning]')) {
+			warning.style.display = 'block'
+		}
+
+		document.getElementById('symbol').style.display = 'inline'
+	}
 }
 
 function makeConfigLinks(type, template) {
@@ -224,8 +254,8 @@ function main() {
 	port.onMessage.addListener(messageHandler)
 	port.postMessage({ name: 'get-commands' })
 
-	if (BROWSER === 'firefox' || BROWSER === 'opera') {
-		document.getElementById('section-sidebar').open = true
+	if (BROWSER !== 'firefox' && BROWSER !== 'opera') {
+		document.getElementById('section-sidebar').open = false
 	}
 
 	makeSettingsAndShortcutsLinks()

src/code/_options.js

@@ -66,7 +66,7 @@ function setUpOptionHandlers() {
 		document.getElementById('reset-messages').onclick = resetMessages
 	}
 
-	document.getElementById('reset-all').onclick = resetAll
+	document.getElementById('reset-to-defaults').onclick = resetToDefaults
 }
 
 function interfaceExplainer() {
@@ -113,9 +113,12 @@ function dismissalStateChanged(thingChanged) {
 	return dismissalStates.hasOwnProperty(thingChanged)
 }
 
-function resetAll() {
-	browser.storage.sync.clear()
-	window.location.reload()
+function resetToDefaults() {
+	browser.storage.sync.set(defaultSettings, function() {
+		window.location.reload()
+	})
+	// Note: Can't use use .clear() as that removes everything, which would
+	//       cause problems for currently-visible borders.
 }
 
 function main() {