New issue
Advanced search Search tips

Issue 869142 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Bookmark arrow is flipped (”>”) when in RTL.

Project Member Reported by shbarezer@chromium.org, Jul 30

Issue description

App 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

 
Labels: -Pri-2 M-69 Pri-1
Owner: rohitrao@chromium.org
Status: Assigned (was: Untriaged)
Need to confirm if this afflicts all collections.
Whoops realized the only other collection would be settings but that one is OK.
Cc: rohitrao@chromium.org
Owner: sczs@chromium.org
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.
Cc: pschaffner@chromium.org martijnb@chromium.org
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.
Do we use that same custom arrow and checkmark anywhere else in collections?
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)
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.
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.


Labels: Merge-TBD
Labels: Type-Bug
Labels: Q2
@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.
Status: Started (was: Assigned)
Because of #c3 I hadn't started yet. Sg, will go with the manual for 69
Project Member

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

Status: Fixed (was: Started)
Waiting for next canary for verification
Cc: kariahda@chromium.org
Labels: Merge-Request-69
Verified on Canary. kariahda@ PTAl for manual approval
Project Member

Comment 16 by sheriffbot@chromium.org, Aug 7

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

Comment 19 by bugdroid1@chromium.org, Aug 7

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

Verified in 69.0.3497.31 Beta,  iPhone X iOS 11.4.1, iPhone7 iOS 12
Looks good.

Sign in to add a comment