Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
325203003:
Turn off accessibility access to notification images. (Closed)
|
Created:
6 years, 6 months ago by Pete Williamson Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTurn off accessibility access to notification images.
Since we don't have alt text yet, accessibility screen readers have
nothing useful to say in screen readers. Until alt text is available,
disable the images from appearing in the screen reader.
This is done by removing the accessibility child object from a parent.
In this case, we introduce a new fake parent object for the small icon,
and make the existing parent ignore the image for the large icon and
the large image of an image notification.
BUG=381363
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277957
Patch Set 1 #
Total comments: 10
Patch Set 2 : Image hiding: Address RSesek CR feedback. #Patch Set 3 : Image hiding: More RSesek CR feedback. #
Total comments: 8
Messages
Total messages: 13 (0 generated)
Fix for the Mac only
RSesek@: Can you please take a first look at this? DewittJ is not too familiar with Mac programming, and he'd appreciate somebody checking out the Mac-ness before he checks the business logic fits with the Notification Center. Thanks!
https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... File ui/message_center/cocoa/notification_controller.mm (right): https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:216: -(id) accessibilityAttributeValue:(NSString*) attribute { nit: space after - but not after ) https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:216: -(id) accessibilityAttributeValue:(NSString*) attribute { nit: no space after ) in arguments https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:220: return [[[NSMutableArray alloc] init] autorelease]; nit: use [NSMutableArray array], or if it does not have to be mutable, just @[] https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:764: smallImage_.reset([[NSImageView alloc] initWithFrame:smallImageFrame]); Is this frame still correct? Since it's no longer a subview of |rootView|, its positioning may be off.
Style fixes as requested by RSesek. https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... File ui/message_center/cocoa/notification_controller.mm (right): https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:216: -(id) accessibilityAttributeValue:(NSString*) attribute { On 2014/06/11 21:37:04, rsesek wrote: > nit: space after - but not after ) Done. https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:216: -(id) accessibilityAttributeValue:(NSString*) attribute { On 2014/06/11 21:37:04, rsesek wrote: > nit: no space after ) in arguments Done. https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:220: return [[[NSMutableArray alloc] init] autorelease]; On 2014/06/11 21:37:04, rsesek wrote: > nit: use [NSMutableArray array], or if it does not have to be mutable, just @[] Done. https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:764: smallImage_.reset([[NSImageView alloc] initWithFrame:smallImageFrame]); On 2014/06/11 21:37:04, rsesek wrote: > Is this frame still correct? Since it's no longer a subview of |rootView|, its > positioning may be off. The imageBox is supposed to be the same size and position as the smallImage_. This implementation is copied from the createIconView method, which does the same thing. As long as both boxes are the same size, I would expect this to still be valid (I'm by no means an expert in this code, though, so I could have missed something.)
https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... File ui/message_center/cocoa/notification_controller.mm (right): https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:764: smallImage_.reset([[NSImageView alloc] initWithFrame:smallImageFrame]); On 2014/06/11 22:12:54, Pete Williamson wrote: > On 2014/06/11 21:37:04, rsesek wrote: > > Is this frame still correct? Since it's no longer a subview of |rootView|, its > > positioning may be off. > > The imageBox is supposed to be the same size and position as the smallImage_. > This implementation is copied from the createIconView method, which does the > same thing. As long as both boxes are the same size, I would expect this to > still be valid (I'm by no means an expert in this code, though, so I could have > missed something.) Unless I'm misreading the code, it's not quite the same. (Have you tested this visually, does it look right?) The frame is the position and size of a view in relation to its superview. In -createIconView, the view placed inside the box has an origin of (0,0), not the origin relative to the rootView, because it's not inside the rootView. Here the image is being placed inside the box at an origin relative to rootView, even though rootView is not its parent: the box is. https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/CocoaV...
Another fix per RSesek. https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... File ui/message_center/cocoa/notification_controller.mm (right): https://codereview.chromium.org/325203003/diff/1/ui/message_center/cocoa/noti... ui/message_center/cocoa/notification_controller.mm:764: smallImage_.reset([[NSImageView alloc] initWithFrame:smallImageFrame]); On 2014/06/12 13:31:10, rsesek wrote: > On 2014/06/11 22:12:54, Pete Williamson wrote: > > On 2014/06/11 21:37:04, rsesek wrote: > > > Is this frame still correct? Since it's no longer a subview of |rootView|, > its > > > positioning may be off. > > > > The imageBox is supposed to be the same size and position as the smallImage_. > > This implementation is copied from the createIconView method, which does the > > same thing. As long as both boxes are the same size, I would expect this to > > still be valid (I'm by no means an expert in this code, though, so I could > have > > missed something.) > > Unless I'm misreading the code, it's not quite the same. (Have you tested this > visually, does it look right?) The frame is the position and size of a view in > relation to its superview. In -createIconView, the view placed inside the box > has an origin of (0,0), not the origin relative to the rootView, because it's > not inside the rootView. Here the image is being placed inside the box at an > origin relative to rootView, even though rootView is not its parent: the box is. > > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/CocoaV... Ah, I see. The code I cut and pasted worked since it was at the origin. Fixed to use a separate box starting at 0,0 for the inner image. I had not tested the image positioning before (most of our test messages leave the field blank), but I have now with the change, and it is working properly.
LGTM https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... File ui/message_center/cocoa/notification_controller.mm (right): https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... ui/message_center/cocoa/notification_controller.mm:766: NSMakeRect(0,0, nit: space after first 0,
lgtm + nits https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... File ui/message_center/cocoa/notification_controller.mm (right): https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... ui/message_center/cocoa/notification_controller.mm:206: // Ignore this element, but expose its children to accessibility. This comment seems out of date? https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... ui/message_center/cocoa/notification_controller.mm:314: [rootView addSubview:[self createSmallImageInFrame:rootFrame]]; Any reason you diverged from the prevailing style here? [self configureFoo]; [rootView addSubview:fooView_];
https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... File ui/message_center/cocoa/notification_controller.mm (right): https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... ui/message_center/cocoa/notification_controller.mm:206: // Ignore this element, but expose its children to accessibility. On 2014/06/12 21:02:27, dewittj wrote: > This comment seems out of date? Hmm, not sure how to change this to make it better. That is in fact what this method does, but the other method makes the kids not appear. In the future we will remove the other method, but this method will remain, and its comment will remain valid. Can you think of a wording that makes more sense to you? https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... ui/message_center/cocoa/notification_controller.mm:314: [rootView addSubview:[self createSmallImageInFrame:rootFrame]]; On 2014/06/12 21:02:27, dewittj wrote: > Any reason you diverged from the prevailing style here? > > [self configureFoo]; > [rootView addSubview:fooView_]; I was following the same pattern that the Icon view used: [rootView addSubview:[self createIconView]]; We're really doing a create now, not a configure, so I thought it made more sense to follow the createIconView pattern. I'm happy to change both if you like. https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... ui/message_center/cocoa/notification_controller.mm:766: NSMakeRect(0,0, On 2014/06/12 18:19:48, rsesek wrote: > nit: space after first 0, Done.
The CQ bit was checked by petewil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/325203003/40001
https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... File ui/message_center/cocoa/notification_controller.mm (right): https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... ui/message_center/cocoa/notification_controller.mm:206: // Ignore this element, but expose its children to accessibility. On 2014/06/17 17:36:36, Pete Williamson wrote: > On 2014/06/12 21:02:27, dewittj wrote: > > This comment seems out of date? > > Hmm, not sure how to change this to make it better. That is in fact what this > method does, but the other method makes the kids not appear. In the future we > will remove the other method, but this method will remain, and its comment will > remain valid. > > Can you think of a wording that makes more sense to you? The problem is partly that the comment is above the @implementation line, not the - (BOOL)accessibilityIsIgnored line, making it a comment that applies to the whole interface. I'd say move the comment up to line 202, where it can document the AccessibilityIgnoredBox, and fully describe the behavior. Then between lines 207 and 208 you can describe that this function alone does not cause children to be ignored. https://codereview.chromium.org/325203003/diff/40001/ui/message_center/cocoa/... ui/message_center/cocoa/notification_controller.mm:314: [rootView addSubview:[self createSmallImageInFrame:rootFrame]]; On 2014/06/17 17:36:35, Pete Williamson wrote: > On 2014/06/12 21:02:27, dewittj wrote: > > Any reason you diverged from the prevailing style here? > > > > [self configureFoo]; > > [rootView addSubview:fooView_]; > > I was following the same pattern that the Icon view used: > [rootView addSubview:[self createIconView]]; > > We're really doing a create now, not a configure, so I thought it made more > sense to follow the createIconView pattern. I'm happy to change both if you > like. Okay I see better now why it's different. sounds ok
Message was sent while issue was closed.
Change committed as 277957 |
