New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 909724 link

Starred by 1 user
Status: Verified
Owner:
justincohen@chromium.org
Closed: Nov 29
Cc:
eugene...@chromium.org
█ noyau@chromium.org
pinkerton@chromium.org
olivierrobin@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Feature



Sign in to add a comment

Consider removing swipe forward gesture to open next reading list item

Project Member Reported by justincohen@chromium.org, Nov 28 
I believe this was added in https://chromereviews.googleplex.com/486917013 by olivierrobin@

Per discussion with mardini@, this is often triggered by accident, marking unread reading list items as read.

olivierrobin@ do we have any metrics on this feature?

Comment 1 by olivierrobin@chromium.org, Nov 28 

I don't think we have metrics for this.

Comment 2 by mard...@chromium.org, Nov 28 

Éric: I know you and I discussed this a lot during the implementation of V1 Reading List and we added it based on your request. I honestly didn't find it useful and on the contrary, found that it is forcing me to go and mark items as unread since I trigger it accidentally all the time. Happy to chat more in person tomorrow.

Comment 3 by mard...@chromium.org, Nov 29

Cc: pinkerton@chromium.org eugene...@chromium.org
Owner: justincohen@chromium.org
Discussed with Éric offline and we're ok to remove it.

Adding Eugene and Pink as FYI.

Comment 4 by justincohen@chromium.org, Nov 29

Status: Started (was: Assigned)
crrev.com/c/1354414
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b74b9644d1301e77bbd00106bf31c46c189137a4

commit b74b9644d1301e77bbd00106bf31c46c189137a4
Author: Justin Cohen <justincohen@google.com>
Date: Thu Nov 29 18:49:07 2018

[ios] Remove forward swipe for reading list.

Bug:  909724 
Change-Id: I24c3341ca5245d599ce43cba8b17c164e38e3618
Reviewed-on: https://chromium-review.googlesource.com/c/1354414
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612291}
[modify] https://crrev.com/b74b9644d1301e77bbd00106bf31c46c189137a4/ios/chrome/browser/ui/reading_list/BUILD.gn
[delete] https://crrev.com/343df2ec93916fd2bee51d60405e9c97b90ad928/ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.h
[delete] https://crrev.com/343df2ec93916fd2bee51d60405e9c97b90ad928/ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.mm
[modify] https://crrev.com/b74b9644d1301e77bbd00106bf31c46c189137a4/ios/chrome/browser/ui/reading_list/resources/BUILD.gn
[delete] https://crrev.com/343df2ec93916fd2bee51d60405e9c97b90ad928/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/Contents.json
[delete] https://crrev.com/343df2ec93916fd2bee51d60405e9c97b90ad928/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe.png
[delete] https://crrev.com/343df2ec93916fd2bee51d60405e9c97b90ad928/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe@2x.png
[delete] https://crrev.com/343df2ec93916fd2bee51d60405e9c97b90ad928/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe@3x.png
[modify] https://crrev.com/b74b9644d1301e77bbd00106bf31c46c189137a4/ios/chrome/browser/ui/side_swipe/BUILD.gn
[delete] https://crrev.com/343df2ec93916fd2bee51d60405e9c97b90ad928/ios/chrome/browser/ui/side_swipe/history_side_swipe_provider.h
[delete] https://crrev.com/343df2ec93916fd2bee51d60405e9c97b90ad928/ios/chrome/browser/ui/side_swipe/history_side_swipe_provider.mm
[modify] https://crrev.com/b74b9644d1301e77bbd00106bf31c46c189137a4/ios/chrome/browser/ui/side_swipe/side_swipe_controller.h
[modify] https://crrev.com/b74b9644d1301e77bbd00106bf31c46c189137a4/ios/chrome/browser/ui/side_swipe/side_swipe_controller.mm
[modify] https://crrev.com/b74b9644d1301e77bbd00106bf31c46c189137a4/ios/chrome/browser/ui/side_swipe/side_swipe_navigation_view.h
[modify] https://crrev.com/b74b9644d1301e77bbd00106bf31c46c189137a4/ios/chrome/browser/ui/side_swipe/side_swipe_navigation_view.mm

Comment 6 by justincohen@chromium.org, Nov 29

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53

commit 27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Fri Nov 30 05:01:11 2018

Revert "[ios] Remove forward swipe for reading list."

This reverts commit b74b9644d1301e77bbd00106bf31c46c189137a4.

Reason for revert: ios_chrome_web_egtests failing

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-slimnav/465

../../ios/chrome/browser/web/tab_order_egtest.mm:74: error: -[TabOrderTestCase testChildTabOrdering] : Exception: ActionFailedException

Exception Name: ActionFailedException
Exception Reason: An action failed. Please refer to the error trace below.
Exception with Action: {
  "Action Name":  "Locate element bounds",
  "Element Matcher":  "web view in web state"
}

Exception Details: Error Trace: [
  {
    "Description":  "Couldn't locate a bounding rect for element with ID normal-link; either it isn't there or it has no area.",
    "Error Domain":  "com.google.earlgrey.ElementInteractionErrorDomain",
    "Error Code":  "2"
  }
]

Bundle ID: org.chromium.gtest.generic-unit-test

Original change's description:
> [ios] Remove forward swipe for reading list.
> 
> Bug:  909724 
> Change-Id: I24c3341ca5245d599ce43cba8b17c164e38e3618
> Reviewed-on: https://chromium-review.googlesource.com/c/1354414
> Commit-Queue: Justin Cohen <justincohen@chromium.org>
> Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612291}

TBR=justincohen@chromium.org,olivierrobin@chromium.org

