Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: native CSS selector for Android UiAutomator #410

Merged
merged 18 commits into from
Sep 5, 2020
5 changes: 5 additions & 0 deletions lib/commands/find.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import CssConverter from '../css-converter';

let helpers = {}, extensions = {};

Expand All @@ -21,6 +22,10 @@ helpers.doFindElementOrEls = async function (params) {
params.strategy = '-android uiautomator';
params.selector = MAGIC_SCROLLABLE_BY;
}
if (params.strategy === 'css selector') {
params.strategy = '-android uiautomator';
params.selector = CssConverter.toUiAutomatorSelector(params.selector);
}
if (params.multiple) {
return await this.uiautomator2.jwproxy.command(`/elements`, 'POST', params);
} else {
Expand Down
309 changes: 309 additions & 0 deletions lib/css-converter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,309 @@
import { CssSelectorParser } from 'css-selector-parser';
import _ from 'lodash';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just do this

Suggested change
import _ from 'lodash';
import { escapeRegExp } from 'lodash';

and change the _ escapeRegExp into escapeRegExp like we normally do with imports?

import { errors } from 'appium-base-driver';

const CssConverter = {};

const parser = new CssSelectorParser();
parser.registerSelectorPseudos('has');
parser.registerNestingOperators('>', '+', '~');
parser.registerAttrEqualityMods('^', '$', '*', '~');
parser.enableSubstitutes();

const RESOURCE_ID = 'resource-id';

const BOOLEAN_ATTRS = [
'checkable', 'checked', 'clickable', 'enabled', 'focusable',
'focused', 'long-clickable', 'scrollable', 'selected',
];

const NUMERIC_ATTRS = [
'index', 'instance',
];

const STR_ATTRS = [
'description', RESOURCE_ID, 'text', 'class-name', 'package-name'
];

const ALL_ATTRS = [
...BOOLEAN_ATTRS,
...NUMERIC_ATTRS,
...STR_ATTRS,
];

const ATTRIBUTE_ALIASES = [
[RESOURCE_ID, ['id']],
['description', [
'content-description', 'content-desc',
'desc', 'accessibility-id',
]],
['index', ['nth-child']],
];

/**
* Convert hyphen separated word to snake case
*
* @param {string} str
* @returns {string} The hyphen separated word translated to snake case
*/
function toSnakeCase (str) {
if (!str) {
return '';
}
const tokens = str.split('-').map((str) => str.charAt(0).toUpperCase() + str.slice(1).toLowerCase());
mykola-mokhnach marked this conversation as resolved.
Show resolved Hide resolved
const out = tokens.join('');
return out.charAt(0).toLowerCase() + out.slice(1);
}

/**
* @typedef {Object} CssNameValueObject
* @property {?name} name The name of the CSS object
* @property {?string} value The value of the CSS object
*/

/**
* Get the boolean from a CSS object. If empty, return true. If not true/false/empty, throw exception
*
* @param {CssNameValueObject} css A CSS object that has 'name' and 'value'
* @returns {string} Either 'true' or 'false'. If value is empty, return 'true'
*/
function assertGetBool (css) {
const val = css.value?.toLowerCase() || 'true'; // an omitted boolean attribute means 'true' (e.g.: input[checked] means checked is true)
if (['true', 'false'].includes(val)) {
return val;
}
throw new Error(`'${css.name}' must be true, false or empty. Found '${css.value}'`);
}

/**
* Get the canonical form of a CSS attribute name
*
* Converts to lowercase and if an attribute name is an alias for something else, return
* what it is an alias for
*
* @param {Object} css CSS object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather prefer more specific definition of the CSS object with properties. Something like

* @typedef {Object} AcceptAlertOptions

* @returns {string} The canonical attribute name
*/
function assertGetAttrName (css) {
const attrName = css.name.toLowerCase();

// Check if it's supported and if it is, return it
if (ALL_ATTRS.includes(attrName)) {
return attrName.toLowerCase();
}

// If attrName is an alias for something else, return that
for (const [officialAttr, aliasAttrs] of ATTRIBUTE_ALIASES) {
if (aliasAttrs.includes(attrName)) {
return officialAttr;
}
}
throw new Error(`'${attrName}' is not a valid attribute. ` +
`Supported attributes are '${ALL_ATTRS.join(', ')}'`);
}

/**
* Get a regex that matches a whole word. For the ~= CSS attribute selector.
*
* @param {string} word
* @returns {string} A regex "word" matcher
*/
function getWordMatcherRegex (word) {
return `\\b(\\w*${_.escapeRegExp(word)}\\w*)\\b`;
}

/**
* Add android:id/ to beginning of string if it's not there already
*
* @param {string} str
* @returns {string} String with `android:id/` prepended (if it wasn't already)
*/
function prependAndroidId (str) {
return str.startsWith('android:id/') ? str : `android:id/${str}`;
}

/**
* @typedef {Object} CssAttr
* @property {?string} valueType Type of attribute (must be string or empty)
* @property {?string} value Value of the attribute
* @property {?string} operator The operator between value and value type (=, *=, , ^=, $=)
*/

/**
* Convert a CSS attribute into a UiSelector method call
*
* @param {CssAttr} cssAttr CSS attribute object
* @returns {string} CSS attribute parsed as UiSelector
*/
function parseAttr (cssAttr) {
if (cssAttr.valueType && cssAttr.valueType !== 'string') {
throw new Error(`'${cssAttr.name}=${cssAttr.value}' is an invalid attribute. ` +
`Only 'string' and empty attribute types are supported. Found '${cssAttr.valueType}'`);
}
const attrName = assertGetAttrName(cssAttr);
const methodName = toSnakeCase(attrName);
if (BOOLEAN_ATTRS.includes(attrName)) {
return `.${methodName}(${assertGetBool(cssAttr)})`;
}

if (!STR_ATTRS.includes(attrName)) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the docstring this method cannot return undefined

}
let value = cssAttr.value || '';
if (attrName === RESOURCE_ID) {
value = prependAndroidId(value);
}
if (value === '') {
return `.${methodName}Matches("")`;
}

switch (cssAttr.operator) {
case '=':
return `.${methodName}("${value}")`;
case '*=':
if (['description', 'text'].includes(attrName)) {
return `.${methodName}Contains("${value}")`;
}
return `.${methodName}Matches("${_.escapeRegExp(value)}")`;
case '^=':
if (['description', 'text'].includes(attrName)) {
return `.${methodName}StartsWith("${value}")`;
}
return `.${methodName}Matches("^${_.escapeRegExp(value)}")`;
case '$=':
return `.${methodName}Matches("${_.escapeRegExp(value)}$")`;
case '~=':
return `.${methodName}Matches("${getWordMatcherRegex(value)}")`;
default:
// Unreachable, but adding error in case a new CSS attribute is added.
throw new Error(`Unsupported CSS attribute operator '${cssAttr.operator}'. ` +
` '=', '*=', '^=', '$=' and '~=' are supported.`);
}
}

