New issue
Advanced search Search tips

Issue 913538 link

Starred by 2 users
Status: Fixed
Owner:
gambard@chromium.org
Closed: Dec 13
Cc:
mrsuyi@chromium.org
sczs@chromium.org
gambard@chromium.org
edchin@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Header/Footer estimated height create empty space when there is no header

Project Member Reported by gambard@chromium.org, Dec 10 
When creating a table view with the "grouped" styling, if the "estimatedSectionFooterHeight" is set, when there is no footer in a section, an empty space of the height of this variable is displayed after each section.

I think we could either set empty items for each empty section (but I think it is a bad idea), or maybe replace -tableView:heightForFooterInSection: to return 0 or a very small value for the sections without footer.

-(CGFloat)tableView:(UITableView *)tableView heightForFooterInSection:(NSInteger)section {
  if([self.tableViewModel footerForSection:section]) {
    return UITableViewAutomaticDimension;
  } else {
    return 0;
  }
}

Same happen with headers.


WDYT? I am in favor of overriding tableView:heightForFooterInSection:, but the current space between section is actually determined with the header/footer estimated height. If I override to set it to 0, it is actually displayed as if there is no space between section (when the sections have no header/footer).

We could probably change this to return a default header/footer height to mimic those fake section spaces.

Comment 1 by sczs@chromium.org, Dec 10 

I remember dealing with this (or something similar before). Though I didn't fully understand the last part:

"If I override to set it to 0, it is actually displayed as if there is no space between section (when the sections have no header/footer)."

This going to be as problem with the proposed solution?, right?


Even though I'd really like to fix this, I'm not sure this is worth adding to the base class, especially since it seems simple enough that we can add it wherever we need it, I'd rather not override a widely used method especially since it will have side-effects. (Though I might need to better understand what issue -if any- this might cause)

Comment 2 by gambard@chromium.org, Dec 11

Owner: gambard@chromium.org
Status: Assigned (was: Available)

Comment 3 by gambard@chromium.org, Dec 11 

I have made a CL for this: https://chromium-review.googlesource.com/c/chromium/src/+/1371388

I have modified the return value when there is no header or footer to still have some spacing but smaller than the UIKit default.
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/897c9948f5004798930dea5003c0ca6e4e407480

commit 897c9948f5004798930dea5003c0ca6e4e407480
Author: Gauthier Ambard <gambard@chromium.org>
Date: Thu Dec 13 10:42:53 2018

[iOS] Update spacing in Settings' TableView

This CL changes the spacing in the Settings. In particular it changes:
- The estimated height for header/footer to avoid some constraints being
  broken.
- The spacing displayed when there is no header/footer in a section.
- The spacing around the link footers.

Bug:  913538 
Change-Id: Id634b2dc55ebed3daeffae78168a1f8fa08b4673
Reviewed-on: https://chromium-review.googlesource.com/c/1371388
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616264}
[modify] https://crrev.com/897c9948f5004798930dea5003c0ca6e4e407480/ios/chrome/browser/ui/settings/settings_root_table_view_controller.mm
[modify] https://crrev.com/897c9948f5004798930dea5003c0ca6e4e407480/ios/chrome/browser/ui/table_view/cells/table_view_link_header_footer_item.mm

Comment 5 by gambard@chromium.org, Dec 13

Status: Fixed (was: Assigned)
►

Sign in to add a comment