New issue
Advanced search Search tips

Issue 877162 link

Starred by 1 user
Status: Verified
Owner:
thegreenfrog@chromium.org
Closed: Sep 6
Cc:
sczs@chromium.org
gambard@chromium.org
thegreenfrog@chromium.org
kariahda@chromium.org
rohitrao@chromium.org
ios-bugs-priority@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug
Team-Accessibility



Sign in to add a comment

[M70] Settings screen is not displayed from the second time if voice over is enabled.

Project Member Reported by srikanthg@chromium.org, Aug 23 
This issue to track the fix specifically for M70.
M69 the issue is fixed at https://bugs.chromium.org/p/chromium/issues/detail?id=865043

Device: iPhone6s, iPhoneX, iPad Pro
URL: n/a

Precondition: Enable Voice Over (Settings > General > Accessibility > VoiceOver > ON)

Steps to reproduce:
  1. Launch Google Chrome
  2. Tap Manu > Settings
  3.  Perform gesture "Two finger swipe - Z" to dismiss the settings.
  4. Tap Menu > Settings again

Observed results: No action performed. Settings Screen is not displayed.

Expected results: Settings screen should be launched again.

Number of times you were able to reproduce: 5/5
Bug reproducible after clean install: Yes
Bug reproducible after clearing cache and cookies: Yes

Link to video/image:  https://drive.google.com/file/d/1mTanEcIVS1BOsnYqtlG88wZXBxM3CRRB/view

Comment 1 by rakurati@chromium.org, Aug 24

Labels: MS-Voice-Search

Comment 2 by olivierrobin@chromium.org, Aug 24

Owner: rohitrao@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by rohitrao@chromium.org, Aug 29

Owner: gambard@chromium.org
gambard or sczs, could one of you take this over for M70?  We have a few options:

1) Revert the MDC CL on M70 as well.  (The worst option, IMO.)

2) Fix the test failures in https://chromium-review.googlesource.com/c/chromium/src/+/1174559 (tests are failing because we are dismissing settings recursively from its dismissal code, which is the infinite loop I was worried about.)

3) Override accessibilityPerformEscape to return NO.  The hard part is figuring out where to add this code, because we are not yet using MDCAppBarViewController.

Comment 4 by sczs@chromium.org, Aug 29

Cc: thegreenfrog@chromium.org
thegreenfrog@ is working on 3. Maybe we can get MDCAppBarViewController soon and we don't have to worry much about this? Chris, is there an ETA on this transition?

Comment 5 by thegreenfrog@chromium.org, Aug 29 

I was going to migrate CollectionViewController over to MDCAppBarViewController next! I can probably get that out this week.

Comment 6 by gambard@chromium.org, Aug 30

Cc: gambard@chromium.org
Owner: thegreenfrog@chromium.org
I think the best option is 3.
I am assigning the bugs to thegreenfrog@ as you are working on it. Don't hesitate to ask if there is anything you need.

Comment 7 by thegreenfrog@chromium.org, Aug 30

Status: Started (was: Assigned)
I'm going to move forward with option 3 then! I'm going to subclass MDCAppBarViewController and override accessibilityPerformEscape to return NO. Are we just going to disable dismissing with the Z gesture for now then?

Comment 8 by thegreenfrog@chromium.org, Aug 30 

Ok I understand whats going on now. We have proper logic to handle dismissals and but accessibilityPerformEscape in MDCAppBar is dismissing for us, leading MainController to never set SettingsNavigationController to nil.

Comment 9 by gambard@chromium.org, Aug 31 

Yes :)
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 31 

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

commit ce33f168b4215d2806f383f6b52080070407218e
Author: Chris Lu <thegreenfrog@chromium.org>
Date: Fri Aug 31 17:15:33 2018

[ios] Move CollectionViewController to MDCAppBarViewController

Screenshot: https://drive.google.com/open?id=1Y28L3fzEwVUZkk7BocBj0guUninmt2N8

