fix: generate suggested locators upon selecting element instead of up…

…on retrieving source (appium#1389)

* add slight caching for retrieved locator map

* remove unused third param for getOptimalXPath calls

* add comment to point to next actions

* WIP moving xpath calc to getLocators

* chore: remove unnecessary filter call

* chore: move findElementByPath to utils

* chore: remove unused state vars

* chore: revert to object for getLocators response

* chore: fix getLocators parameter

* chore: move getLocators to utils file

* start work on the complex locator detection

* chore: rename various source variables for clarity

* chore: move DOM conversion from areAttrAndValueUnique

* chore: add function to retrieve DOM node using path

* chore: update docstrings

* chore: save DOMParser in a const

* fix: calculate optimal xpath only on demand

* chore: do not pass local consts as parameters

* fix: enable predicate string for XCUIElementTypeApplication

* chore: adjust comments for getOptimalClassChain

* fix: calculate optimal class chain/predicate only on demand

* chore: define NATIVE_APP only once

* chore: update more var names

* chore: remove dedicated isIOS variable

* fix: skip accessibility id strategy in webviews

* chore: simplify childNodesOf

* chore: lint & format

* test: update unit tests

* test: add tests for accessibility id and class name

* test: add tests for path lookup methods

* test: add tests for class chain/predicate + format

* chore: split utils file

* chore: address review comments

* chore: update typedefs

* chore: update typedef again

app/renderer/actions/Inspector.js

@@ -3,10 +3,16 @@ import {v4 as UUID} from 'uuid';
 
 import i18n from '../../configs/i18next.config.renderer';
 import {SAVED_FRAMEWORK, SET_SAVED_GESTURES, getSetting, setSetting} from '../../shared/settings';
-import {APP_MODE, getLocators} from '../components/Inspector/shared';
-import AppiumClient, {NATIVE_APP} from '../lib/appium-client';
+import {APP_MODE, NATIVE_APP} from '../components/Inspector/shared';
+import AppiumClient from '../lib/appium-client';
 import frameworks from '../lib/client-frameworks';
-import {xmlToJSON} from '../util';
+import {getOptimalXPath, getSuggestedLocators} from '../utils/locator-generation';
+import {
+  domParser,
+  findDOMNodeByPath,
+  findJSONElementByPath,
+  xmlToJSON,
+} from '../utils/source-parsing';
 import {showError} from './Session';
 
 export const SET_SESSION_DETAILS = 'SET_SESSION_DETAILS';
@@ -19,6 +25,7 @@ export const SET_INTERACTIONS_NOT_AVAILABLE = 'SET_INTERACTIONS_NOT_AVAILABLE';
 export const METHOD_CALL_REQUESTED = 'METHOD_CALL_REQUESTED';
 export const METHOD_CALL_DONE = 'METHOD_CALL_DONE';
 export const SET_EXPANDED_PATHS = 'SET_EXPANDED_PATHS';
+export const SET_OPTIMAL_LOCATORS = 'SET_OPTIMAL_LOCATORS';
 export const SELECT_HOVERED_ELEMENT = 'SELECT_HOVERED_ELEMENT';
 export const UNSELECT_HOVERED_ELEMENT = 'UNSELECT_HOVERED_ELEMENT';
 
@@ -115,12 +122,12 @@ const findElement = _.debounce(async function (strategyMap, dispatch, getState,
       strategy,
       selector,
     });
-    let {elementId, variableName, variableType} = await action(dispatch, getState);
+    let {elementId} = await action(dispatch, getState);
 
-    // Set the elementId, variableName and variableType for the selected element
+    // Set the elementId for the selected element
     // (check first that the selectedElementPath didn't change, to avoid race conditions)
     if (elementId && getState().inspector.selectedElementPath === path) {
-      return dispatch({type: SET_SELECTED_ELEMENT_ID, elementId, variableName, variableType});
+      return dispatch({type: SET_SELECTED_ELEMENT_ID, elementId});
     }
   }
 
