Set cell accessibility identifiers to section+cell keys by Caiopia · Pull Request #109 · Shopify/FunctionalTableData

Caiopia · 2018-09-14T22:14:49Z

What

Adds an accessibility identifier to each cell so we can access them more easily for UI Testing. Since we already add unique keys to each section configuration and cell configuration, we should be able to reuse them as the accessibility identifiers. This way we don't have to input any extra identifiers.

If you have two components inside a cell you want to access separately you can set those components as accessibility elements to override the cell's accessibility identifier.

Since accessibility identifiers are only used for UITesting, this shouldn't affect any integrations with Voice Over.

Example

Similar to the test, if we had a section:

TableSection(key: "sectionKey", rows: [SomeCell(key: "cellKey")])

The accessibilityIdentifier of the cell inside that section will be "sectionKeycellKey"

raulriera · 2018-09-15T00:29:04Z

 		let row = indexPath.row
 		let cellConfig = sectionData[row]
 		let cell = cellConfig.dequeueCell(from: tableView, at: indexPath)
+		cell.accessibilityIdentifier = sectionData.sectionKeyPathForRow(row)


I believe this is the job of the states. Cells can be very complex with a lot of views, and we will need to identify each of them, like we are currently doing in some of our states ButtonState for example.

Meaning that the HostCell's state should hold the identifier? And for the views inside, couldn't we just set their identifiers to override the cell as the superview like Caio said?

Yes, that is what I mean. I have no strong opinions on it, but I dont think the key makes a great identifier. Not every view needs an unique identifier, I would argue all orders cell for example should have the same identifier. But I dont know the scope of this change

That information should still belong in ButtonState along with setting accessibilityLabels since we might want to access that button specifically in a UI test even if it's contained inside a HostCell's view, but having cell identifiers in addition would be super helpful. With this, we can add support for cell selection in UI tests for free and differentiate from other accessible items that are repeated in similar cells more easily.

I have no strong opinions on it, but I dont think the key makes a great identifier. Not every view needs an unique identifier, I would argue all orders cell for example should have the same identifier.

Having unique identifiers for things like individual orders has been pretty useful so far. If you want to access the 4th order, you can use the key you already wrote instead of getting the cell at index 3. I can write up an example...

But I dont know the scope of this change

accessibilityIdentifier is only used for UI testing, so this is an additive change for those doing UITesting. If you want your state to set your hosted view's accessibility information, you can still do that the same way as previously, but the cells that contain it will have identifiers instead of being anonymous.

Ok bear with me...

Let's say we have a table view with a bunch sections of products on a screen with a product name and an info button and we want to tap on the info button of the 🍌 product. In our state, we could set the button accessibility identifier to be something like "productView.button.info-\(product.name)" so it can be differentiated from the others. Cool we can find and tap the button with something like:

app.tables["tableView.productList"].buttons["productView.button.info-🍌"].tap()

What if we want to tap on the cell in the test too? Or what if this product name is listed twice in the list but in different sections? UIView subclasses aren't accessibility containers by default so our hosted product name view won't be hittable/tappable in UI tests unless we make it an accessibility container or element. If we do that, we have to implement Voice over support too. After that, our state can create and set the custom view's accessibility identifier and we could potentially access it like:

app.tables["tableView.productList"].otherElements["productView-🍌"].buttons["productView.button.info"].tap()

Though, if we set the cell accessibility identifier, we can use UITableViewCell's Voice over support by default instead of providing our own (can still provide our own if needed). By using the section + cell keys provided by the cell configuration, we also get guaranteed unique identifiers for free. We could then access the button similarly:

app.tables["tableView.productList"].cells["fruit-🍌"].buttons["productView.button.info"].tap()

And also tap the cell like:

app.tables["tableView.productList"].cells["fruit-🍌"].tap()

Now you have access to a specific cell and any of its hosted view's subviews that have accessibility identifiers.

Having unique identifiers for things like individual orders has been pretty useful so far. If you want to access the 4th order,

That's the part I don't agree with.

// pseudo code warning let orders = XCTFindElement("orderCell").limit(5) // ... then do something with the actual content // I think its better to have access to "generic UI elements"