Bug:  877162 
Change-Id: I7fcfd6f5207c8057430d25a21a5dc3dafb547698
Reviewed-on: https://chromium-review.googlesource.com/1196907
Commit-Queue: Chris Lu <thegreenfrog@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588069}
[modify] https://crrev.com/ce33f168b4215d2806f383f6b52080070407218e/ios/chrome/browser/ui/authentication/signin_account_selector_view_controller.mm
[modify] https://crrev.com/ce33f168b4215d2806f383f6b52080070407218e/ios/chrome/browser/ui/authentication/signin_confirmation_view_controller.mm
[modify] https://crrev.com/ce33f168b4215d2806f383f6b52080070407218e/ios/chrome/browser/ui/collection_view/collection_view_controller.h
[modify] https://crrev.com/ce33f168b4215d2806f383f6b52080070407218e/ios/chrome/browser/ui/collection_view/collection_view_controller.mm
[modify] https://crrev.com/ce33f168b4215d2806f383f6b52080070407218e/ios/chrome/browser/ui/collection_view/collection_view_controller_unittest.mm
[modify] https://crrev.com/ce33f168b4215d2806f383f6b52080070407218e/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
[modify] https://crrev.com/ce33f168b4215d2806f383f6b52080070407218e/ios/chrome/browser/ui/settings/settings_navigation_controller.mm
[modify] https://crrev.com/ce33f168b4215d2806f383f6b52080070407218e/ios/chrome/browser/ui/settings/settings_root_collection_view_controller.mm

Comment 11 by thegreenfrog@chromium.org, Sep 5

Labels: Merge-Request-70
kariah@ requesting that the two CLs tied to this bug be cherry picked into M-70
Project Member

Comment 13 by bugdroid1@chromium.org, Sep 5 

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

commit c64880bd2b18303f6bf3a256f9d35c242b16a753
Author: Chris Lu <thegreenfrog@chromium.org>
Date: Wed Sep 05 15:40:09 2018

[ios] Subclass MDCAppBarViewController to prevent it from dismissing on Accessibility Z-gesture in Settings

This is already managed well by SettingsNavigationController.

Video: https://drive.google.com/open?id=1P3kKX_m8_xnaS2lF58BKvaRL5tTbsuyw


Bug:  877162 , 874471
Change-Id: Ieb95fe427cece1d1fdb8f223bb16c0f0bbf05ce3
Reviewed-on: https://chromium-review.googlesource.com/1197411
Commit-Queue: Chris Lu <thegreenfrog@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588875}
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/authentication/BUILD.gn
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/authentication/signin_account_selector_view_controller.mm
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/authentication/signin_confirmation_view_controller.mm
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/collection_view/collection_view_controller.mm
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/material_components/BUILD.gn
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/material_components/app_bar_view_controller_presenting.h
[add] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/material_components/chrome_app_bar_view_controller.h
[add] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/material_components/chrome_app_bar_view_controller.m
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/reading_list/BUILD.gn
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/settings/settings_root_collection_view_controller.mm
[modify] https://crrev.com/c64880bd2b18303f6bf3a256f9d35c242b16a753/ios/chrome/browser/ui/table_view/chrome_table_view_controller.mm

Comment 14 by thegreenfrog@chromium.org, Sep 6

Status: Fixed (was: Started)
Verified on Canary on 9/6
Project Member

Comment 15 by sheriffbot@chromium.org, Sep 6

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 6

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59f5aabad16930ba1c156138a5e7705766a621b6

commit 59f5aabad16930ba1c156138a5e7705766a621b6
Author: Chris Lu <thegreenfrog@chromium.org>
Date: Thu Sep 06 23:29:34 2018

[ios] Move CollectionViewController to MDCAppBarViewController

Screenshot: https://drive.google.com/open?id=1Y28L3fzEwVUZkk7BocBj0guUninmt2N8

TBR=thegreenfrog@chromium.org

