Commit d49ccee4 authored by sczs's avatar sczs Committed by Commit Bot

[ios] Fixes History CollectionVC top margin.

Before crrev.com/c/695934, a top message was always being displayed.
Now thats not the case, this CLremoves the top cell that contains the message if there's no message.

Also, on iOS11 the contentInset was being adjusted automatically, causing an even larger
blank space on top. This is fixed by setting the ContentInsetAdjustmentBehavior to never.

Screenshots:
https://drive.google.com/open?id=1N6egGvEOeSF2uags3w4hmLupWy7Vk4v3
https://drive.google.com/open?id=1hCW15FIUtw2o5rJ7MiqDcRd1z6Xq42O0

Bug: 778571
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I2b9f2b5f92e22f955fb10c5ea609f4ee4f130836
Reviewed-on: https://chromium-review.googlesource.com/830911
Commit-Queue: Sergio Collazos <sczs@chromium.org>
Reviewed-by: default avatarSky Malice <skym@chromium.org>
Reviewed-by: default avatarLouis Romero <lpromero@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524588}
parent 9968c0e5
...@@ -203,6 +203,13 @@ const CGFloat kSeparatorInset = 10; ...@@ -203,6 +203,13 @@ const CGFloat kSeparatorInset = 10;
UIEdgeInsetsMake(0, kSeparatorInset, 0, kSeparatorInset); UIEdgeInsetsMake(0, kSeparatorInset, 0, kSeparatorInset);
self.styler.allowsItemInlay = NO; self.styler.allowsItemInlay = NO;
// Never adjust the content inset on iOS11 or it will create padding on top of
// the first cell.
if (@available(iOS 11, *)) {
[self.collectionView setContentInsetAdjustmentBehavior:
UIScrollViewContentInsetAdjustmentNever];
}
self.clearsSelectionOnViewWillAppear = NO; self.clearsSelectionOnViewWillAppear = NO;
self.collectionView.keyboardDismissMode = self.collectionView.keyboardDismissMode =
UIScrollViewKeyboardDismissModeOnDrag; UIScrollViewKeyboardDismissModeOnDrag;
...@@ -645,21 +652,20 @@ const CGFloat kSeparatorInset = 10; ...@@ -645,21 +652,20 @@ const CGFloat kSeparatorInset = 10;
self.isSearching ? l10n_util::GetNSString(IDS_HISTORY_NO_SEARCH_RESULTS) self.isSearching ? l10n_util::GetNSString(IDS_HISTORY_NO_SEARCH_RESULTS)
: l10n_util::GetNSString(IDS_HISTORY_NO_RESULTS); : l10n_util::GetNSString(IDS_HISTORY_NO_RESULTS);
entriesStatusItem = noResultsItem; entriesStatusItem = noResultsItem;
} else { } else if (self.shouldShowNoticeAboutOtherFormsOfBrowsingHistory) {
HistoryEntriesStatusItem* historyEntriesStatusItem = HistoryEntriesStatusItem* historyEntriesStatusItem =
[[HistoryEntriesStatusItem alloc] initWithType:ItemTypeEntriesStatus]; [[HistoryEntriesStatusItem alloc] initWithType:ItemTypeEntriesStatus];
historyEntriesStatusItem.delegate = self; historyEntriesStatusItem.delegate = self;
historyEntriesStatusItem.hidden = self.isSearching; historyEntriesStatusItem.hidden = self.isSearching;
historyEntriesStatusItem.showsOtherBrowsingDataNotice =
_shouldShowNoticeAboutOtherFormsOfBrowsingHistory;
entriesStatusItem = historyEntriesStatusItem; entriesStatusItem = historyEntriesStatusItem;
} }
// Replace the item in the first section, which is always present. // Replace the item in the first section if it exists. Then insert the new
// item if it exists.
NSArray* items = [self.collectionViewModel NSArray* items = [self.collectionViewModel
itemsInSectionWithIdentifier:kEntriesStatusSectionIdentifier]; itemsInSectionWithIdentifier:kEntriesStatusSectionIdentifier];
if ([items count]) { if ([items count]) {
// There should only ever be one item in this section. // There should only ever be at most one item in this section.
DCHECK([items count] == 1); DCHECK([items count] <= 1);
// Only update if the item has changed. // Only update if the item has changed.
if ([items[0] isEqual:entriesStatusItem]) { if ([items[0] isEqual:entriesStatusItem]) {
return; return;
...@@ -674,9 +680,11 @@ const CGFloat kSeparatorInset = 10; ...@@ -674,9 +680,11 @@ const CGFloat kSeparatorInset = 10;
fromSectionWithIdentifier:kEntriesStatusSectionIdentifier]; fromSectionWithIdentifier:kEntriesStatusSectionIdentifier];
[self.collectionView deleteItemsAtIndexPaths:@[ indexPath ]]; [self.collectionView deleteItemsAtIndexPaths:@[ indexPath ]];
} }
if (entriesStatusItem) {
[self.collectionViewModel addItem:entriesStatusItem [self.collectionViewModel addItem:entriesStatusItem
toSectionWithIdentifier:kEntriesStatusSectionIdentifier]; toSectionWithIdentifier:kEntriesStatusSectionIdentifier];
[self.collectionView insertItemsAtIndexPaths:@[ indexPath ]]; [self.collectionView insertItemsAtIndexPaths:@[ indexPath ]];
}
} }
completion:nil]; completion:nil];
} }
......
...@@ -160,9 +160,8 @@ TEST_F(HistoryCollectionViewControllerTest, DeleteSingleEntry) { ...@@ -160,9 +160,8 @@ TEST_F(HistoryCollectionViewControllerTest, DeleteSingleEntry) {
scrollPosition:UICollectionViewScrollPositionNone]; scrollPosition:UICollectionViewScrollPositionNone];
[history_collection_view_controller_ deleteSelectedItemsFromHistory]; [history_collection_view_controller_ deleteSelectedItemsFromHistory];
// Expect header section with one item and one entries section with one item. // Expect header section and one entries section with one item.
EXPECT_EQ(2, [collection_view numberOfSections]); EXPECT_EQ(2, [collection_view numberOfSections]);
EXPECT_EQ(1, [collection_view numberOfItemsInSection:0]);
EXPECT_EQ(1, [collection_view numberOfItemsInSection:1]); EXPECT_EQ(1, [collection_view numberOfItemsInSection:1]);
} }
...@@ -189,7 +188,6 @@ TEST_F(HistoryCollectionViewControllerTest, DeleteMultipleEntries) { ...@@ -189,7 +188,6 @@ TEST_F(HistoryCollectionViewControllerTest, DeleteMultipleEntries) {
// Expect only the header section to remain. // Expect only the header section to remain.
EXPECT_EQ(1, [collection_view numberOfSections]); EXPECT_EQ(1, [collection_view numberOfSections]);
EXPECT_EQ(1, [collection_view numberOfItemsInSection:0]);
} }
// Tests that adding two entries to history from different days then deleting // Tests that adding two entries to history from different days then deleting
...@@ -217,5 +215,4 @@ TEST_F(HistoryCollectionViewControllerTest, DeleteMultipleSections) { ...@@ -217,5 +215,4 @@ TEST_F(HistoryCollectionViewControllerTest, DeleteMultipleSections) {
// Expect only the header section to remain. // Expect only the header section to remain.
EXPECT_EQ(1, [collection_view numberOfSections]); EXPECT_EQ(1, [collection_view numberOfSections]);
EXPECT_EQ(1, [collection_view numberOfItemsInSection:0]);
} }
...@@ -25,8 +25,6 @@ class GURL; ...@@ -25,8 +25,6 @@ class GURL;
@interface HistoryEntriesStatusItem : CollectionViewItem @interface HistoryEntriesStatusItem : CollectionViewItem
// YES if messages should be hidden. // YES if messages should be hidden.
@property(nonatomic, assign, getter=isHidden) BOOL hidden; @property(nonatomic, assign, getter=isHidden) BOOL hidden;
// YES if message for other forms of browsing data should be shown.
@property(nonatomic, assign) BOOL showsOtherBrowsingDataNotice;
// Delegate for HistoryEntriesStatusItem. Is notified when a link is pressed. // Delegate for HistoryEntriesStatusItem. Is notified when a link is pressed.
@property(nonatomic, weak) id<HistoryEntriesStatusItemDelegate> delegate; @property(nonatomic, weak) id<HistoryEntriesStatusItemDelegate> delegate;
@end @end
......
...@@ -44,16 +44,6 @@ ...@@ -44,16 +44,6 @@
@implementation HistoryEntriesStatusItem @implementation HistoryEntriesStatusItem
@synthesize delegate = _delegate; @synthesize delegate = _delegate;
@synthesize hidden = _hidden; @synthesize hidden = _hidden;
@synthesize showsOtherBrowsingDataNotice = _showsOtherBrowsingDataNotice;
- (instancetype)initWithType:(NSInteger)type {
self = [super initWithType:type];
if (self) {
_hidden = NO;
_showsOtherBrowsingDataNotice = NO;
}
return self;
}
- (Class)cellClass { - (Class)cellClass {
return [HistoryEntriesStatusCell class]; return [HistoryEntriesStatusCell class];
...@@ -62,7 +52,7 @@ ...@@ -62,7 +52,7 @@
- (void)configureCell:(HistoryEntriesStatusCell*)cell { - (void)configureCell:(HistoryEntriesStatusCell*)cell {
[super configureCell:cell]; [super configureCell:cell];
[cell setDelegate:self]; [cell setDelegate:self];
if (self.hidden || !self.showsOtherBrowsingDataNotice) { if (self.hidden) {
cell.textLabel.text = nil; cell.textLabel.text = nil;
} else { } else {
cell.textLabel.text = cell.textLabel.text =
...@@ -78,8 +68,7 @@ ...@@ -78,8 +68,7 @@
} }
- (BOOL)isEqualToHistoryEntriesStatusItem:(HistoryEntriesStatusItem*)object { - (BOOL)isEqualToHistoryEntriesStatusItem:(HistoryEntriesStatusItem*)object {
return self.hidden == object.hidden && return self.hidden == object.hidden;
self.showsOtherBrowsingDataNotice == self.showsOtherBrowsingDataNotice;
} }
- (BOOL)isEqual:(id)object { - (BOOL)isEqual:(id)object {
......
...@@ -50,39 +50,16 @@ namespace { ...@@ -50,39 +50,16 @@ namespace {
using HistoryEntriesStatusItemTest = PlatformTest; using HistoryEntriesStatusItemTest = PlatformTest;
// Tests that configuring a cell for HistoryEntriesStatusItem with hidden // Tests that configuring a cell for HistoryEntriesStatusItem with hidden
// property set to YES results in an empty label, regardless of what // property set to YES results in an empty label.
// showsOtherBrowsingDataNotice is set to.
TEST_F(HistoryEntriesStatusItemTest, TestHidden) { TEST_F(HistoryEntriesStatusItemTest, TestHidden) {
HistoryEntriesStatusItem* item = HistoryEntriesStatusItem* item =
[[HistoryEntriesStatusItem alloc] initWithType:0]; [[HistoryEntriesStatusItem alloc] initWithType:0];
item.hidden = YES; item.hidden = YES;
item.showsOtherBrowsingDataNotice = YES;
HistoryEntriesStatusCell* cell = [[HistoryEntriesStatusCell alloc] init]; HistoryEntriesStatusCell* cell = [[HistoryEntriesStatusCell alloc] init];
[item configureCell:cell]; [item configureCell:cell];
EXPECT_FALSE(cell.textLabel.text); EXPECT_FALSE(cell.textLabel.text);
} }
// Tests that configuring a cell for HistoryEntriesStatusItem with
// showsOtherBrowsingDataNotice set to YES adds other browsing
// data text to the label, while set to NO has no text.
TEST_F(HistoryEntriesStatusItemTest, TestOtherBrowsingDataNotice) {
HistoryEntriesStatusItem* item =
[[HistoryEntriesStatusItem alloc] initWithType:0];
HistoryEntriesStatusCell* cell = [[HistoryEntriesStatusCell alloc] init];
item.hidden = NO;
item.showsOtherBrowsingDataNotice = YES;
[item configureCell:cell];
NSString* label_text =
l10n_util::GetNSString(IDS_IOS_HISTORY_OTHER_FORMS_OF_HISTORY);
NSRange range;
label_text = ParseStringWithLink(label_text, &range);
EXPECT_NSEQ(label_text, cell.textLabel.text);
item.showsOtherBrowsingDataNotice = NO;
[item configureCell:cell];
EXPECT_NSEQ(nil, cell.textLabel.text);
}
// Tests that tapping on links on a configured cell invokes // Tests that tapping on links on a configured cell invokes
// the HistoryEntriesStatusItemDelegate method. // the HistoryEntriesStatusItemDelegate method.
TEST_F(HistoryEntriesStatusItemTest, TestDelegate) { TEST_F(HistoryEntriesStatusItemTest, TestDelegate) {
...@@ -93,7 +70,6 @@ TEST_F(HistoryEntriesStatusItemTest, TestDelegate) { ...@@ -93,7 +70,6 @@ TEST_F(HistoryEntriesStatusItemTest, TestDelegate) {
[[MockEntriesStatusItemDelegate alloc] init]; [[MockEntriesStatusItemDelegate alloc] init];
item.delegate = delegate; item.delegate = delegate;
item.hidden = NO; item.hidden = NO;
item.showsOtherBrowsingDataNotice = YES;
[item configureCell:cell]; [item configureCell:cell];
// Layout the cell so that links are drawn. // Layout the cell so that links are drawn.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment