Added attributed versions of accessibilityLabel, accessibilityHint, accessibilityValue #554

fruitcoder · 2017-09-07T15:25:06Z

First version of how we might add attributed string functionality to accessibility labels, hints and values. The properties are only available from iOS 11 and tv 11 and up so I added API_AVAILABLE to the properties in ASDisplayNode.

…d accessibilityValue

ghost · 2017-09-07T15:34:15Z

🚫 CI failed with log

nguyenhuy

Thanks for working on this! Overall, LGTM. But let's sort out a few questions before merging.

nguyenhuy · 2017-09-13T14:41:02Z

Source/ASDisplayNode.h

@@ -716,8 +716,11 @@ extern NSInteger const ASDefaultDrawingPriority;
 // Accessibility support
 @property (nonatomic, assign) BOOL isAccessibilityElement;
 @property (nonatomic, copy, nullable) NSString *accessibilityLabel;
+@property (nullable, nonatomic, copy) NSAttributedString *accessibilityAttributedLabel API_AVAILABLE(ios(11.0),tvos(11.0));


To follow the existing convention, let's put nullable last. So (nonatomic, copy, nullable)

nguyenhuy · 2017-09-13T14:44:22Z

Source/Details/_ASDisplayViewAccessiblity.mm

+ NSArray *attributedLabels = [labeledNodes valueForKey:@"accessibilityAttributedLabel"];
+ NSMutableAttributedString *attributedLabel = [NSMutableAttributedString new];
+ [attributedLabels enumerateObjectsUsingBlock:^(id _Nonnull obj, NSUInteger idx, BOOL * _Nonnull stop) {
+ if (idx != 0)


The convention is to use curly brackets regardless of the number of statements followed.

Sorry I thought I followed convention because I saw this in _PendingState.mm a lot for single statements (L375, L377), but I'll change my code to use curly braces.

Yeah, we need to have it written down in CONTRIBUTING.md. Others may disagree, but I personally follow this convention (as much as possible).

I wouldn't change it for applyToView:withSpecialPropertiesHandling: though since every flag check seems to skip the curly braces

nguyenhuy · 2017-09-13T14:51:10Z

Source/Private/_ASPendingState.mm

@@ -586,9 +595,26 @@ - (NSString *)accessibilityLabel

 - (void)setAccessibilityLabel:(NSString *)newAccessibilityLabel
 {
- _flags.setAccessibilityLabel = YES;
 if (accessibilityLabel != newAccessibilityLabel) {


Hm, are we comparing strings here?

Yes, but this is from the original implementation: https://github.com/fruitcoder/Texture/blob/master/Source/Private/_ASPendingState.mm#L590

Yes, I see you follow this implementation throughout your changes. I should have asked better questions, sorry.

Is there any reason to compare memory addresses here? Is the original implementation correct/acceptable?

TBO I would compare those Strings with isEqualToString:, since if they are equal (by means of isEqualToString), we don't have to do anything.

I agree. I'd do it the right way and not worry about the perf implication of isEqualToString, compared to ==, especially because those labels aren't changed often I believe. Would you mind taking this opportunity to do an audit in this file and switch to the formally correct compare methods?

The existing code makes some sense, since a common use case for accessibility is to use constants for them.

The pointer compare made more sense when (1) CPUs were way slower and (2) this was an in-house framework.

I agree we should change to use isEqualToString:.

Also just below here, we don't need to explicitly copy the newAccessibilityLabel before passing it to initWithString:. That initializer copies the contents ("initialized with the characters of aString").

nguyenhuy · 2017-09-13T14:56:27Z

Source/Private/ASDisplayNode+UIViewBridge.mm

+{
+ _bridge_prologue_write;
+ { _setAccessibilityToViewAndProperty(_accessibilityAttributedLabel, accessibilityAttributedLabel, accessibilityAttributedLabel, accessibilityAttributedLabel); }
+ { _setAccessibilityToViewAndProperty(_accessibilityLabel, accessibilityAttributedLabel.string, accessibilityLabel, accessibilityAttributedLabel.string); }


This looks a bit strange. Are attributed labels supposed to overwrite non-attributed ones? Since these attributed labels are still in beta, I guess we don't know yet?

This is in fact the behaviour I'm observing. It's like UILabel's attributedText and text attributes, which override themselves (confirmed in Playground with XC9 GM).

Cool by me then :)

nguyenhuy · 2017-09-13T14:59:00Z

Also, please update CHANGELOG and make sure CI builds. Thanks!

ghost · 2017-09-15T10:51:21Z

🚫 CI failed with log

nguyenhuy · 2017-09-15T10:53:17Z

Source/Details/UIView+ASConvenience.h

+ @property (nonatomic, copy, nullable) NSString *accessibilityHint;
+ @property (nullable, nonatomic, copy) NSAttributedString *accessibilityAttributedHint API_AVAILABLE(ios(11.0),tvos(11.0));
+ @property (nonatomic, copy, nullable) NSString *accessibilityValue;
+ @property (nullable, nonatomic, copy) NSAttributedString *accessibilityAttributedValue API_AVAILABLE(ios(11.0),tvos(11.0));


Please follow the (nonatomic, copy, nullable) convention for changes in this file.

fruitcoder · 2017-09-15T13:41:19Z

The CI Build fails can all be categorized in 2 errors in _ASPendingState.mm:
if (@available(iOS 11.0, *)) unexpected '@' in program -> affects all @availibility checks
property 'accessibilityAttributedHint' not found on object of type 'UIView *' -> affects all accesses on the new attributed versions of accessibility label/hint/value.

I don't know how to resolve those issues. Which Xcode are you using on your CI server?

ghost · 2017-09-15T14:07:39Z

🚫 CI failed with log

nguyenhuy · 2017-09-15T17:01:34Z

@fruitcoder We're running Xcode 8. @garrettmoon and I agreed that we shouldn't upgrade to Xcode 9 until it's out of GM. And even so, using @available in the framework will force people to use Xcode 9. So this PR either needs to wait for a while, or switch to AS_AT_LEAST_IOS11 provided in ASAvailability.h for the time being. There will come a time when we migrate to the new stuff, just not very soon I'm afraid.

fruitcoder · 2017-09-16T15:43:59Z

Totally understand, I‘ll update my code to run on XCode 8 on Monday

nguyenhuy · 2017-09-16T15:48:14Z

Great. Thank you!

fruitcoder · 2017-09-18T11:20:48Z

I'm not too happy about the runtime code with setValue:forKey: and valueForKey: but it's the shortest I could come up with. I added two macros which use the runtime accessors/setters for the attributed versions, but you could still use them without checking the API Version and using bad keys. Any alternative ideas?

nguyenhuy

1 last iteration and this PR will be good to land!

nguyenhuy · 2017-09-18T11:24:14Z

Source/Private/ASDisplayNode+UIViewBridge.mm

@@ -901,6 +911,28 @@ - (void)setAccessibilityLabel:(NSString *)accessibilityLabel
 {
 _bridge_prologue_write;
 _setAccessibilityToViewAndProperty(_accessibilityLabel, accessibilityLabel, accessibilityLabel, accessibilityLabel);
+ if (AS_AT_LEAST_IOS11) {
+ _accessibilityAttributedLabel = [[NSAttributedString alloc] initWithString:accessibilityLabel];
+ BOOL shouldApply = ASDisplayNodeShouldApplyBridgedWriteToView(self);


Use _setAttributedAccessibilityToViewAndProperty here?

nguyenhuy · 2017-09-18T11:29:24Z

Source/Private/ASDisplayNode+UIViewBridge.mm

+{
+ _bridge_prologue_write;
+ { _setAttributedAccessibilityToViewAndProperty(_accessibilityAttributedLabel, accessibilityAttributedLabel, @"accessibilityAttributedLabel", accessibilityAttributedLabel); }
+ { _setAttributedAccessibilityToViewAndProperty(_accessibilityLabel, accessibilityAttributedLabel.string, @"accessibilityLabel", accessibilityAttributedLabel.string); }


Shouldn't we use _setAccessibilityToViewAndProperty for _accessibilityLabel?

Hm, I wonder if one of the tests should have caught that. But you're definitely right!

nguyenhuy · 2017-09-18T11:31:56Z

Source/Private/ASDisplayNode+UIViewBridge.mm

@@ -913,6 +945,22 @@ - (void)setAccessibilityHint:(NSString *)accessibilityHint
 {
 _bridge_prologue_write;
 _setAccessibilityToViewAndProperty(_accessibilityHint, accessibilityHint, accessibilityHint, accessibilityHint);
+ if (AS_AT_LEAST_IOS11) {
+ _setAttributedAccessibilityToViewAndProperty(_accessibilityAttributedHint, [[NSAttributedString alloc] initWithString:accessibilityHint], @"accessibilityAttributedHint", [[NSAttributedString alloc] initWithString:accessibilityHint]);


Nit: For readability and a tiny perf win, cache [[NSAttributedString alloc] initWithString:accessibilityHint] instead of allocating 2 identical objects.

nguyenhuy · 2017-09-18T11:32:39Z

Source/Private/ASDisplayNode+UIViewBridge.mm

+{
+ _bridge_prologue_write;
+ { _setAttributedAccessibilityToViewAndProperty(_accessibilityAttributedHint, accessibilityAttributedHint, @"accessibilityAttributedHint", accessibilityAttributedHint); }
+ { _setAttributedAccessibilityToViewAndProperty(_accessibilityHint, accessibilityAttributedHint.string, @"accessibilityHint", accessibilityAttributedHint.string); }


Same as above, use _setAccessibilityToViewAndProperty for _accessibilityHint.

nguyenhuy · 2017-09-18T11:33:01Z

Source/Private/ASDisplayNode+UIViewBridge.mm

@@ -925,6 +973,22 @@ - (void)setAccessibilityValue:(NSString *)accessibilityValue
 {
 _bridge_prologue_write;
 _setAccessibilityToViewAndProperty(_accessibilityValue, accessibilityValue, accessibilityValue, accessibilityValue);
+ if (AS_AT_LEAST_IOS11) {
+ _setAttributedAccessibilityToViewAndProperty(_accessibilityAttributedValue, [[NSAttributedString alloc] initWithString:accessibilityValue], @"accessibilityAttributedValue", [[NSAttributedString alloc] initWithString:accessibilityValue]);


Nit: Same here, cache the attributed string.

nguyenhuy · 2017-09-18T11:33:36Z

Source/Private/ASDisplayNode+UIViewBridge.mm

+{
+ _bridge_prologue_write;
+ { _setAttributedAccessibilityToViewAndProperty(_accessibilityAttributedValue, accessibilityAttributedValue, @"accessibilityAttributedValue", accessibilityAttributedValue); }
+ { _setAttributedAccessibilityToViewAndProperty(_accessibilityValue, accessibilityAttributedValue.string, @"accessibilityValue", accessibilityAttributedValue.string); }


Use _setAccessibilityToViewAndProperty.

nguyenhuy · 2017-09-18T11:42:33Z

@fruitcoder I think it's a good approach for the time being. These new macros are private so I wouldn't worry much about them being used without version checks.

…yToViewAndProperty only for attributed properties.

nguyenhuy · 2017-09-18T17:06:57Z

Landing time 🛬 Thanks, @fruitcoder!

…ted strings for them - Fix crashes caused by TextureGroup#554

…allocating attributed strings for them #trivial (#581) * Make sure accessibility strings are not nil before allocating attributed strings for them - Fix crashes caused by #554 * Update tests

…ccessibilityValue (TextureGroup#554) * Added attributed versions of accessibilityLabel, accessibilityHint and accessibilityValue * Follow conventions for property types * Use curly braces * Update changelog * Follow conventions for property types in UIView+ASConvenience.h * Add compatibility for Xcode 8 * Use isEqualToString instead of pointer comparison * Only allocate attributed strings once. Use _setAttributedAccessibilityToViewAndProperty only for attributed properties.

…allocating attributed strings for them #trivial (TextureGroup#581) * Make sure accessibility strings are not nil before allocating attributed strings for them - Fix crashes caused by TextureGroup#554 * Update tests

Added attributed versions of accessibilityLabel, accessibilityHint an…

54ebd6f

…d accessibilityValue

fruitcoder mentioned this pull request Sep 7, 2017

Attributed versions of accessibility elements #551

Closed

nguyenhuy requested changes Sep 13, 2017

View reviewed changes

fruitcoder added 3 commits September 15, 2017 12:16

Follow conventions for property types

fb89130

Use curly braces

f0e48fc

Update changelog

1677568

nguyenhuy reviewed Sep 15, 2017

View reviewed changes

Follow conventions for property types in UIView+ASConvenience.h

c43f7de

fruitcoder added 2 commits September 18, 2017 13:05

Add compatibility for Xcode 8

b0a5dc1

Use isEqualToString instead of pointer comparison

254637a

nguyenhuy requested changes Sep 18, 2017

View reviewed changes

Only allocate attributed strings once. Use _setAttributedAccessibilit…

7505168

…yToViewAndProperty only for attributed properties.

nguyenhuy approved these changes Sep 18, 2017

View reviewed changes

Merge branch 'master' into attributedA11y

9da8544

nguyenhuy merged commit 76eccbf into TextureGroup:master Sep 18, 2017

nguyenhuy mentioned this pull request Sep 25, 2017

[_ASPendingState] Make sure accessibility strings are not nil before allocating attributed strings for them #trivial #581

Merged

nguyenhuy added the iOS 11 label Sep 26, 2017

nguyenhuy added a commit to nguyenhuy/Texture that referenced this pull request Sep 27, 2017

Make sure accessibility strings are not nil before allocating attribu…

55fe868

…ted strings for them - Fix crashes caused by TextureGroup#554

nguyenhuy mentioned this pull request Oct 24, 2017

Update implementation of support for attributed accessibility properties #636

Closed

fruitcoder mentioned this pull request Oct 26, 2017

Use Xcode9 API For Attributed Versions Of accessibilityLabel, accessibilityHint, accessibilityValue #642

Closed

Added attributed versions of accessibilityLabel, accessibilityHint, accessibilityValue #554

Added attributed versions of accessibilityLabel, accessibilityHint, accessibilityValue #554

Conversation

fruitcoder commented Sep 7, 2017

ghost commented Sep 7, 2017

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy commented Sep 13, 2017

ghost commented Sep 15, 2017

Choose a reason for hiding this comment

fruitcoder commented Sep 15, 2017

ghost commented Sep 15, 2017

nguyenhuy commented Sep 15, 2017

fruitcoder commented Sep 16, 2017

nguyenhuy commented Sep 16, 2017

fruitcoder commented Sep 18, 2017

nguyenhuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy commented Sep 18, 2017

nguyenhuy commented Sep 18, 2017

nguyenhuy Sep 15, 2017 •

edited

Loading