Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2249933002:
WebUI cr-dialog: Cancel dialog on any 'popstate' event. (Closed)
|
Created:
4 years, 4 months ago by tommycli Modified:
4 years, 4 months ago Reviewers:
Dan Beam CC:
chromium-reviews, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org, calamity Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebUI cr-dialog: Cancel dialog on any 'popstate' event.
BUG=635861
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0f286c5b64829c1ad41018ed0b772b5496d7bf91
Cr-Commit-Position: refs/heads/master@{#412590}
Patch Set 1 #
Total comments: 1
Patch Set 2 : ensure we popstate before any route changes. #
Messages
Total messages: 24 (12 generated)
Description was changed from ========== WebUI cr-dialog: Cancel dialog on any 'popstate' event. BUG=635861 ========== to ========== WebUI cr-dialog: Cancel dialog on any 'popstate' event. BUG=635861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tommycli@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: PTAL. Before Demetrios left, we came to an agreement on this solution. We decided (offline) to make the cancellation automatic on any popstate for any cr-dialog. That seemed more correct and less work than making it opt-in. Thanks, Tommy
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Added PS2 which ensures we popstate before the route changes.
https://codereview.chromium.org/2249933002/diff/1/ui/webui/resources/cr_eleme... File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): https://codereview.chromium.org/2249933002/diff/1/ui/webui/resources/cr_eleme... ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:22: window.addEventListener('popstate', function() { tsergeant@/calamity@/mahmadi@: does this behavior work for your UI? if not (and we only want to do this in settings), tommycli@ could probably: 1) make this an option (i.e. <dialog is="cr-dialog" no-close-on-popstate>) 2) make a settings specific version of <cr-dialog> through inheritance or composition the important thing to note is that this will close every dialog when a 'popstate' event (generally: pressing back while using history.{push,replace}State()) https://developer.mozilla.org/en-US/docs/Web/Events/popstate
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/16 01:30:43, Dan Beam wrote: > https://codereview.chromium.org/2249933002/diff/1/ui/webui/resources/cr_eleme... > File ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js (right): > > https://codereview.chromium.org/2249933002/diff/1/ui/webui/resources/cr_eleme... > ui/webui/resources/cr_elements/cr_dialog/cr_dialog.js:22: > window.addEventListener('popstate', function() { > tsergeant@/calamity@/mahmadi@: does this behavior work for your UI? > > if not (and we only want to do this in settings), tommycli@ could probably: > > 1) make this an option (i.e. <dialog is="cr-dialog" no-close-on-popstate>) > 2) make a settings specific version of <cr-dialog> through inheritance or > composition > > the important thing to note is that this will close every dialog when a > 'popstate' event (generally: pressing back while using > history.{push,replace}State()) > > https://developer.mozilla.org/en-US/docs/Web/Events/popstate I don't see any problems, sounds good to me
BTW, I did some investigation. 'popstate' is only triggered by the Browser Forward and Back button (and window.history.back()), so I think we are good. https://developer.mozilla.org/en-US/docs/Web/Events/popstate
lgtm but maybe wait for mahmadi@ to answer
On 2016/08/16 20:58:19, Dan Beam wrote: > lgtm but maybe wait for mahmadi@ to answer thanks! I will wait for Moe to comment.
Description was changed from ========== WebUI cr-dialog: Cancel dialog on any 'popstate' event. BUG=635861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI cr-dialog: Cancel dialog on any 'popstate' event. BUG=635861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/08/16 20:59:31, tommycli wrote: > On 2016/08/16 20:58:19, Dan Beam wrote: > > lgtm but maybe wait for mahmadi@ to answer > > thanks! I will wait for Moe to comment. this looks good to me. User Manager doesn't user cr_dialog at the moment. I'm planning to refactor that in the future. But User Manager doesn't manipulate the history anyway.
On 2016/08/17 17:09:50, moe wrote: > On 2016/08/16 20:59:31, tommycli wrote: > > On 2016/08/16 20:58:19, Dan Beam wrote: > > > lgtm but maybe wait for mahmadi@ to answer > > > > thanks! I will wait for Moe to comment. > > this looks good to me. User Manager doesn't user cr_dialog at the moment. I'm > planning to refactor that in the future. But User Manager doesn't manipulate the > history anyway. woo hoo. I'm sending this in. tsargeant, moe: if either of you need to parameterize this behavior or confine it to Settings in the future, chat me up.
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== WebUI cr-dialog: Cancel dialog on any 'popstate' event. BUG=635861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI cr-dialog: Cancel dialog on any 'popstate' event. BUG=635861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== WebUI cr-dialog: Cancel dialog on any 'popstate' event. BUG=635861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI cr-dialog: Cancel dialog on any 'popstate' event. BUG=635861 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0f286c5b64829c1ad41018ed0b772b5496d7bf91 Cr-Commit-Position: refs/heads/master@{#412590} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0f286c5b64829c1ad41018ed0b772b5496d7bf91 Cr-Commit-Position: refs/heads/master@{#412590} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
