Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899533002:
[MD settings] move <dialog> out of content (Closed)
|
Created:
3 years, 7 months ago by dschuyler Modified:
3 years, 7 months ago Reviewers:
hcarmona CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] move <dialog> out of content
This CL moves the "Do not track" dialog out of the page content where it
was picking up css erroneously.
BUG=724394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2899533002
Cr-Commit-Position: refs/heads/master@{#474481}
Committed: https://chromium.googlesource.com/chromium/src/+/516d1b3d84dece163efa12412b76f10a0f4ba5b1
Patch Set 1 #
Total comments: 2
Patch Set 2 : focusWithoutInk #
Total comments: 7
Patch Set 3 : review changes #
Messages
Total messages: 29 (21 generated)
Description was changed from ========== [MD settings] move <dialog> out of content This CL moves the "Do not track" dialog out of the page content where it was picking up css erroneously. BUG=724394 ========== to ========== [MD settings] move <dialog> out of content This CL moves the "Do not track" dialog out of the page content where it was picking up css erroneously. BUG=724394 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@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...
dschuyler@chromium.org changed reviewers: + hcarmona@chromium.org
https://codereview.chromium.org/2899533002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (left): https://codereview.chromium.org/2899533002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:127: <paper-button class="action-button" This was picking up CSS from .settings-box paper-button Because it was actually within a settings-box. https://codereview.chromium.org/2899533002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2899533002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/privacy_page/privacy_page.html:64: </template> This should be moved with no other changes.
This would introduce an a11y issue by moving the focus to the beginning of the section. Dialog focus is weird. Focus remains where the dialog was. By having the dialog just before the element that opens it we can ensure that focus behaves in an expected manner.
On 2017/05/20 at 00:00:36, hcarmona wrote: > This would introduce an a11y issue by moving the focus to the beginning of the section. > > Dialog focus is weird. Focus remains where the dialog was. By having the dialog just before the element that opens it we can ensure that focus behaves in an expected manner. Indeed return focus behavior when a dialog closes is a bit odd, but for this reason we should be explicitly handling focus when a dialog closes. We already do this in most places (if not all). Example from the same file https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/privac.... In other words, we should not let the out-of-the-box dialog focus behavior prevent us from placing the <dialog> where it makes more sense in the markup.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dschuyler@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 dschuyler@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_toggle_button.js (right): https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_toggle_button.js:12: behaviors: [SettingsBooleanControlBehavior], Just moved, not changed. https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_toggle_button.js:23: this.$.control.focus(); Pass along the focus to avoid doing this elsewhere: this.$.toggleButton.$.control.focus() and allowing: this.$.toggleButton.focus() https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.html (right): https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.html:51: on-close="onDoNotTrackDialogClosed_"> Line 51 is new, the rest is simply moved.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/ some nits https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_toggle_button.js (right): https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_toggle_button.js:21: /** @private */ Should this be @override instead? https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:195: cr.ui.focusWithoutInk(assert(this.$.doNotTrack)); NIT: is the assert necessary?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/controls/settings_toggle_button.js (right): https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/controls/settings_toggle_button.js:21: /** @private */ On 2017/05/24 18:05:36, hcarmona wrote: > Should this be @override instead? Done. https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/privacy_page/privacy_page.js (right): https://codereview.chromium.org/2899533002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/privacy_page/privacy_page.js:195: cr.ui.focusWithoutInk(assert(this.$.doNotTrack)); On 2017/05/24 18:05:37, hcarmona wrote: > NIT: is the assert necessary? done.
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2899533002/#ps60001 (title: "review changes")
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": 60001, "attempt_start_ts": 1495669722590280,
"parent_rev": "f0115f469e06ebf49f0265ec89eee5e9220f187c", "commit_rev":
"516d1b3d84dece163efa12412b76f10a0f4ba5b1"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] move <dialog> out of content This CL moves the "Do not track" dialog out of the page content where it was picking up css erroneously. BUG=724394 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] move <dialog> out of content This CL moves the "Do not track" dialog out of the page content where it was picking up css erroneously. BUG=724394 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2899533002 Cr-Commit-Position: refs/heads/master@{#474481} Committed: https://chromium.googlesource.com/chromium/src/+/516d1b3d84dece163efa12412b76... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/516d1b3d84dece163efa12412b76... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