Change-Id: I3b9b6b9b2a61eaaa11924b5b3e189034831c59d0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  909724 
Reviewed-on: https://chromium-review.googlesource.com/c/1356176
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612568}
[modify] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/BUILD.gn
[add] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.h
[add] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.mm
[modify] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/BUILD.gn
[add] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/Contents.json
[add] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe.png
[add] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe@2x.png
[add] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe@3x.png
[modify] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/BUILD.gn
[add] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/history_side_swipe_provider.h
[add] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/history_side_swipe_provider.mm
[modify] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/side_swipe_controller.h
[modify] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/side_swipe_controller.mm
[modify] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/side_swipe_navigation_view.h
[modify] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/side_swipe_navigation_view.mm
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/76b52d89509bb8a8755beb8bc7cc41838c0420f9

commit 76b52d89509bb8a8755beb8bc7cc41838c0420f9
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Fri Nov 30 05:03:55 2018

Reland "[ios] Remove forward swipe for reading list."

This reverts commit 27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53.

Reason for revert: Seems like the fix is about to land. So reverting
this revert. Sorry for the noise.

Original change's description:
> Revert "[ios] Remove forward swipe for reading list."
> 
> This reverts commit b74b9644d1301e77bbd00106bf31c46c189137a4.
> 
> Reason for revert: ios_chrome_web_egtests failing
> 
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-slimnav/465
> 
> ../../ios/chrome/browser/web/tab_order_egtest.mm:74: error: -[TabOrderTestCase testChildTabOrdering] : Exception: ActionFailedException
> 
> Exception Name: ActionFailedException
> Exception Reason: An action failed. Please refer to the error trace below.
> Exception with Action: {
>   "Action Name":  "Locate element bounds",
>   "Element Matcher":  "web view in web state"
> }
> 
> Exception Details: Error Trace: [
>   {
>     "Description":  "Couldn't locate a bounding rect for element with ID normal-link; either it isn't there or it has no area.",
>     "Error Domain":  "com.google.earlgrey.ElementInteractionErrorDomain",
>     "Error Code":  "2"
>   }
> ]
> 
> Bundle ID: org.chromium.gtest.generic-unit-test
> 
> Original change's description:
> > [ios] Remove forward swipe for reading list.
> > 
> > Bug:  909724 
> > Change-Id: I24c3341ca5245d599ce43cba8b17c164e38e3618
> > Reviewed-on: https://chromium-review.googlesource.com/c/1354414
> > Commit-Queue: Justin Cohen <justincohen@chromium.org>
> > Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#612291}
> 
> TBR=justincohen@chromium.org,olivierrobin@chromium.org
> 
> Change-Id: I3b9b6b9b2a61eaaa11924b5b3e189034831c59d0
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  909724 
> Reviewed-on: https://chromium-review.googlesource.com/c/1356176
> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612568}

TBR=justincohen@chromium.org,ortuno@chromium.org,olivierrobin@chromium.org

Change-Id: I8174acbe3d36adb70fbf19a12454373c1b247ea4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  909724 
Reviewed-on: https://chromium-review.googlesource.com/c/1356178
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612569}
[modify] https://crrev.com/76b52d89509bb8a8755beb8bc7cc41838c0420f9/ios/chrome/browser/ui/reading_list/BUILD.gn
[delete] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.h
[delete] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/reading_list_side_swipe_provider.mm
[modify] https://crrev.com/76b52d89509bb8a8755beb8bc7cc41838c0420f9/ios/chrome/browser/ui/reading_list/resources/BUILD.gn
[delete] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/Contents.json
[delete] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe.png
[delete] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe@2x.png
[delete] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/reading_list/resources/reading_list_side_swipe.imageset/reading_list_side_swipe@3x.png
[modify] https://crrev.com/76b52d89509bb8a8755beb8bc7cc41838c0420f9/ios/chrome/browser/ui/side_swipe/BUILD.gn
[delete] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/history_side_swipe_provider.h
[delete] https://crrev.com/27f55bc8ec4c7780737dabb8fe7cb9c52fa66c53/ios/chrome/browser/ui/side_swipe/history_side_swipe_provider.mm
[modify] https://crrev.com/76b52d89509bb8a8755beb8bc7cc41838c0420f9/ios/chrome/browser/ui/side_swipe/side_swipe_controller.h
[modify] https://crrev.com/76b52d89509bb8a8755beb8bc7cc41838c0420f9/ios/chrome/browser/ui/side_swipe/side_swipe_controller.mm
[modify] https://crrev.com/76b52d89509bb8a8755beb8bc7cc41838c0420f9/ios/chrome/browser/ui/side_swipe/side_swipe_navigation_view.h
[modify] https://crrev.com/76b52d89509bb8a8755beb8bc7cc41838c0420f9/ios/chrome/browser/ui/side_swipe/side_swipe_navigation_view.mm

Comment 9 by rakurati@chromium.org, Dec 4 

Verified in 73.0.3629.0 Canary in iPhone 8plus(iOS 11.4.1), iPad Air(iOS 12.0.1)

Swipe forward gesture to open next reading list item is now removed 

Tested in following combinations

1. Swipe forward from NTP page
2. Swipe forward from Offline reading list page
3. Swipe forward from Tab page which is loaded with webpage

None of the above cases displays reading list. 

justincohen@ could you please confirm this is the expected behavior

Link to video:
https://drive.google.com/file/d/1lC2zcSDcYStc6x-T-xLM0n01fhcZWKIi/view?usp=sharing

Comment 10 by justincohen@chromium.org, Dec 4 

rakurati@ Looks good to me.

Comment 11 by srikan...@google.com, Dec 4

Status: Verified (was: Fixed)
Updating status as per comments #9 and #10.
►

Sign in to add a comment