[iOS] Update image cell to use content configuration [chromium/src : main]
Louis Romero (Gerrit)
Louis Romero voted and added 8 comments![]( "Open in Gerrit")
Votes added by Louis Romero
| Code-Review | +1 |
8 comments
@property(nonatomic, assign) BOOL disabledText;```suggestion
@property(nonatomic, assign, getter=isTextDisabled) BOOL textDisabled;
```
// Whether the labels should be disabled (change text color). Default NO.Maybe YAGNI, but shouldn't this be settable per label?
// LINT.IfChange(Copy)Out of curiosity: did this LINT.IfChange helped you or you still had that in mind when making the change?
_imageView = [[UIImageView alloc] init];
// The favicon image is smaller than its UIImageView's bounds, so center it.
_imageView.contentMode = UIViewContentModeCenter;
[_imageView setContentHuggingPriority:UILayoutPriorityRequired
forAxis:UILayoutConstraintAxisHorizontal];
// Set font size using dynamic type.
_textLabel = [[UILabel alloc] init];
_textLabel.font = [UIFont preferredFontForTextStyle:UIFontTextStyleBody];
_textLabel.adjustsFontForContentSizeCategory = YES;
_textLabel.numberOfLines = 2;
[_textLabel
setContentCompressionResistancePriority:UILayoutPriorityDefaultLow
forAxis:
UILayoutConstraintAxisHorizontal];
_detailTextLabel = [[UILabel alloc] init];
_detailTextLabel.font =
[UIFont preferredFontForTextStyle:UIFontTextStyleFootnote];
_detailTextLabel.adjustsFontForContentSizeCategory = YES;
_detailTextLabel.numberOfLines = 0;
UIStackView* verticalStack = [[UIStackView alloc]
initWithArrangedSubviews:@[ _textLabel, _detailTextLabel ]];
verticalStack.translatesAutoresizingMaskIntoConstraints = NO;
verticalStack.axis = UILayoutConstraintAxisVertical;
verticalStack.spacing = 0;
verticalStack.distribution = UIStackViewDistributionFill;
verticalStack.alignment = UIStackViewAlignmentLeading;
[self.contentView addSubview:verticalStack];
UIStackView* horizontalStack = [[UIStackView alloc]
initWithArrangedSubviews:@[ _imageView, verticalStack ]];
horizontalStack.translatesAutoresizingMaskIntoConstraints = NO;
horizontalStack.axis = UILayoutConstraintAxisHorizontal;
horizontalStack.spacing = kTableViewSubViewHorizontalSpacing;
horizontalStack.distribution = UIStackViewDistributionFill;
horizontalStack.alignment = UIStackViewAlignmentCenter;
[self.contentView addSubview:horizontalStack];
NSLayoutConstraint* heightConstraint = [self.contentView.heightAnchor
constraintGreaterThanOrEqualToConstant:kChromeTableViewCellHeight];
// Don't set the priority to required to avoid clashing with the estimated
// height.
heightConstraint.priority = UILayoutPriorityRequired - 1;
[NSLayoutConstraint activateConstraints:@[
// Horizontal Stack constraints.
[horizontalStack.leadingAnchor
constraintEqualToAnchor:self.contentView.leadingAnchor
constant:kTableViewHorizontalSpacing],
[horizontalStack.trailingAnchor
constraintEqualToAnchor:self.contentView.trailingAnchor
constant:-kTableViewHorizontalSpacing],
[horizontalStack.centerYAnchor
constraintEqualToAnchor:self.contentView.centerYAnchor],
[horizontalStack.topAnchor
constraintGreaterThanOrEqualToAnchor:self.contentView.topAnchor
constant:kTableViewVerticalSpacing],
[horizontalStack.bottomAnchor
constraintLessThanOrEqualToAnchor:self.contentView.bottomAnchor
constant:-kTableViewVerticalSpacing],
heightConstraint,
]];
}Rubberstamp on this. Please ack or tell me if you want thorough look at it.
self.userInteractionEnabled = YES;I think this should be added to LegacyTableViewCell? As otherwise, reusing a cell disabled by a TableViewImageItem with another unsuspecting item could make the cell disabled by mistake.
- (UIAccessibilityTraits)accessibilityTraits {
UIAccessibilityTraits accessibilityTraits = super.accessibilityTraits;
if (!self.isUserInteractionEnabled) {
accessibilityTraits |= UIAccessibilityTraitNotEnabled;
}
return accessibilityTraits;
}This behavior is missing in the new code, no?
- (NSArray<NSString*>*)accessibilityUserInputLabels {This behavior is missing in the new code, no?
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted and added 7 comments![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Commit-Queue | +1 |
7 comments
```suggestion
@property(nonatomic, assign, getter=isTextDisabled) BOOL textDisabled;
```
Done
// Whether the labels should be disabled (change text color). Default NO.Maybe YAGNI, but shouldn't this be settable per label?
I thought about it, but I don't see a case where it would make sense to have only some labels disabled but not all.
This is more "the whole cell is disabled" kind of thing in my opinion.
Out of curiosity: did this LINT.IfChange helped you or you still had that in mind when making the change?
I had it in mind 😊
There are small changes (margins are not the same etc), but overall the design is close enough in my opinion.
self.userInteractionEnabled = YES;I think this should be added to LegacyTableViewCell? As otherwise, reusing a cell disabled by a TableViewImageItem with another unsuspecting item could make the cell disabled by mistake.
Done
- (UIAccessibilityTraits)accessibilityTraits {
UIAccessibilityTraits accessibilityTraits = super.accessibilityTraits;
if (!self.isUserInteractionEnabled) {
accessibilityTraits |= UIAccessibilityTraitNotEnabled;
}
return accessibilityTraits;
}This behavior is missing in the new code, no?
Done
- (NSArray<NSString*>*)accessibilityUserInputLabels {This behavior is missing in the new code, no?
I am not sure to understand why this is here.
Isn't the accessibility label enough?
Related details
- Louis Romero
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Louis Romero (Gerrit)
Louis Romero added 3 comments![]( "Open in Gerrit")
// Whether the labels should be disabled (change text color). Default NO.Gauthier AmbardMaybe YAGNI, but shouldn't this be settable per label?
I thought about it, but I don't see a case where it would make sense to have only some labels disabled but not all.
This is more "the whole cell is disabled" kind of thing in my opinion.
Acknowledged
Gauthier AmbardRubberstamp on this. Please ack or tell me if you want thorough look at it.
There are small changes (margins are not the same etc), but overall the design is close enough in my opinion.
Acknowledged
- (NSArray<NSString*>*)accessibilityUserInputLabels {Gauthier AmbardThis behavior is missing in the new code, no?
I am not sure to understand why this is here.
Isn't the accessibility label enough?
Apple doc (thanks for nothing!): https://developer.apple.com/documentation/objectivec/nsobject-swift.class/accessibilityuserinputlabels?language=objc
https://iosdev.space/@dadederk/110355939404315012 shows it well: this is for Voice Control. With the current accessibilityLabel implementation and without this, users have to utter "text, detail text", not just "text". This adds the possibility to just read the text.
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Louis Romero (Gerrit)
Louis Romero added 3 comments![]( "Open in Gerrit")
UIAccessibilityTraits accessibilityTraits = super.accessibilityTraits;Out of curiosity: Did you check that super didn't return UIAccessibilityTraitNotEnabled already when !self.isUserInteractionEnabled?
cell.accessibilityLabel = configuration.accessibilityLabel;So how is the label being set on the cell now?
- (NSArray<NSString*>*)accessibilityUserInputLabels {Gauthier AmbardThis behavior is missing in the new code, no?
Louis RomeroI am not sure to understand why this is here.
Isn't the accessibility label enough?
Apple doc (thanks for nothing!): https://developer.apple.com/documentation/objectivec/nsobject-swift.class/accessibilityuserinputlabels?language=objc
https://iosdev.space/@dadederk/110355939404315012 shows it well: this is for Voice Control. With the current accessibilityLabel implementation and without this, users have to utter "text, detail text", not just "text". This adds the possibility to just read the text.
I don't have a great solution for this other than to add it to the config and pass it to the cell like we do for the accessibility label? (cf previous brainstorm re: this accessibility)
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted and added 3 comments![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Auto-Submit | +1 |
| Commit-Queue | +1 |
3 comments
UIAccessibilityTraits accessibilityTraits = super.accessibilityTraits;Out of curiosity: Did you check that super didn't return UIAccessibilityTraitNotEnabled already when !self.isUserInteractionEnabled?
I checked, it doesn't. I think this is because the cell is not a UIControl.
cell.accessibilityLabel = configuration.accessibilityLabel;So how is the label being set on the cell now?
Done
- (NSArray<NSString*>*)accessibilityUserInputLabels {Gauthier AmbardThis behavior is missing in the new code, no?
Louis RomeroI am not sure to understand why this is here.
Isn't the accessibility label enough?
Louis RomeroApple doc (thanks for nothing!): https://developer.apple.com/documentation/objectivec/nsobject-swift.class/accessibilityuserinputlabels?language=objc
https://iosdev.space/@dadederk/110355939404315012 shows it well: this is for Voice Control. With the current accessibilityLabel implementation and without this, users have to utter "text, detail text", not just "text". This adds the possibility to just read the text.
I don't have a great solution for this other than to add it to the config and pass it to the cell like we do for the accessibility label? (cf previous brainstorm re: this accessibility)
- Louis Romero
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Louis Romero (Gerrit)
Louis Romero voted and added 5 comments![]( "Open in Gerrit")
Votes added by Louis Romero
| Code-Review | +1 |
5 comments
self.accessibilityUserInputLabels = nil;Good idea. Likely also the accessibilityLabel?
_accessibilityLabel = accessibilityLabel;```suggestion
_accessibilityLabel = [accessibilityLabel copy];
```
But can't we use the parent class property instead?
_accessibilityUserInputLabels = accessibilityUserInputLabels;```suggestion
_accessibilityUserInputLabels = [accessibilityUserInputLabels copy];
```
- (NSArray<NSString*>*)accessibilityUserInputLabels {Nice. I hadn't seen that this pattern was already in place for the label!
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted and added 4 comments![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Auto-Submit | +1 |
| Commit-Queue | +1 |
4 comments
Good idea. Likely also the accessibilityLabel?
Done
_accessibilityLabel = accessibilityLabel;```suggestion
_accessibilityLabel = [accessibilityLabel copy];
```But can't we use the parent class property instead?
It is a "strong" in the documentation: https://developer.apple.com/documentation/uikit/uiaccessibilityelement/accessibilitylabel?language=objc
_accessibilityUserInputLabels = accessibilityUserInputLabels;```suggestion
_accessibilityUserInputLabels = [accessibilityUserInputLabels copy];
```
Same, the documentation has it as "strong" https://developer.apple.com/documentation/objectivec/nsobject-swift.class/accessibilityuserinputlabels?language=objc
Related details
- Louis Romero
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Louis Romero (Gerrit)
Louis Romero voted and added 2 comments![]( "Open in Gerrit")
Votes added by Louis Romero
| Code-Review | +1 |
2 comments
_accessibilityLabel = accessibilityLabel;Gauthier Ambard```suggestion
_accessibilityLabel = [accessibilityLabel copy];
```But can't we use the parent class property instead?
It is a "strong" in the documentation: https://developer.apple.com/documentation/uikit/uiaccessibilityelement/accessibilitylabel?language=objc
Acknowledged
_accessibilityUserInputLabels = accessibilityUserInputLabels;Gauthier Ambard```suggestion
_accessibilityUserInputLabels = [accessibilityUserInputLabels copy];
```
Same, the documentation has it as "strong" https://developer.apple.com/documentation/objectivec/nsobject-swift.class/accessibilityuserinputlabels?language=objc
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Louis Romero (Gerrit)
Louis Romero voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[iOS] Update image cell to use content configuration
This CL updates the ImageItem to remove the use of ImageCell.
It also updates the FilePicker to stop using the image cell.
Before:
https://screenshot.googleplex.com/76qLF76cHRaKqC4
https://screenshot.googleplex.com/AutVBZgDjgJ3MYa
https://screenshot.googleplex.com/6ZNEAALAKQVvu8T
After:
https://screenshot.googleplex.com/BUrzSHb9AHcSjvk
https://screenshot.googleplex.com/QNJyTHcfzDcXPs4
https://screenshot.googleplex.com/iWDsPfNh7jovHae
- M ios/chrome/browser/bookmarks/ui_bundled/home/bookmarks_batch_upload_egtest.mm
- M ios/chrome/browser/save_to_drive/ui_bundled/file_destination_picker_view_controller.mm
- M ios/chrome/browser/shared/ui/table_view/cells/legacy_table_view_cell.mm
- M ios/chrome/browser/shared/ui/table_view/cells/table_view_cell.mm
- M ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.h
- M ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_configuration.mm
- M ios/chrome/browser/shared/ui/table_view/cells/table_view_cell_content_view.mm
- M ios/chrome/browser/shared/ui/table_view/cells/table_view_image_item.h
- M ios/chrome/browser/shared/ui/table_view/cells/table_view_image_item.mm
- M ios/chrome/browser/shared/ui/table_view/cells/table_view_image_item_unittest.mm
Code-Review: +1 by Louis Romero
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |