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

Issue 635861 link

Starred by 3 users
Status: Fixed
Owner:
tommycli@chromium.org
Closed: Aug 2016
Cc:
calamity@chromium.org
nyerramilli@chromium.org
dbeam@chromium.org
dpa...@chromium.org
█ dschuyler@chromium.org
█ ranjitkan@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Regression



Sign in to add a comment

MD Settings: Dialogs behave badly when user clicks Back button.

Reported by rk...@etouch.net, Aug 9 2016 
Chrome Version: 54.0.2824.0 Revision facabd3224aecbcab4bea9daadad31c67488d78c-refs/heads/master@{#410520}(32/64 bit) 
OS: Windows(7,8,10), Mac(10.10.5,10.11.4). Linux(14.04 LTS)

What steps will reproduce the problem?
(1) Launch chrome, navigate to chrome://md-settings.
(2) Click on 'Reset' button under 'Advanced' section Reset overlay gets opened).
(3) Click on back navigation button and observed.

Actual: After clicking on back navigation button, 'Reset overlay' is opened even if omnibox url gets changed.

Expected: After clicking on back navigation button, 'Reset overlay' should gets closed.

This is a regression issue, broken in 'M-53' below is bisect info:

Good Build: 53.0.2761.0
Bad Build: 53.0.2763.0

Narrow Bisect:
https://chromium.googlesource.com/chromium/src/+log/42947e24de37b28e80c8456fdf36460c5296db63..f97dc9cd33155967950da6b4ed68da34744fc6e7?pretty=fuller&n=100

Suspecting: r398434 ?
Actual_Navigation.mp4.ns.mp4
0 bytes View Download

Comment 1 by dschuyler@chromium.org, Aug 12 2016

Owner: tommycli@chromium.org
For me, the Reset dialog appears to go away, but then
no further input is allowed in the page I went back to
(as if it's still in the background behind the dialog
and not accepting any events/input).

Tommy, do you think this is related to the routing at
all - or would you prefer that I looked into it?

Comment 2 by tommycli@chromium.org, Aug 12 2016

Cc: dschuyler@chromium.org dbeam@chromium.org dpa...@chromium.org
Summary: MD Settings: Dialogs behave badly when user clicks Back button. (was: Regression: [MD Settings] 'Reset overlay' is seen opened even if omnibox url gets changed on back navigation.)
I believe every single dialog (other than Clear Browsing Data) has this bad behavior.

We should probably either:

1. Modify cr-dialog to listen to 'popstate' events and auto-close on them.

2. Make every cr-dialog a settings-dialog that is a RouteObserver and closes on any route change.

Probably #1 is better. dbeam / dpapad thoughts?

Comment 3 by dpa...@chromium.org, Aug 12 2016 

Closing via cancel() when the user clicks the back button sounds reasonable behavior.

Regarding the implementation, suggestion 1, sounds better than 2. I think if we were to modify cr-dialog, we should make the "auto-close on popstate" behavior opt-in instead of default.

Alternatively, we could introduce some kind of "DialogManager", that knows at any given time if any dialog is open (perhaps we need to introduce on "open" event on cr-dialog for this). It listens for route changes and closes the displayed dialog, if any.

Comment 4 by tommycli@chromium.org, Aug 12 2016 

Regarding opt-in: I can't imagine a scenario where you wouldn't want to close a cr-dialog on popstate. That seems to be the more exceptional case.

Comment 5 by calamity@chromium.org, Aug 16 2016

Cc: calamity@chromium.org
 Issue 633921  has been merged into this issue.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 17 2016 

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

commit 0f286c5b64829c1ad41018ed0b772b5496d7bf91
Author: tommycli <tommycli@chromium.org>
Date: Wed Aug 17 18:19:56 2016

WebUI cr-dialog: Cancel dialog on any 'popstate' event.

BUG= 635861 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2249933002
Cr-Commit-Position: refs/heads/master@{#412590}

[modify] https://crrev.com/0f286c5b64829c1ad41018ed0b772b5496d7bf91/chrome/browser/resources/settings/route.js
[modify] https://crrev.com/0f286c5b64829c1ad41018ed0b772b5496d7bf91/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js

Comment 7 by tommycli@chromium.org, Aug 17 2016

Status: Fixed (was: Assigned)

Comment 8 by msrchandra@chromium.org, Aug 19 2016

Labels: TE-Verified-M54 TE-Verified-54.0.2832.2
Checked the issue on Latest Dev# 54.0.2832.2 on Windows, Mac and Linux and is working as intended.
Hence adding TE-Verified-Labels.
Thank You.
►

Sign in to add a comment