/**
* @typedef {Object} CssPseudo
* @property {?string} valueType The type of CSS pseudo selector (https://www.npmjs.com/package/css-selector-parser for reference)
* @property {?string} name The name of the pseudo selector
* @property {?string} value The value of the pseudo selector
*/

/**
* Convert a CSS pseudo class to a UiSelector
*
* @param {CssPseudo} cssPseudo CSS Pseudo class
* @returns {string} Pseudo selector parsed as UiSelector
*/
function parsePseudo (cssPseudo) {
if (cssPseudo.valueType && cssPseudo.valueType !== 'string') {
throw new Error(`'${cssPseudo.name}=${cssPseudo.value}'. ` +
`Unsupported css pseudo class value type: '${cssPseudo.valueType}'. Only 'string' type or empty is supported.`);
}

const pseudoName = assertGetAttrName(cssPseudo);

if (BOOLEAN_ATTRS.includes(pseudoName)) {
return `.${toSnakeCase(pseudoName)}(${assertGetBool(cssPseudo)})`;
}

if (NUMERIC_ATTRS.includes(pseudoName)) {
return `.${pseudoName}(${cssPseudo.value})`;
}
}

/**
* @typedef {Object} CssRule
* @property {?string} nestingOperator The nesting operator (aka: combinator https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors)
* @property {?string} tagName The tag name (aka: type selector https://developer.mozilla.org/en-US/docs/Web/CSS/Type_selectors)
* @property {?string[]} classNames An array of CSS class names
* @property {?CssAttr[]} attrs An array of CSS attributes
* @property {?CssPseudo[]} attrs An array of CSS pseudos
* @property {?string} id CSS identifier
* @property {?CssRule} rule A descendant of this CSS rule
*/

/**
* Convert a CSS rule to a UiSelector
* @param {CssRule} cssRule CSS rule definition
*/
function parseCssRule (cssRule) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to have a class for such transformation rather than a set of functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mykola-mokhnach I prefer the procedural approach myself, but that's a much larger debate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having a class could help to avoid passing the css object (and possibly many other shared args) to all the functions

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this could be refactored to have a main class that performs css expression validation and tokenization and then passing a tranformer class/function to it. This class/function could be responsible for the expression tranformation itself to, let say, UiSelector or class chain expression. This way we could extract the common part of the code and possibly reuse it by moving to appium-support module. So, only the driver-specific transformation method would stay in the driver source.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose this approach because of my personal preference ... I find the testing simpler and I personally lean towards avoiding the cognitive overhead of thinking about "state". That is it to say, when I'm writing and troubleshooting functions, I prefer the functions to be "pure" so that I only have to think about the inputs (the args) and the output and that I don't have to think about the state of the object under test.

