New issue
Advanced search Search tips

Issue 868228 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug
Q2



Sign in to add a comment

[iOS] Reading List Bug: Animation is not smooth when reading list items are marked read to unread

Project Member Reported by vbarig...@chromium.org, Jul 27

Issue description

App 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

 
Labels: M-69 Q2
Labels: S-Reading-List-UI
Cc: -sczs@chromium.org kkhorimoto@chromium.org
Owner: sczs@chromium.org
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.

Status: Started (was: Assigned)
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.
Thanks Sergio I think what you proposed is definitely better and acceptable for M69. 
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Labels: Merge-TBD
Status: Verified (was: Fixed)
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
Labels: -Merge-TBD Merge-Request-69
Project Member

Comment 12 by sheriffbot@chromium.org, Aug 8

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
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
Labels: -Merge-Review-69 Merge-Approved-69
Approved.
Project Member

Comment 14 by sheriffbot@chromium.org, Aug 13

Cc: kariahda@chromium.org marq@chromium.org
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
Please merge asap.
Project Member

Comment 16 by bugdroid1@chromium.org, Aug 14

Labels: -merge-approved-69 merge-merged-3497
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

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