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

Accessibility #263

Merged
merged 10 commits into from
Jan 27, 2017
Merged

Accessibility #263

merged 10 commits into from
Jan 27, 2017

Conversation

aral
Copy link
Contributor

@aral aral commented Jan 24, 2017

The demo strings in the iOS Examples app are currently not accessible.

Specifically:

  • The XML example has a decorative image that is visible to the accessibility system and shouldn’t be.
  • The Composition example has a content image without alternative text.
  • The Images & Special characters example has content images without alternative text.

Furthermore, there is a bug in the tintedImage(color:) method in the BONImage extension that loses the accessibility label on the original image.

This pull request:

  1. Fixes the tintedImage(color:) method in the BONImage extension so that the resulting tinted image has the same accessibility label as the original image.
  2. Adds accessibility support to the images in the iOS Example app.
  3. (Minor) Adds the .idea folder added to projects by AppCode to .gitignore.

Regarding the second point, the difference in experience of a person using the iOS Example app with VoiceOver is the following:

img_5887

XML example

Before: Maria Sharapova attachment png, crisp, very dark, file

After: Maria Sharapova


Composition:

Before: You’re going to need a bigger attachment, png, crisp, dark, file

After: You’re going to need a bigger boat


Images & Special Characters:

Before: 2 2

After: 2 bee oar knot 2 bee

@ZevEisenberg
Copy link
Collaborator

ZevEisenberg commented Jan 24, 2017

@aral it's saying "file", not "foul" (I slowed down the speaking rate to check).

If you make a change to the branch (even a trivial one), I've enabled CircleCI builds for fork pull requests. I updated the branch after merging something in to master, so it's building now.

@ZevEisenberg
Copy link
Collaborator

@aral it looks like accessibilityLabel on iOS/tvOS maps to accessibilityDescription on the Mac. Would you mind updating your branch to fix the Mac build? You can test it locally by running tests in the BonMot-OSX target or building the AllTheThings target.

@ZevEisenberg
Copy link
Collaborator

@aral oh, and it also looks like accessibilityLabel isn't available on UIImage on watchOS, so you'll probably want to conditionally include that line as well.

UIImage(named: "bee")!,
static let imagesExample: NSAttributedString = {

func accessibleImage(named name:String) -> UIImage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Space after colon (per SwiftLint)

@@ -64,6 +64,9 @@ public extension BONImage {
// Prevent further tinting
image.isTemplate = false

// Transfer accessibility label
image.accessibilityLabel = self.accessibilityLabel
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, this is accessibilityDescription on the Mac.

@@ -85,4 +85,13 @@ class ImageTintingTests: XCTestCase {
BONAssertEqualImages(untintedResult!, tintAttemptResult!)
}

func testAccessibilityLabelTransferToTintedImage() {
let accessibilityLabel = "I’m the very model of a modern accessible image."
let accessibleImage = UIImage(cgImage: imageForTest.cgImage!)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use BONImage instead of UIImage for cross-platform. You'll need to use accessibilityLabel vs. accessibilityDescription on iOS vs. Mac. Let me know if you need help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As for the initializer, and various permutations of optional an non-optional parameters, you may want to add some helpers to Compatibility+Tests.swift, or use a bunch of conditional compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for all the notes, Zev. Will update today. Sorry for missing the cross-platform issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem! It's pretty rare for a project like this. I should make sure the readme includes enough info on contributing. I never imagined we'd get as much community involvement as we have!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked into it on WatchKit a bit and couldn’t come up with anything. I don’t have any Apple Watch apps at the moment. Will look into it again when I can and possibly file a radar. Do you know of anyone using the framework in an open source Watch app I can look at?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't. I haven't seen much watchOS development in the wild.

@aral
Copy link
Contributor Author

aral commented Jan 25, 2017

The latest commits should address all the review points. Do let me know if I missed anything.

@@ -113,6 +116,11 @@ public extension BONImage {
image = image.resizableImage(withCapInsets: originalCapInsets, resizingMode: originalResizingMode)
}

// Transfer accessibility label (iOS only; watchOS does not have accessibilityLabel on UIImage).
#if os(iOS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about tvOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -113,6 +116,11 @@ public extension BONImage {
image = image.resizableImage(withCapInsets: originalCapInsets, resizingMode: originalResizingMode)
}

// Transfer accessibility label (iOS only; watchOS does not have accessibilityLabel on UIImage).
#if os(iOS)
image.accessibilityLabel = self.accessibilityLabel
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice-to-have: indent this line (project style: anything enclosed in #if should be indented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@@ -65,7 +67,7 @@ class ImageTintingTests: XCTestCase {
XCTAssertNotNil(tintedResult)

BONAssertNotEqualImages(untintedResult!, tintedResult!)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra whitespace. (If you brew install swiftlint 0.16.1 or higher, it will catch this for you. You can also enable trimming trailing whitespace in Xcode → Preferences → Text Editing → Automatically trim trailing whitespace.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

swiftlint installed – will go mumble to myself about brace placement now ;)

@@ -84,5 +86,23 @@ class ImageTintingTests: XCTestCase {

BONAssertEqualImages(untintedResult!, tintAttemptResult!)
}

func testAccessibilityIOS() {
#if os(iOS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above: what about tvOS?
Also: you could just do a single testAccessibility function, which the contents wrapped in #if, instead of two separate functions. I don't strongly care either way, I guess, but it might let you share more lines of code between the tests.

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 initially had it in one method but decided it made the code harder to read. Updated to include tvOS.

@ZevEisenberg ZevEisenberg merged commit f2fc860 into Rightpoint:master Jan 27, 2017
@ZevEisenberg
Copy link
Collaborator

@aral thanks so much for your contribution! I'm going to try to get a couple more AX things in (#155) and cut a new release today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants