Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2239973002:
MD Settings: Use dom-if for the profile disconnect dialog. (Closed)
|
Created:
4 years, 4 months ago by dpapad Modified:
3 years, 8 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Use dom-if for the profile disconnect dialog.
BUG=597347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2239973002
Cr-Commit-Position: refs/heads/master@{#460165}
Committed: https://chromium.googlesource.com/chromium/src/+/f412dc8f3c6aacbc56e51b4b17d0f5f765208fb2
Patch Set 1 #Patch Set 2 : Fix test. #
Total comments: 2
Patch Set 3 : Fix test. #Patch Set 4 : Nit. #
Total comments: 7
Patch Set 5 : Address comments. #
Messages
Total messages: 36 (25 generated)
Description was changed from ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. BUG=633520 ========== to ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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...
Description was changed from ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. Perhaps this was caused by a bug of native <dialog> (retains focus after it has been closed, but still in the DOM). Lazy instantiating the dialog and aggressively removing it from the DOM when closed is better anyway. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. Perhaps this was caused by a bug of native <dialog> (retains focus after it has been closed, but still in the DOM). Lazy instantiating the dialog and aggressively removing it from the DOM when closed is better anyway. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. Perhaps this was caused by a bug of native <dialog> (retains focus after it has been closed, but still in the DOM). Lazy instantiating the dialog and aggressively removing it from the DOM when closed is considered better, and in this case also fixes a bug. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. Perhaps this was caused by a bug of native <dialog> (retains focus after it has been closed, but still in the DOM). Lazy instantiating the dialog and aggressively removing it from the DOM when closed is considered better, and in this case also fixes a bug. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. Perhaps this was caused by a bug of native <dialog> (retains focus after it has been closed, but still in the DOM). Lazy instantiating the dialog and aggressively removing it from the DOM when closed is considered better, and sidesteps the potential issue with <dialog>. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
I thought about moving the dialog to its own file, in accordance with most other dialogs in our codebase. Decided to leave that for a future (potential) cleanup.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2239973002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2239973002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:294: <div inner-h-t-m-l="[[getDisconnectExplanationHtml_(syncStatus.domain)]]"> wrap https://codereview.chromium.org/2239973002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:310: <paper-button id="disconnectManagedProfileConfirm" class="action-button" wrap
Not sure what happened to this CL. Looking at the date, I probably sent it for review and then went on vacation, then forgot about it. Anyway, I am going to revive this CL for performance reasons (why render a dialog that it is not requested by the user?).
Description was changed from ========== MD Settings: Use dom-if for the profile disconnect dialog. This fixes an issue where the page could not be scrolled after the disconnect dialog was closed. Perhaps this was caused by a bug of native <dialog> (retains focus after it has been closed, but still in the DOM). Lazy instantiating the dialog and aggressively removing it from the DOM when closed is considered better, and sidesteps the potential issue with <dialog>. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Use dom-if for the profile disconnect dialog. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Use dom-if for the profile disconnect dialog. BUG=633520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Use dom-if for the profile disconnect dialog. BUG=597347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by dpapad@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...
dpapad@chromium.org changed reviewers: + tommycli@chromium.org
Adding you as a reviewer since the existing LG was submitted many months ago, and I just had to resolve conflits with ToT. See comment#12 too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm but get tommycli to look as well https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:194: this.async(function() { is it possible that this.showingDisconnectDialog_ is now false?
lgtm sans some nits https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:84: showingDisconnectDialog_: Boolean, I think the other instances i've seen have been "showFooDialog_" rather than 'showing'. Also I believe this property may be at the wrong indent level. (within another property) https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:194: this.async(function() { On 2017/03/25 11:26:04, Dan Beam (slow) wrote: > is it possible that this.showingDisconnectDialog_ is now false? It does seem possible... We could recheck the variable... or we could also post async the close call below, and in that case, it should post and execute the 'show' and 'close' tasks in the correct order. https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:310: this.showingDisconnectDialog_ = false; Hmm... I'm not sure this call is even necessary. On the navigate to previous route, it should also close the dialog.
https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:84: showingDisconnectDialog_: Boolean, On 2017/03/27 at 16:11:23, tommycli wrote: > I think the other instances i've seen have been "showFooDialog_" rather than 'showing'. Also I believe this property may be at the wrong indent level. (within another property) Done and done. https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:194: this.async(function() { On 2017/03/27 at 16:11:23, tommycli wrote: > On 2017/03/25 11:26:04, Dan Beam (slow) wrote: > > is it possible that this.showingDisconnectDialog_ is now false? > > It does seem possible... > > We could recheck the variable... or we could also post async the close call below, and in that case, it should post and execute the 'show' and 'close' tasks in the correct order. I believe this is not possible. The boolean is set to false, only when the dialog is closed, and the dialog can't be closed until it has been opened first. Also, we use the same pattern in other places (flip boolean, async(), get a ref to the dialog and call showModal), and have never seen an issue. I'd rather not do anything more than necessary here, until it proves to be a problem. https://codereview.chromium.org/2239973002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:310: this.showingDisconnectDialog_ = false; On 2017/03/27 at 16:11:23, tommycli wrote: > Hmm... I'm not sure this call is even necessary. On the navigate to previous route, it should also close the dialog. The dialog will be closed from the navigation (happens in currentRouteChanged), but the boolean needs to be flipped to false to be consistent (note that we are using restamp with the dom-if). If I don't flip the boolean here, it will always stay "true" passed that point.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2239973002/#ps100001 (title: "Address comments.")
The CQ bit was unchecked by dpapad@chromium.org
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490722210767680,
"parent_rev": "8e630828534715e0983ffbb0c964fde29bf6afbb", "commit_rev":
"f412dc8f3c6aacbc56e51b4b17d0f5f765208fb2"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Use dom-if for the profile disconnect dialog. BUG=597347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Use dom-if for the profile disconnect dialog. BUG=597347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2239973002 Cr-Commit-Position: refs/heads/master@{#460165} Committed: https://chromium.googlesource.com/chromium/src/+/f412dc8f3c6aacbc56e51b4b17d0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/f412dc8f3c6aacbc56e51b4b17d0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
