Bookmark arrow is flipped (”>”) when in RTL. |
|||||||||||||
Issue descriptionApp Version: 69.0.3497.21 beta iOS Version: 12.0 beta#5, 11.4.1 Device: iPhoneX, iPhone7 plus URL: na Steps to reproduce: 1. Launch Google Chrome 2. Navigate to Bookmarks Observed results: Bookmark arrow is flipped ( “>”) when in RTL. Expected results: Bookmark arrow should be ( “<”) when in RTL. 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: Not tested Bug reproducible on Safari/Firefox: Firefox: , Safari: NA Link to video/image: https://drive.google.com/file/d/1p0WaubGUnS9qQkZo2rVdOyITI4NorORT/view Stable: https://drive.google.com/file/d/11V9HoeSeu9_vxrcdyEqYWTWm0zaoAMg8/view
,
Aug 1
Whoops realized the only other collection would be settings but that one is OK.
,
Aug 1
Can we use accessoryType here instead of a custom accessoryView? That would give us the RTL behavior for free. I can make the change once sczs confirms that we don't need to use accessoryView for any other reason.
,
Aug 1
Yeah we can, I believe we went with that custom accessoryView because it was the mocks were a little different than the default "arrow" asset, so if Pete or Martijn are OK then we should be good.
,
Aug 1
Do we use that same custom arrow and checkmark anywhere else in collections?
,
Aug 1
We use the same arrow rotated 90 degrees on the collapsable/expandable headers in RecentTabs. Its also used in one of the cells on EditBookmarks (the one that allows you to select the parent folder). Finally there's a TableViewAccessoryItem that uses it, I think we only use that one on Show Full History in recent tabs. (Apparently its also used on SettingsCollapsibleItem)
,
Aug 1
Ok, then I think we should keep the same images and just use a flip transform when we're in RTL. Otherwise we'll have inconsistent chevrons. Does that sound reasonable? We can try to move to system images for 70 or 71.
,
Aug 1
We could also change the 2 items that push something into the NavStack to use the default chevron (This one and the one for choosing the parent folder), maybe even the one from TableViewAccessoryItem. Those changes should be easy I believe. Also I guess this should also be happening on the other cells I'm talking about, right? So we might have to fix those too. The other cases are rotating the chevron, so we'll need to use a custom image anyways.
,
Aug 1
,
Aug 3
,
Aug 3
@sczs are you taking this one? Let's just manually add a transform on the imageview for now. I think that's the safest change for 69.
,
Aug 3
Because of #c3 I hadn't started yet. Sg, will go with the manual for 69
,
Aug 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba8109d2b154d93baba07f960772bb2d4a8129ad commit ba8109d2b154d93baba07f960772bb2d4a8129ad Author: sczs <sczs@chromium.org> Date: Fri Aug 03 22:19:45 2018 [ios] Rotates Collections TableViewCell chevrons on RTL Uses CGAffineTransformMakeRotation to do a 180 rotation of the collection cells accessoryView whenever a chevron is being used on RTL. Bug: 869142 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ic10ebc0067e34a7fe8d593d4aafeb25dca4f9e99 Reviewed-on: https://chromium-review.googlesource.com/1162428 Commit-Queue: Sergio Collazos <sczs@chromium.org> Reviewed-by: edchin <edchin@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#580674} [modify] https://crrev.com/ba8109d2b154d93baba07f960772bb2d4a8129ad/ios/chrome/browser/ui/bookmarks/cells/BUILD.gn [modify] https://crrev.com/ba8109d2b154d93baba07f960772bb2d4a8129ad/ios/chrome/browser/ui/bookmarks/cells/bookmark_folder_item.mm [modify] https://crrev.com/ba8109d2b154d93baba07f960772bb2d4a8129ad/ios/chrome/browser/ui/bookmarks/cells/bookmark_parent_folder_item.mm [modify] https://crrev.com/ba8109d2b154d93baba07f960772bb2d4a8129ad/ios/chrome/browser/ui/table_view/cells/BUILD.gn [modify] https://crrev.com/ba8109d2b154d93baba07f960772bb2d4a8129ad/ios/chrome/browser/ui/table_view/cells/table_view_accessory_item.mm
,
Aug 3
Waiting for next canary for verification
,
Aug 7
Verified on Canary. kariahda@ PTAl for manual approval
,
Aug 7
This bug requires manual review: Less than 24 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 7
,
Aug 7
Issue verified Version: Chrome Canary 70.0.3515.0 Device: iPhone 8 iOS: 11.4 Bookmark arrow is ( “<”) when in RTL. https://drive.google.com/open?id=1q0KGJYFm28LeNj7BWDP1ojGN_6kN82GR https://screenshot.googleplex.com/RYTBH3QLuKj
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/404d9a0bee46a103cfadc2d3b9ed8e394ea1ec64 commit 404d9a0bee46a103cfadc2d3b9ed8e394ea1ec64 Author: sczs <sczs@chromium.org> Date: Tue Aug 07 18:17:56 2018 [ios] Rotates Collections TableViewCell chevrons on RTL Uses CGAffineTransformMakeRotation to do a 180 rotation of the collection cells accessoryView whenever a chevron is being used on RTL. TBR=sczs@chromium.org (cherry picked from commit ba8109d2b154d93baba07f960772bb2d4a8129ad) Bug: 869142 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ic10ebc0067e34a7fe8d593d4aafeb25dca4f9e99 Reviewed-on: https://chromium-review.googlesource.com/1162428 Commit-Queue: Sergio Collazos <sczs@chromium.org> Reviewed-by: edchin <edchin@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#580674} Reviewed-on: https://chromium-review.googlesource.com/1165786 Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#470} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/404d9a0bee46a103cfadc2d3b9ed8e394ea1ec64/ios/chrome/browser/ui/bookmarks/cells/BUILD.gn [modify] https://crrev.com/404d9a0bee46a103cfadc2d3b9ed8e394ea1ec64/ios/chrome/browser/ui/bookmarks/cells/bookmark_folder_item.mm [modify] https://crrev.com/404d9a0bee46a103cfadc2d3b9ed8e394ea1ec64/ios/chrome/browser/ui/bookmarks/cells/bookmark_parent_folder_item.mm [modify] https://crrev.com/404d9a0bee46a103cfadc2d3b9ed8e394ea1ec64/ios/chrome/browser/ui/table_view/cells/BUILD.gn [modify] https://crrev.com/404d9a0bee46a103cfadc2d3b9ed8e394ea1ec64/ios/chrome/browser/ui/table_view/cells/table_view_accessory_item.mm
,
Aug 8
Verified in 69.0.3497.31 Beta, iPhone X iOS 11.4.1, iPhone7 iOS 12 Looks good. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by pschaffner@chromium.org
, Aug 1Owner: rohitrao@chromium.org
Status: Assigned (was: Untriaged)