I still believe there are many cases where using classes/objects is the right approach. I won't be advocating that we port Appium to Haskell any time soon. I like that Javascript affords the choice between functional programming and object-oriented :)

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is fine to use Haskell-based approach. For me it's more important to have it properly designed from architectural perspective. E.g. that we could easily separate different entities (tokenization, validation and transformation) and then implement the feature in different drivers without copying the same parts of code here and there

Copy link
Contributor Author

@dpgraham dpgraham Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this feature, my plan is to not share any code between the drivers and just copy-paste + refactor.

Normally this is terrible coding practice, but I'm making an exception because this is based on the CSS selector standard, which isn't going to be introducing breaking changes any time soon and it's very unlikely that we'll ever need to make alterations to the code that would have to be ported to all drivers. I would rather each driver do it's own independent implementation.

const { nestingOperator } = cssRule;
if (nestingOperator && nestingOperator !== ' ') {
throw new Error(`'${nestingOperator}' is not a supported combinator. ` +
`Only child combinator (>) and descendant combinator are supported.`);
}

let uiAutomatorSelector = 'new UiSelector()';
mykola-mokhnach marked this conversation as resolved.
Show resolved Hide resolved
if (cssRule.tagName && cssRule.tagName !== '*') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to do this differently, but I always have some issues reading multiple if|else-statements like this

let androidClass = [cssRule.tagName];
for (const cssClassNames of (cssRule.classNames || [])) {
androidClass.push(cssClassNames);
}
uiAutomatorSelector += `.className("${androidClass.join('.')}")`;
} else if (cssRule.classNames) {
uiAutomatorSelector += `.classNameMatches("\\.${cssRule.classNames.join('\\.')}")`;
}
if (cssRule.id) {
uiAutomatorSelector += `.resourceId("${prependAndroidId(cssRule.id)}")`;
}
if (cssRule.attrs) {
for (const attr of cssRule.attrs) {
uiAutomatorSelector += parseAttr(attr);
}
}
if (cssRule.pseudos) {
for (const pseudo of cssRule.pseudos) {
uiAutomatorSelector += parsePseudo(pseudo);
}
}
if (cssRule.rule) {
uiAutomatorSelector += `.childSelector(${parseCssRule(cssRule.rule)})`;
}
return uiAutomatorSelector;
}

/**
* @typedef {Object} CssObject
* @property {?string} type Type of CSS object. 'rule', 'ruleset' or 'selectors'
*/

/**
* Convert CSS object to UiAutomator2 selector
* @param {CssObject} css CSS object
* @returns {string} The CSS object parsed as a UiSelector
*/
function parseCssObject (css) {
switch (css.type) {
case 'rule':
return parseCssRule(css);
case 'ruleSet':
return parseCssObject(css.rule);
case 'selectors':
return css.selectors.map((selector) => parseCssObject(selector)).join('; ');

default:
// This is never reachable, but if it ever is do this.
throw new Error(`UiAutomator does not support '${css.type}' css. Only supports 'rule', 'ruleSet', 'selectors' `);
}
}

/**
* Convert a CSS selector to a UiAutomator2 selector
* @param {string} cssSelector CSS Selector
* @returns {string} The CSS selector converted to a UiSelector
*/
CssConverter.toUiAutomatorSelector = function toUiAutomatorSelector (cssSelector) {
let cssObj;
try {
cssObj = parser.parse(cssSelector);
} catch (e) {
throw new errors.InvalidSelectorError(`Invalid CSS selector '${cssSelector}'. Reason: '${e}'`);
}
try {
return parseCssObject(cssObj);
} catch (e) {
throw new errors.InvalidSelectorError(`Unsupported CSS selector '${cssSelector}'. Reason: '${e}'`);
}
};

export default CssConverter;
1 change: 1 addition & 0 deletions lib/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ class AndroidUiautomator2Driver extends BaseDriver {
'id',
'class name',
'accessibility id',
'css selector',
'-android uiautomator'
];
this.desiredCapConstraints = desiredCapConstraints;
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
"asyncbox": "^2.3.1",
"axios": "^0.19.2",
"bluebird": "^3.5.1",
"css-selector-parser": "^1.4.1",
"lodash": "^4.17.4",
"portscanner": "2.2.0",
"source-map-support": "^0.5.5",
Expand Down
Loading