Skip to content

Set cell accessibility identifiers to section+cell keys #109

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.

Sign up for GitHub

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

Merged
Caiopia merged 3 commits into from
Sep 18, 2018
Merged

Set cell accessibility identifiers to section+cell keys #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81c07f7
set cell accessibility identifiers to section+cell keys
Caiopia Sep 14, 2018
f6a7fdb
add tests to check accessibility identifiers
Caiopia Sep 14, 2018
645787e
hard code accessibility identifier in test
Caiopia Sep 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions FunctionalTableData/TableView/FunctionalTableData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,7 @@ extension FunctionalTableData: UITableViewDataSource {
let row = indexPath.row
let cellConfig = sectionData[row]
let cell = cellConfig.dequeueCell(from: tableView, at: indexPath)
cell.accessibilityIdentifier = sectionData.sectionKeyPathForRow(row)
Copy link
Copy Markdown
Contributor

@raulriera raulriera Sep 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@ArielSD ArielSD Sep 16, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

@raulriera raulriera Sep 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@Caiopia Caiopia Sep 16, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@Caiopia Caiopia Sep 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@raulriera raulriera Sep 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@krbarnes krbarnes Sep 17, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@alinebee alinebee Sep 17, 2018

Choose a reason for hiding this comment

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

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.


cellConfig.update(cell: cell, in: tableView)
sectionData.mergedStyle(for: row)?.configure(cell: cell, in: tableView)
Expand Down
18 changes: 18 additions & 0 deletions FunctionalTableDataTests/FunctionalDataTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,24 @@ class FunctionalDataTests: XCTestCase {
XCTAssertNotNil(indexPath)
}

func testCellAccessibilityIdentifiers() {
let tableData = FunctionalTableData()
let tableView = UITableView()

tableData.tableView = tableView
let expectation1 = expectation(description: "rendered")
let cellConfig = TestCaseCell(key: "cellKey", state: TestCaseState(data: "red"), cellUpdater: TestCaseState.updateView)
let section = TableSection(key: "sectionKey", rows: [cellConfig])

tableData.renderAndDiff([section]) {
expectation1.fulfill()
}
waitForExpectations(timeout: 1, handler: nil)

let cell = tableData.tableView?.visibleCells.first
XCTAssertEqual(cell?.accessibilityIdentifier, "sectionKeycellKey")
}

func testPerformance() {
let functionalData = FunctionalTableData()

Expand Down