I have always been talking about testing here, not accessibility information. I don't think the tests should be written with "specifics" in mind, we should be able to easily query for all orderCells in the screen instead of querying for section1-orderCell25.

What's the downside of doing this though? You can still set accessibility state on the hosted view. Why not both?

As others have said this is purely additive - you can still access elements in a set by index, that's not changing.

For list views, maybe we shouldn't write UI tests that target rows by ID. But for detail views, the keypath usually carries semantic meaning and is a useful way of identifying particular pieces of the UI.

This change gives us the flexibility to use both approaches.

ArielSD

Code looks good to me - Curious to understand Raul's point more.

ArielSD · 2018-09-16T17:49:08Z

 		let row = indexPath.row
 		let cellConfig = sectionData[row]
 		let cell = cellConfig.dequeueCell(from: tableView, at: indexPath)
+		cell.accessibilityIdentifier = sectionData.sectionKeyPathForRow(row)


Meaning that the HostCell's state should hold the identifier? And for the views inside, couldn't we just set their identifiers to override the cell as the superview like Caio said?

raulriera · 2018-09-16T17:57:19Z

+
+		let cell = tableData.tableView?.visibleCells.first
+		XCTAssertNotNil(cell)
+		XCTAssertEqual(cell!.accessibilityIdentifier, section.sectionKeyPathForRow(0))


Can you type out the expected string instead? If the sectionKeyPathForRow changes, this will not break

cell?.accessibilityIndentifier. XCTAssertEqual handles optionals just fine, and this prevents the test from crashing if XCTAssertNotNil(cell) fails.

Agree with Raul though that it should use a hardcoded comparison value. That documents the format of the expected output, on which all of our unit tests will depend, so if it changes we want to know about it.

raulriera · 2018-09-16T18:07:12Z

 		let row = indexPath.row
 		let cellConfig = sectionData[row]
 		let cell = cellConfig.dequeueCell(from: tableView, at: indexPath)
+		cell.accessibilityIdentifier = sectionData.sectionKeyPathForRow(row)


Yes, that is what I mean. I have no strong opinions on it, but I dont think the key makes a great identifier. Not every view needs an unique identifier, I would argue all orders cell for example should have the same identifier. But I dont know the scope of this change

alinebee

Minor test change, otherwise I think this is a good addition 👍

alinebee · 2018-09-17T19:10:04Z

+
+		let cell = tableData.tableView?.visibleCells.first
+		XCTAssertNotNil(cell)
+		XCTAssertEqual(cell!.accessibilityIdentifier, section.sectionKeyPathForRow(0))


cell?.accessibilityIndentifier. XCTAssertEqual handles optionals just fine, and this prevents the test from crashing if XCTAssertNotNil(cell) fails.

Agree with Raul though that it should use a hardcoded comparison value. That documents the format of the expected output, on which all of our unit tests will depend, so if it changes we want to know about it.

Caiopia · 2018-09-17T21:10:08Z

Updated test to use hard coded accessibility identifier.

Caiopia added 2 commits September 14, 2018 16:07

set cell accessibility identifiers to section+cell keys

81c07f7

add tests to check accessibility identifiers

f6a7fdb

Caiopia requested review from ArielSD and raulriera September 14, 2018 22:16

raulriera reviewed Sep 15, 2018

View reviewed changes

ArielSD approved these changes Sep 16, 2018

View reviewed changes

raulriera reviewed Sep 16, 2018

View reviewed changes

krbarnes approved these changes Sep 17, 2018

View reviewed changes

alinebee reviewed Sep 17, 2018

View reviewed changes

hard code accessibility identifier in test

645787e

Caiopia merged commit f19de34 into master Sep 18, 2018

Caiopia deleted the accessibility-identifiers branch September 18, 2018 14:08

Conversation

Caiopia commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Example

Uh oh!

raulriera Sep 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Caiopia Sep 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulriera Sep 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArielSD left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alinebee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Caiopia commented Sep 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Caiopia commented Sep 14, 2018 •

edited

Loading

raulriera Sep 15, 2018 •

edited

Loading

Caiopia Sep 16, 2018 •

edited

Loading

raulriera Sep 16, 2018 •

edited

Loading