History: Remove populate and search animation on HistoryTableView |
||||||||||
Issue descriptionWhenever History loads you can see the tableView expanding from top to bottom, also whenever you search there's an animation when removing tableview rows. As an UI/UX request, we should try to get rid of these animations.
,
Jul 3
Thanks Sergio, The UIView approach sounds a bit too hacky, I'm concerned it might break other things. Would you want to try your reloudData approach? If this works, and performance is really poor, you could additionally try to only use this for the initial load like you are saying.
,
Jul 3
The reloadData approach means rewriting most of the update logic. And depending on the amount of History (how much you've scrolled), reloading everything at once might not going to be good for performance. We can definitely try, but I think we should wait until after shipping M-69, even if performance is good we might be introducing some bugs, and I think that risk might not be worth improving the animation at this point.
,
Jul 10
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6eadc4cb631d7726ba8ae001446bb945b0128c31 commit 6eadc4cb631d7726ba8ae001446bb945b0128c31 Author: sczs <sczs@chromium.org> Date: Wed Jul 25 16:58:14 2018 [ios] Disables History searchBar while editing. This is a preparation CL to remove animations when populating the history TableView and when performing a search. In order achieve that we need to reload all the content of the TableView instead of inserting/removing specific entries. This causes previously selected items to deselect when searching. For this reason there's no purpose to searching while editing is enabled, furthermore selecting the search bar will deselect selected items, which can be frustrating for the user. This CL disables the searchBar while the table is on Edit mode. The user can still search something and edit the results of the query, but it can't perform a new search query before exiting edit mode. This is the behavior of the legacy implementation. Screenshots: Enabled https://drive.google.com/open?id=1ZdplK968U4XgBE1cFSB9L4scblJwFeBN https://drive.google.com/open?id=1dH47e9tPHmDd2eUuOUVIt-W3Nay4v_A1 Disabled https://drive.google.com/open?id=1Gp4IXNawohaZnlwSnQTSJ0LLbvaFW6Iw https://drive.google.com/open?id=1aDPPjOd3DqX99vncP2wNXNHx_XKRyGhM Bug: 859736 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I705a936f8bb339ab29a1431d7d4d969d77337d71 Reviewed-on: https://chromium-review.googlesource.com/1148890 Reviewed-by: Rohit Rao <rohitrao@chromium.org> Commit-Queue: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/heads/master@{#577944} [modify] https://crrev.com/6eadc4cb631d7726ba8ae001446bb945b0128c31/ios/chrome/browser/ui/history/history_table_view_controller.mm
,
Jul 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/833ca80aa1c474feb9a432f020800560c6a6eaa3 commit 833ca80aa1c474feb9a432f020800560c6a6eaa3 Author: sczs <sczs@chromium.org> Date: Thu Jul 26 00:55:38 2018 [ios] Use reloadData on HistoryTableView inserts. After crbug.com/836562 was fixed it introduced crbug.com/860649 . In order to solve this we need to keep track of which indexPaths are being inserted and if 2 items are inserted in the same indexPath we need to manually shift the previous indexPath row, if not we will end up with 2 insertions in the same indexPath and get a consistency crash. Because of this and the fact that we want to get rid of the initial insertion and search animations ( crbug.com/859736 ) I decided to get rid of the historyInserter updates and just reloadData. We will still update individual rows for the status message and deletion when editing. Bug: 860649 , 859736 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ib8c7488110441727be767ead22903700927f69d1 Reviewed-on: https://chromium-review.googlesource.com/1141127 Commit-Queue: Sergio Collazos <sczs@chromium.org> Reviewed-by: Rohit Rao <rohitrao@chromium.org> Cr-Commit-Position: refs/heads/master@{#578152} [modify] https://crrev.com/833ca80aa1c474feb9a432f020800560c6a6eaa3/ios/chrome/browser/ui/history/history_table_view_controller.mm
,
Jul 26
,
Jul 26
,
Jul 26
Issue 868120 has been merged into this issue.
,
Jul 27
Confirmed that the desired behavior is in Canary.
,
Jul 27
Thanks Gabe. kariahda, could you PTAL?
,
Jul 28
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 30
Forgot to cc Kariah in my last comment, PTAL.
,
Jul 30
Manually approved as well!
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c7654277fc6e81e168ed6ac96e9a7cf5f5575ecd commit c7654277fc6e81e168ed6ac96e9a7cf5f5575ecd Author: sczs <sczs@chromium.org> Date: Mon Jul 30 18:17:36 2018 [ios] Disables History searchBar while editing. This is a preparation CL to remove animations when populating the history TableView and when performing a search. In order achieve that we need to reload all the content of the TableView instead of inserting/removing specific entries. This causes previously selected items to deselect when searching. For this reason there's no purpose to searching while editing is enabled, furthermore selecting the search bar will deselect selected items, which can be frustrating for the user. This CL disables the searchBar while the table is on Edit mode. The user can still search something and edit the results of the query, but it can't perform a new search query before exiting edit mode. This is the behavior of the legacy implementation. Screenshots: Enabled https://drive.google.com/open?id=1ZdplK968U4XgBE1cFSB9L4scblJwFeBN https://drive.google.com/open?id=1dH47e9tPHmDd2eUuOUVIt-W3Nay4v_A1 Disabled https://drive.google.com/open?id=1Gp4IXNawohaZnlwSnQTSJ0LLbvaFW6Iw https://drive.google.com/open?id=1aDPPjOd3DqX99vncP2wNXNHx_XKRyGhM TBR=sczs@chromium.org (cherry picked from commit 6eadc4cb631d7726ba8ae001446bb945b0128c31) Bug: 859736 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I705a936f8bb339ab29a1431d7d4d969d77337d71 Reviewed-on: https://chromium-review.googlesource.com/1148890 Reviewed-by: Rohit Rao <rohitrao@chromium.org> Commit-Queue: Sergio Collazos <sczs@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577944} Reviewed-on: https://chromium-review.googlesource.com/1155368 Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#219} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/c7654277fc6e81e168ed6ac96e9a7cf5f5575ecd/ios/chrome/browser/ui/history/history_table_view_controller.mm
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d98efc287b848f75f0475c0e16be5068de4c31af commit d98efc287b848f75f0475c0e16be5068de4c31af Author: sczs <sczs@chromium.org> Date: Mon Jul 30 18:22:46 2018 [ios] Use reloadData on HistoryTableView inserts. After crbug.com/836562 was fixed it introduced crbug.com/860649 . In order to solve this we need to keep track of which indexPaths are being inserted and if 2 items are inserted in the same indexPath we need to manually shift the previous indexPath row, if not we will end up with 2 insertions in the same indexPath and get a consistency crash. Because of this and the fact that we want to get rid of the initial insertion and search animations ( crbug.com/859736 ) I decided to get rid of the historyInserter updates and just reloadData. We will still update individual rows for the status message and deletion when editing. TBR=sczs@chromium.org (cherry picked from commit 833ca80aa1c474feb9a432f020800560c6a6eaa3) Bug: 860649 , 859736 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ib8c7488110441727be767ead22903700927f69d1 Reviewed-on: https://chromium-review.googlesource.com/1141127 Commit-Queue: Sergio Collazos <sczs@chromium.org> Reviewed-by: Rohit Rao <rohitrao@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#578152} Reviewed-on: https://chromium-review.googlesource.com/1155370 Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#220} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/d98efc287b848f75f0475c0e16be5068de4c31af/ios/chrome/browser/ui/history/history_table_view_controller.mm
,
Jul 31
History Search bar is disabled while in Editing mode. Verified on iPhoneX , iPad Pro iOS11.4.1, 12.0 beta#5
,
Aug 1
History Search bar is disabled while in Editing mode. App Version: 69.0.3497.22 beta Devices: iPhone 8 plus , iPad Pro iOS: 11.4.1, 12.0 beta #5 |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by sczs@chromium.org
, Jul 3Status: Started (was: Assigned)
I started to take a look at this. Since we're reloading/inserting/deleting each row individually performBatchUpdates introduces an animation even when didInsert/Delete is called using UITableViewRowAnimationNone, since this value uses default animations and not NO animations. I've tried to perform the updates using [UIView setAnimationsEnabled:NO]; or [UIView performWithoutAnimation:^{ and it works, and there's no animation. The problem is that sometimes (especially when searching) some instructions/animations seem to not complete. This causes some headers to never dissapear and keep floating on top of the tableView even if we delete our search terms or scroll the tableView. TableView needs to finish these animations in order to update itself, so using these 2 approaches might not be the best/safest. Conversely we could use reloadData, which updates the table with no animations and reloads everything on it. I think this might be the right approach, but performance is not great. Maybe we can use this for the initial load and then keep using the current tableUpdates.