Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792663002:
MD Settings: Restore focus after closing dialogs, for certificate page. (Closed)
|
Created:
3 years, 8 months ago by dpapad Modified:
3 years, 8 months ago Reviewers:
tommycli 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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Restore focus after closing dialogs, for certificate page.
BUG=668313
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2792663002
Cr-Commit-Position: refs/heads/master@{#462161}
Committed: https://chromium.googlesource.com/chromium/src/+/d54ece4d6be085d618f0210036a7cb373e029c49
Patch Set 1 #Patch Set 2 : Update error dialog. #
Total comments: 11
Patch Set 3 : Add better explanation for |anchor| parameter in openDialog_ #Messages
Total messages: 28 (13 generated)
Description was changed from ========== MD Settings: Restore focus after closing dialogs, for certificate page. BUG=668313 ========== to ========== MD Settings: Restore focus after closing dialogs, for certificate page. BUG=668313 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...
dpapad@chromium.org changed reviewers: + tommycli@chromium.org
lgtm assuming below comment is uncontroversial https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_list.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_list.js:142: this.onRejected_.bind(this, anchor)); Nice partially applied function. https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); It looks like you don't actually need to store activeDialogAnchor_ as a property... since you can access the anchor parameter in the inner function too right?
https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); On 2017/04/04 22:42:29, tommycli wrote: > It looks like you don't actually need to store activeDialogAnchor_ as a > property... since you can access the anchor parameter in the inner function too > right? also, why only set it if (anchor)?
https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); On 2017/04/04 22:46:00, Dan Beam wrote: > On 2017/04/04 22:42:29, tommycli wrote: > > It looks like you don't actually need to store activeDialogAnchor_ as a > > property... since you can access the anchor parameter in the inner function > too > > right? > > also, why only set it if (anchor)? tommycli@: the difference is that openDialog_ (in theory) could be called multiple times and only the last |anchor| it's called with would be focused. but I'm not sure if that's intended.
https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); On 2017/04/04 22:47:21, Dan Beam wrote: > On 2017/04/04 22:46:00, Dan Beam wrote: > > On 2017/04/04 22:42:29, tommycli wrote: > > > It looks like you don't actually need to store activeDialogAnchor_ as a > > > property... since you can access the anchor parameter in the inner function > > too > > > right? > > > > also, why only set it if (anchor)? > > tommycli@: the difference is that openDialog_ (in theory) could be called > multiple times and only the last |anchor| it's called with would be focused. > but I'm not sure if that's intended. Ah yes, that behavior is different. Actually now I think using the parameter directly is more desirable, since then the last-closed dialog controls which element gets refocused afterwards.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_list.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_list.js:142: this.onRejected_.bind(this, anchor)); On 2017/04/04 at 22:42:29, tommycli wrote: > Nice partially applied function. Ack! https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); On 2017/04/04 at 22:56:49, tommycli wrote: > On 2017/04/04 22:47:21, Dan Beam wrote: > > On 2017/04/04 22:46:00, Dan Beam wrote: > > > On 2017/04/04 22:42:29, tommycli wrote: > > > > It looks like you don't actually need to store activeDialogAnchor_ as a > > > > property... since you can access the anchor parameter in the inner function > > > too > > > > right? > > > > > > also, why only set it if (anchor)? > > > > tommycli@: the difference is that openDialog_ (in theory) could be called > > multiple times and only the last |anchor| it's called with would be focused. > > but I'm not sure if that's intended. > > Ah yes, that behavior is different. > > Actually now I think using the parameter directly is more desirable, since then the last-closed dialog controls which element gets refocused afterwards. Certificates manager page is a pretty interesting case in terms of dialogs. I hope this will explain the questions above - 1st dialog opens, anchorElement_ is set. - An error occurs which causes the 1st dialog to be closed and to immediately request the error dialog to be shown, (example https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif...). - The close handler of the 1st dialog is executing, which focuses the |anchorElement_|, but we intentionally never set |anchorElement_| back to null. - The error-dialog is shown, and the |anchor| parameter in this call is null intentionally, which forces it to reuse the previously set |anchorElement_|, when the error dialog is closed. anchorElement_ needs to be stored on |this| so that it can be accessed across different calls of openDialog_(). Let me know if that addresses all the questions above.
https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); On 2017/04/04 23:15:42, dpapad wrote: > On 2017/04/04 at 22:56:49, tommycli wrote: > > On 2017/04/04 22:47:21, Dan Beam wrote: > > > On 2017/04/04 22:46:00, Dan Beam wrote: > > > > On 2017/04/04 22:42:29, tommycli wrote: > > > > > It looks like you don't actually need to store activeDialogAnchor_ as a > > > > > property... since you can access the anchor parameter in the inner > function > > > > too > > > > > right? > > > > > > > > also, why only set it if (anchor)? > > > > > > tommycli@: the difference is that openDialog_ (in theory) could be called > > > multiple times and only the last |anchor| it's called with would be focused. > > > > but I'm not sure if that's intended. > > > > Ah yes, that behavior is different. > > > > Actually now I think using the parameter directly is more desirable, since > then the last-closed dialog controls which element gets refocused afterwards. > > Certificates manager page is a pretty interesting case in terms of dialogs. I > hope this will explain the questions above > > - 1st dialog opens, anchorElement_ is set. > - An error occurs which causes the 1st dialog to be closed and to immediately > request the error dialog to be shown, (example > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif...). > - The close handler of the 1st dialog is executing, which focuses the > |anchorElement_|, but we intentionally never set |anchorElement_| back to null. > - The error-dialog is shown, and the |anchor| parameter in this call is null > intentionally, which forces it to reuse the previously set |anchorElement_|, > when the error dialog is closed. > > anchorElement_ needs to be stored on |this| so that it can be accessed across > different calls of openDialog_(). Let me know if that addresses all the > questions above. Wow that's really tricky. If the above is the intended behavior I recommend copy pasting that example scenario into the comment of this.activeDialogAnchor_. still lgtm.
https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); > Wow that's really tricky. If the above is the intended behavior I recommend copy pasting that example scenario into the comment of this.activeDialogAnchor_. > > still lgtm. I was hoping that the explanation given at the JSDoc comment for the possible null value of the |anchor| parameter would be sufficient. Can you verify that you still think a longer explanation would add value, on top of what is already stated in that comment?
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); On 2017/04/04 22:46:00, Dan Beam wrote: > On 2017/04/04 22:42:29, tommycli wrote: > > It looks like you don't actually need to store activeDialogAnchor_ as a > > property... since you can access the anchor parameter in the inner function > too > > right? > > also, why only set it if (anchor)? ping ^
https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); On 2017/04/05 03:31:36, Dan Beam wrote: > On 2017/04/04 22:46:00, Dan Beam wrote: > > On 2017/04/04 22:42:29, tommycli wrote: > > > It looks like you don't actually need to store activeDialogAnchor_ as a > > > property... since you can access the anchor parameter in the inner function > > too > > > right? > > > > also, why only set it if (anchor)? > > ping ^ dbeam: I'm assuming it's because if anchor is null, we want to preserve the old activeDialogAnchor_ rather than overwriting it... dpapad: Eh... I would say the desired behavior wasn't obvious to me, as a code reader. I think the behavior here is pretty subtle, and I'm not 100% sure it's worth it. But you guys think it's worth it, I would still support a longer comment. I don't see it as a blocking issue though. Just a matter of style / taste -- so I'll leave it to your discretion.
On 2017/04/05 at 03:31:36, dbeam wrote: > https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js (right): > > https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: this.activeDialogAnchor_.focus(); > On 2017/04/04 22:46:00, Dan Beam wrote: > > On 2017/04/04 22:42:29, tommycli wrote: > > > It looks like you don't actually need to store activeDialogAnchor_ as a > > > property... since you can access the anchor parameter in the inner function > > too > > > right? > > > > also, why only set it if (anchor)? > > ping ^ If a new anchor is specified, update activeDialogAnchor_. If |null| re-use previous activeDialogAnchor_. This is explained in the comment of |anchor| parameter as well as in my previous response.
On 2017/04/05 17:24:32, dpapad wrote: > On 2017/04/05 at 03:31:36, dbeam wrote: > > > https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... > > File > chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js > (right): > > > > > https://codereview.chromium.org/2792663002/diff/20001/chrome/browser/resource... > > > chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:179: > this.activeDialogAnchor_.focus(); > > On 2017/04/04 22:46:00, Dan Beam wrote: > > > On 2017/04/04 22:42:29, tommycli wrote: > > > > It looks like you don't actually need to store activeDialogAnchor_ as a > > > > property... since you can access the anchor parameter in the inner > function > > > too > > > > right? > > > > > > also, why only set it if (anchor)? > > > > ping ^ > > If a new anchor is specified, update activeDialogAnchor_. If |null| re-use > previous activeDialogAnchor_. This is explained in the comment of |anchor| > parameter as well as in my previous response. ok, go ahead then
Description was changed from ========== MD Settings: Restore focus after closing dialogs, for certificate page. BUG=668313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Restore focus after closing dialogs, for certificate page. BUG=668313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
> dpapad: Eh... I would say the desired behavior wasn't obvious to me, as a code reader. I think the behavior here is pretty subtle, and I'm not 100% sure it's worth it. But you guys think it's worth it, I would still support a longer comment. > > I don't see it as a blocking issue though. Just a matter of style / taste -- so I'll leave it to your discretion. Added a longer explanation for the |anchor| parameter.
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 Link to the patchset: https://codereview.chromium.org/2792663002/#ps40001 (title: "Add better explanation for |anchor| parameter in openDialog_")
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": 40001, "attempt_start_ts": 1491413890238550,
"parent_rev": "0dfe56315cccb02ce9411c8963012594843d84b0", "commit_rev":
"d54ece4d6be085d618f0210036a7cb373e029c49"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Restore focus after closing dialogs, for certificate page. BUG=668313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Restore focus after closing dialogs, for certificate page. BUG=668313 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2792663002 Cr-Commit-Position: refs/heads/master@{#462161} Committed: https://chromium.googlesource.com/chromium/src/+/d54ece4d6be085d618f0210036a7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d54ece4d6be085d618f0210036a7... |