@@ -129,9 +136,11 @@ const findElement = _.debounce(async function (strategyMap, dispatch, getState,
 
 export function selectElement(path) {
   return async (dispatch, getState) => {
+    const {sourceJSON, sourceXML, expandedPaths, currentContext} = getState().inspector;
+    const isNative = currentContext === NATIVE_APP;
     // Set the selected element in the source tree
-    dispatch({type: SELECT_ELEMENT, path});
-    const {selectedElement, sourceXML, expandedPaths} = getState().inspector;
+    const selectedElement = findJSONElementByPath(path, sourceJSON);
+    dispatch({type: SELECT_ELEMENT, selectedElement});
 
     // Expand all of this element's ancestors so that it's visible in the source tree
     // Make a copy of the array to avoid state mutation
@@ -146,9 +155,9 @@ export function selectElement(path) {
     }
     dispatch({type: SET_EXPANDED_PATHS, paths: copiedExpandedPaths});
 
-    // Find the optimal selection strategy. If none found, fall back to XPath.
-    const strategyMap = _.toPairs(getLocators(selectedElement.attributes, sourceXML));
-    strategyMap.push(['xpath', selectedElement.xpath]);
+    // Calculate the recommended locator strategies
+    const strategyMap = getSuggestedLocators(selectedElement, sourceXML, isNative);
+    dispatch({type: SET_OPTIMAL_LOCATORS, strategyMap});
 
     // Debounce find element so that if another element is selected shortly after, cancel the previous search
     await findElement(strategyMap, dispatch, getState, path);
@@ -186,8 +195,10 @@ export function unselectHoveredCentroid() {
 }
 
 export function selectHoveredElement(path) {
-  return (dispatch) => {
-    dispatch({type: SELECT_HOVERED_ELEMENT, path});
+  return (dispatch, getState) => {
+    const {sourceJSON} = getState().inspector;
+    const hoveredElement = findJSONElementByPath(path, sourceJSON);
+    dispatch({type: SELECT_HOVERED_ELEMENT, hoveredElement});
   };
 }
 
@@ -249,7 +260,7 @@ export function applyClientMethod(params) {
           type: SET_SOURCE_AND_SCREENSHOT,
           contexts,
           currentContext,
-          source: source && xmlToJSON(source),
+          sourceJSON: xmlToJSON(source),
           sourceXML: source,
           screenshot,
           windowSize,
@@ -489,30 +500,30 @@ export function setLocatorTestElement(elementId) {
  * Given an element ID found through search, and its bounds,
  * attempt to find and select this element in the source tree
  */
-export function selectLocatedElement(source, bounds, id) {
+export function selectLocatedElement(sourceJSON, sourceXML, bounds, id) {
   const UPPER_FILTER_LIMIT = 10;
 
   // Parse the source tree and find all nodes whose bounds match the expected bounds
-  // Return the path + xpath of each node
+  // Return the path of each node
   function findPathsMatchingBounds() {
-    if (!bounds || !source.children || !source.children[0].attributes) {
+    if (!bounds || !sourceJSON.children || !sourceJSON.children[0].attributes) {
       return null;
     }
-    if (source.children[0].attributes.bounds) {
+    if (sourceJSON.children[0].attributes.bounds) {
       const [endX, endY] = [
         bounds.location.x + bounds.size.width,
         bounds.location.y + bounds.size.height,
       ];
       const coords = `[${bounds.location.x},${bounds.location.y}][${endX},${endY}]`;
-      return findPathsFromCoords(source.children, coords);
-    } else if (source.children[0].attributes.x) {
+      return findPathsFromCoords(sourceJSON.children, coords);
+    } else if (sourceJSON.children[0].attributes.x) {
       const combinedBounds = {
         x: String(bounds.location.x),
         y: String(bounds.location.y),
         height: String(bounds.size.height),
         width: String(bounds.size.width),
       };
-      return findPathsFromBounds(source.children, combinedBounds);
+      return findPathsFromBounds(sourceJSON.children, combinedBounds);
     }
     return null;
   }
@@ -522,7 +533,7 @@ export function selectLocatedElement(source, bounds, id) {
     let collectedPaths = [];
     for (const tree of trees) {
       if (tree.attributes.bounds === coords) {
-        collectedPaths.push([tree.path, tree.xpath]);
+        collectedPaths.push(tree.path);
       }
       if (tree.children.length) {
         collectedPaths.push(...findPathsFromCoords(tree.children, coords));
@@ -541,7 +552,7 @@ export function selectLocatedElement(source, bounds, id) {
         tree.attributes.height === bounds.height &&
         tree.attributes.width === bounds.width
       ) {
-        collectedPaths.push([tree.path, tree.xpath]);
+        collectedPaths.push(tree.path);
       }
       if (tree.children.length) {
         collectedPaths.push(...findPathsFromBounds(tree.children, bounds));
@@ -557,21 +568,24 @@ export function selectLocatedElement(source, bounds, id) {
       return null;
     }
     if (foundPaths.length === 1) {
-      return foundPaths[0][0];
+      return foundPaths[0];
     } else if (foundPaths.length !== 0 && foundPaths.length <= UPPER_FILTER_LIMIT) {
       return await findElementWithMatchingId(foundPaths, dispatch, getState);
     }
     return null;
   }
 
-  // Calls Appium findElement for each provided xpath, and returns the path
-  // of the element whose ID matches the expected ID
+  // For each provided path, get its xpath and call Appium findElement
+  // Return the path of the element whose ID matches the expected ID
   async function findElementWithMatchingId(foundPaths, dispatch, getState) {
+    const sourceDoc = domParser.parseFromString(sourceXML);
     for (const path of foundPaths) {
-      const action = callClientMethod({strategy: 'xpath', selector: path[1]});
+      const domNode = findDOMNodeByPath(path, sourceDoc);
+      const xpath = getOptimalXPath(sourceDoc, domNode);
+      const action = callClientMethod({strategy: 'xpath', selector: xpath});
       const {el} = await action(dispatch, getState);
       if (el && el.elementId === id) {
-        return path[0];
+        return path;
       }
     }
     return null;

app/renderer/actions/Session.js

@@ -27,7 +27,7 @@ import {
 import {APP_MODE} from '../components/Inspector/shared';
 import CloudProviders from '../components/Session/CloudProviders';
 import {fs, ipcRenderer, util} from '../polyfills';
-import {addVendorPrefixes} from '../util';
+import {addVendorPrefixes} from '../utils/other';
 import {quitSession, setSessionDetails} from './Inspector';
 
 export const NEW_SESSION_REQUESTED = 'NEW_SESSION_REQUESTED';

app/renderer/components/ErrorBoundary/ErrorMessage.js

@@ -3,7 +3,7 @@ import {Alert, Button, Tooltip} from 'antd';
 import React from 'react';
 
 import {shell} from '../../polyfills';
-import {withTranslation} from '../../util';
+import {withTranslation} from '../../utils/other';
 import {ALERT} from '../AntdTypes';
 import styles from './ErrorMessage.css';
 

app/renderer/components/Inspector/HighlighterRects.js

@@ -12,7 +12,7 @@ const {CENTROID, OVERLAP, EXPAND} = RENDER_CENTROID_AS;
  */
 const HighlighterRects = (props) => {
   const {
-    source,
+    sourceJSON,
     containerEl,
     searchedForElementBounds,
     scaleRatio,
@@ -26,8 +26,8 @@ const HighlighterRects = (props) => {
   let highlighterXOffset = 0;
   let screenshotEl = null;
 
-  const getElements = (source) => {
-    const elementsByOverlap = buildElementsWithProps(source, null, [], {});
+  const getElements = (sourceJSON) => {
+    const elementsByOverlap = buildElementsWithProps(sourceJSON, null, [], {});
     let elements = [];
 
     // Adjust overlapping elements
@@ -68,16 +68,16 @@ const HighlighterRects = (props) => {
   // This func creates a new object for each element and determines its properties
   // 'elements' is an array that stores all prev elements
   // 'overlaps' is an object which organzies elements by their positions
-  const buildElementsWithProps = (source, prevElement, elements, overlaps) => {
-    if (!source) {
+  const buildElementsWithProps = (sourceJSON, prevElement, elements, overlaps) => {
+    if (!sourceJSON) {
       return {};
     }
-    const {x1, y1, x2, y2} = parseCoordinates(source);
+    const {x1, y1, x2, y2} = parseCoordinates(sourceJSON);
     const xOffset = highlighterXOffset || 0;
     const centerPoint = (v1, v2) => Math.round(v1 + (v2 - v1) / 2) / scaleRatio;
     const obj = {
       type: CENTROID,
-      element: source,
+      element: sourceJSON,
       parent: prevElement,
       properties: {
         left: x1 / scaleRatio + xOffset,
@@ -88,28 +88,28 @@ const HighlighterRects = (props) => {
         centerY: centerPoint(y1, y2),
         angleX: null,
         angleY: null,
-        path: source.path,
+        path: sourceJSON.path,
         keyCode: null,
         container: false,
-        accessible: source.attributes ? source.attributes.accessible : null,
+        accessible: sourceJSON.attributes ? sourceJSON.attributes.accessible : null,
       },
     };
     const coordinates = `${obj.properties.centerX},${obj.properties.centerY}`;
     obj.properties.container = isElementContainer(obj, elements);
 
     elements.push(obj);
 
-    if (source.path) {
+    if (sourceJSON.path) {
       if (overlaps[coordinates]) {
         overlaps[coordinates].push(obj);
       } else {
         overlaps[coordinates] = [obj];
       }
     }
 
-    if (source.children) {
-      for (const childEl of source.children) {
-        buildElementsWithProps(childEl, source, elements, overlaps);
+    if (sourceJSON.children) {
+      for (const childEl of sourceJSON.children) {
+        buildElementsWithProps(childEl, sourceJSON, elements, overlaps);
       }
     }
 
@@ -167,8 +167,8 @@ const HighlighterRects = (props) => {
   };
 
   // Displays element rectangles only
-  const renderElements = (source) => {
-    for (const elem of source) {
+  const renderElements = (elements) => {
+    for (const elem of elements) {
       // only render elements with non-zero height and width
       if (!elem.properties.width || !elem.properties.height) {
         continue;
@@ -200,7 +200,7 @@ const HighlighterRects = (props) => {
   };
 
   // Array of all element objects with properties to draw rectangles and/or centroids
-  const elements = getElements(source);
+  const elements = getElements(sourceJSON);
 
   if (containerEl) {
     screenshotEl = containerEl.querySelector('img');

app/renderer/components/Inspector/LocatedElements.js

@@ -15,7 +15,8 @@ const LocatedElements = (props) => {
     isFindingLocatedElementInSource,
     searchedForElementBounds,
     selectLocatedElement,
-    source,
+    sourceJSON,
+    sourceXML,
     driver,
     t,
   } = props;
@@ -93,7 +94,12 @@ const LocatedElements = (props) => {
                     disabled={!locatorTestElement}
                     icon={<MenuUnfoldOutlined />}
                     onClick={() =>
-                      selectLocatedElement(source, searchedForElementBounds, locatorTestElement)
+                      selectLocatedElement(
+                        sourceJSON,
+                        sourceXML,
+                        searchedForElementBounds,
+                        locatorTestElement,
+                      )
                     }
                   />
                 </Tooltip>