[iOS] Reading List Bug: Animation is not smooth when reading list items are marked read to unread |
||||||||||||
Issue descriptionApp Version: 69.0.3497.15 beta iOS Version: 11.4.1, 12 beta 4, 10.3.3 Device: iPhone X, iPad 2018, iPhone 7 URL: Precondition: Have 2 or more reading list items saved. Steps to reproduce: 1. Launch chrome beta. 2. Go to Menu --> Reading List --> Edit --> Mark All --> Mark All Read 3. Edit --> Mark All --> Mark All Unread. Observed results: Notice that the animation of converting read items to unread and unread items to read are not smooth at Step 2 and Step 3 Expected results: Animation should be smooth on reading list items. Number of times you were able to reproduce: 5/5 Bug reproducible after clean install: Yes Bug reproducible after clearing cache and cookies: Yes Bug reproducible on Chrome Mobile on Android: NA Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA Bug reproducible on current stable build (App Version, iOS Version): NA on M68 Bug reproducible on the current beta channel build (App Version, iOS Version): Yes on M69 Link to video/image: https://drive.google.com/file/d/1NDZUFLg6-IZjObjjV1HzvN4owdfzWTj9/view?usp=sharing
,
Jul 27
,
Jul 30
Taking this. It looks like the Automatic animation from TableView is inserting the new section (This pushes the section header down the table), and then removes the section (animating the section header that was pushed down to the right. I'll check if there's a quick config we can change to improve this.
,
Jul 30
,
Jul 30
Sent https://chromium-review.googlesource.com/c/chromium/src/+/1155923 for review. I think we can improve the animation a little bit with no risk by changing the animation type. Here's a video of what I'm proposing: https://drive.google.com/open?id=1jf1n56P4apUCHjC6ZqZlLe0mCEORsIzG I think its better than what we have now, but maybe can be improved more by playing around with performBatchTableViewUpdates, but that's risky and I wouldn't want to do that for M69 since it can lead to crashes. + Martijn, PTAL at the video in case you feel the new animation is not better. The CL fixes some constraints that were breaking when animating, because of this I think this should remain a P1.
,
Jul 31
Thanks Sergio I think what you proposed is definitely better and acceptable for M69.
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb5e1734c257ce47c84cdda7ed8c5b9ba126f622 commit eb5e1734c257ce47c84cdda7ed8c5b9ba126f622 Author: sczs <sczs@chromium.org> Date: Tue Jul 31 18:05:08 2018 [ios] Improves ReadingList header insert/delete animation - Changes ReadingListTableVC section rowAnimation to UITableViewRowAnimationMiddle and UITableViewRowAnimationFade. - Constraints were breaking while animating, this because of a known bug that was fixed in other headers (crbug.com/854117). In order to fix this all the padding constraints priority was lowered. Video: https://drive.google.com/open?id=1jf1n56P4apUCHjC6ZqZlLe0mCEORsIzG Bug: 868228 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I5580fe7377f16070fc43b1bc283754d4b00d60ef Reviewed-on: https://chromium-review.googlesource.com/1155923 Commit-Queue: Sergio Collazos <sczs@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Commit-Position: refs/heads/master@{#579479} [modify] https://crrev.com/eb5e1734c257ce47c84cdda7ed8c5b9ba126f622/ios/chrome/browser/ui/reading_list/reading_list_table_view_controller.mm [modify] https://crrev.com/eb5e1734c257ce47c84cdda7ed8c5b9ba126f622/ios/chrome/browser/ui/table_view/cells/table_view_text_header_footer_item.mm
,
Jul 31
,
Aug 1
,
Aug 7
Verified in: App Version: 70.0.3515.0 canary Devices: 10.3.3, 11.4.1, 12.0 beta 6 iOS Versions: iPhone 7 Plus, iPhone 8 Plus, iPad Air New Animation looks good for reading list. Video: https://drive.google.com/open?id=1n24IAPNAYjj2VmM1HztjHiDw-hIUSExy
,
Aug 8
,
Aug 8
This bug requires manual review: Less than 23 days to go before AppStore submit on M69 Please contact the 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
,
Aug 8
Approved.
,
Aug 13
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 14
Please merge asap.
,
Aug 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e518b1908673739700edba52c90fdc482970679c commit e518b1908673739700edba52c90fdc482970679c Author: sczs <sczs@chromium.org> Date: Tue Aug 14 20:40:41 2018 [ios] Improves ReadingList header insert/delete animation - Changes ReadingListTableVC section rowAnimation to UITableViewRowAnimationMiddle and UITableViewRowAnimationFade. - Constraints were breaking while animating, this because of a known bug that was fixed in other headers (crbug.com/854117). In order to fix this all the padding constraints priority was lowered. Video: https://drive.google.com/open?id=1jf1n56P4apUCHjC6ZqZlLe0mCEORsIzG TBR=sczs@chromium.org (cherry picked from commit eb5e1734c257ce47c84cdda7ed8c5b9ba126f622) Bug: 868228 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I5580fe7377f16070fc43b1bc283754d4b00d60ef Reviewed-on: https://chromium-review.googlesource.com/1155923 Commit-Queue: Sergio Collazos <sczs@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#579479} Reviewed-on: https://chromium-review.googlesource.com/1175085 Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#630} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/e518b1908673739700edba52c90fdc482970679c/ios/chrome/browser/ui/reading_list/reading_list_table_view_controller.mm [modify] https://crrev.com/e518b1908673739700edba52c90fdc482970679c/ios/chrome/browser/ui/table_view/cells/table_view_text_header_footer_item.mm
,
Aug 22
Issue verified Version: Chrome Beta 69.0.3497.53 Device: iPhone 5C, 8 iOS: 11.4, 12.0 Animation verified as per comment 10 https://drive.google.com/open?id=17Y9Juof-V4VeadasODv1pnknHMa5PPjD |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by marq@chromium.org
, Jul 27