rohitrao@ PTAL
Blocking since this is an important a11y gesture that dismissed the screen.

I don't think this is a regression in the refresh UI.

The issue is that MDCAppBar has its own implementation of accessibilityPerformEscape which directly calls dismissViewControllerAnimated:completion: on the SettingsCollectionViewController.  This bypasses our settings-specific dismissal logic and leaves MainController thinking that settings is still open.

I can override dismissViewControllerAnimated:completion: in SettingsRootCollectionViewController to do nothing.  This might be a win overall, because any time we invoke that method directly, we've permanently broken Settings.  But it's risky, because there may be cases where we ignore legitimate attempts to dismiss Settings.  I don't think I'd want to make any changes here for M69.

Should we try to noop dismissViewControllerAnimated:completion: for M70? 

I can confirm that this bug exists with UIRefresh disabled, and I'm guessing it happens in previous milestones too.  Moving out of M69 and removing RBS.

 Issue 873640  has been merged into this issue.

FYI: Rohit, gambard@ filed  crbug.com/873640  where he mentions this wasn't happening on M68. In case you think this issue priority needs to be re assessed.

Yes, I can't reproduce on M68. For me, this is 69 RBS.

Yup, does not repro on M68.  On M69, it repros with and without UIRefresh enabled.

This regressed in M69 due to an intentional change in MDC: https://github.com/material-components/material-components-ios/issues/3656

I can implement my proposed workaround in SettingsRootCollectionViewController, but I would consider that risky for M69.

We could alternatively create an MDC branch and revert that MDC change out of M69.  That should be safe, but requires some work to create the branch.

Over to Kariah for creating the branch, if we decide we want to keep this as an M69 RBS.

Reverting the MDC change does seem to fix the issue.  Settings continues to respect the escape gesture, and it is possible to open Settings multiple times in a row after using the gesture.

For M70+, I can override -[SettingsRootCollectionViewController dismissViewControllerAnimated:completion:] to call [self.dispatcher closeSettingsUI].  My main fear there is accidentally introducing some sort of infinite loop, but from manual testing it seems ok.  I have a slight preference for reverting the MDC CL on 69, then landing the Settings fix for 70.

https://chromium-review.googlesource.com/c/chromium/src/+/1174559 for my proposed M70 fix.  I've asked the reviewers to evaluate it for M69 as well.

If we revert the MDC change, we lose the Z swipe gesture fix correct? https://github.com/material-components/material-components-ios/issues/3656

How important is it to revert the MDC fix as a short term solution to fix this bug vs. having the MDC fix in M69?

marq@, gambard@ Do you all feel the Gerritt CL is safe for M69? sczs@ has already given his recommendation in the CL: "My preference would be to revert the MDC change for M69 and land this for M70 only, though I'm not sure how much work is needed for that."

The Z swipe was working fine on M68 (before that MDC change), so reverting that change shouldn't cause us to lose the swipe gesture fix, at least that was my understanding.

I just feel that we know that reverting that change fixes the problem, and we've been shipping Bling like that for some milestones, conversely the CL that fixes might introduce issues (like any other new change). 

I can't find the issue I created, but I asked to have a branch for beta for mdc. I don't know if it is still active.

Do we know which MDC commit introduced the error?

Created bug for Git branch creation crbug/876019.

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

commit 7c90bff1bf0daf01c8496d1217c4484fed599740
Author: Rohit Rao <rohitrao@chromium.org>
Date: Wed Aug 22 15:58:28 2018

[ios] Updates DEPS for material_components_ios to use the 3497 branch.

BUG= 876019 , 865043 

Change-Id: I036272e43c272c0e821b6d619d734fcc313e4c86
Reviewed-on: https://chromium-review.googlesource.com/1185203
Reviewed-by: Michael Moss <mmoss@chromium.org>
Reviewed-by: Kariah Davis <kariahda@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#766}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/7c90bff1bf0daf01c8496d1217c4484fed599740/DEPS

Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 7c90bff1bf0daf01c8496d1217c4484fed599740 was merged to refs/branch-heads/3497 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!

Branch created. Marking as fixed.

Test team please verify on beta and canary.

This issue is no longer reproducible on 69.0.3497.58 Beta. However I am still able to reproduce this issue on 70.0.3531.0 Canary

Version: 
Chrome Beta 69.0.3497.58
Chrome Canary 70.0.3531.0
Device: iPhone 8
iOS: 12.0

Beta 69.0.3497.58
https://drive.google.com/open?id=1NJQLk71LCrGM2wJ3FxOBaYT9eAZE8tbG

Canary 70.0.3531.0
https://drive.google.com/open?id=1eqNVCjiuKwKyJ9GYi8dM7VVtjjTDBUac

Rohit, any insight as to why this could be? Can we ship M69 as is since it is fixed there even though it is not fixed on 70?

This is WAI since we created a new MDC branch as a workaround for M69 (without the code that broke this), but we still haven't decided on what's the best approach to permanently deal with this on M70 (https://chromium-review.googlesource.com/c/chromium/src/+/1174559)

Ah thanks sczs that makes sense. We did not point M70 to the new MDC branch so it shouldn't work there. Marking this verified.

Let's create another bug for 70 solution.

http://crbug/877162 is reported for M70. Please assign as needed.

TPM approved merge.