(cherry picked from commit ce33f168b4215d2806f383f6b52080070407218e)

Bug:  877162 
Change-Id: I7fcfd6f5207c8057430d25a21a5dc3dafb547698
Reviewed-on: https://chromium-review.googlesource.com/1196907
Commit-Queue: Chris Lu <thegreenfrog@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588069}
Reviewed-on: https://chromium-review.googlesource.com/1211672
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#115}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/59f5aabad16930ba1c156138a5e7705766a621b6/ios/chrome/browser/ui/authentication/signin_account_selector_view_controller.mm
[modify] https://crrev.com/59f5aabad16930ba1c156138a5e7705766a621b6/ios/chrome/browser/ui/authentication/signin_confirmation_view_controller.mm
[modify] https://crrev.com/59f5aabad16930ba1c156138a5e7705766a621b6/ios/chrome/browser/ui/collection_view/collection_view_controller.h
[modify] https://crrev.com/59f5aabad16930ba1c156138a5e7705766a621b6/ios/chrome/browser/ui/collection_view/collection_view_controller.mm
[modify] https://crrev.com/59f5aabad16930ba1c156138a5e7705766a621b6/ios/chrome/browser/ui/collection_view/collection_view_controller_unittest.mm
[modify] https://crrev.com/59f5aabad16930ba1c156138a5e7705766a621b6/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
[modify] https://crrev.com/59f5aabad16930ba1c156138a5e7705766a621b6/ios/chrome/browser/ui/settings/settings_navigation_controller.mm
[modify] https://crrev.com/59f5aabad16930ba1c156138a5e7705766a621b6/ios/chrome/browser/ui/settings/settings_root_collection_view_controller.mm
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 6 

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

commit ca72ec0d54fa3a461bf9b1a82f4317f7b1178517
Author: Chris Lu <thegreenfrog@chromium.org>
Date: Thu Sep 06 23:32:16 2018

[ios] Subclass MDCAppBarViewController to prevent it from dismissing on Accessibility Z-gesture in Settings

This is already managed well by SettingsNavigationController.

Video: https://drive.google.com/open?id=1P3kKX_m8_xnaS2lF58BKvaRL5tTbsuyw


Bug:  877162 , 874471
Change-Id: Ieb95fe427cece1d1fdb8f223bb16c0f0bbf05ce3
Reviewed-on: https://chromium-review.googlesource.com/1197411
Commit-Queue: Chris Lu <thegreenfrog@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588875}(cherry picked from commit c64880bd2b18303f6bf3a256f9d35c242b16a753)
Reviewed-on: https://chromium-review.googlesource.com/1211878
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#116}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/authentication/BUILD.gn
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/authentication/signin_account_selector_view_controller.mm
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/authentication/signin_confirmation_view_controller.mm
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/collection_view/collection_view_controller.mm
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/material_components/BUILD.gn
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/material_components/app_bar_view_controller_presenting.h
[add] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/material_components/chrome_app_bar_view_controller.h
[add] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/material_components/chrome_app_bar_view_controller.m
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/reading_list/BUILD.gn
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/settings/settings_root_collection_view_controller.mm
[modify] https://crrev.com/ca72ec0d54fa3a461bf9b1a82f4317f7b1178517/ios/chrome/browser/ui/table_view/chrome_table_view_controller.mm

Comment 18 by srikanthg@chromium.org, Sep 11

Status: Verified (was: Fixed)
Verified in M71.0.3549.0 canary
iOS: 12.0 11.4.1
iPhoneX, iPad Pro.

Comment 19 by subhashi...@chromium.org, Sep 12 

Verified in:

App Version: 70.0.3538.13 beta
iOS Version: 10.3.3, 11.4.1, 12.0 beta 12
Devices: iPhone 7 Plus, iPhone 6, iPhone 8 Plus

Settings screen is accessible on dismissing the screen with Two Finger ‘Z’ swipe gesture when voiceover is enabled
►

Sign in to add a comment