Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813033004:
MD Settings: Restore focus after exiting Content settings sub-subpages. (Closed)
|
Created:
3 years, 8 months ago by dpapad Modified:
3 years, 8 months ago Reviewers:
Dan Beam 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 exiting Content settings sub-subpages,.
BUG=709586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2813033004
Cr-Commit-Position: refs/heads/master@{#464085}
Committed: https://chromium.googlesource.com/chromium/src/+/f6d9d3d52ed411f5c39948cbb48969f1ccf41192
Patch Set 1 #
Total comments: 8
Patch Set 2 : More work #
Total comments: 7
Patch Set 3 : Address comments. #
Messages
Total messages: 27 (20 generated)
Description was changed from ========== MD Settings: Restore focus after exiting Content settincgs sub-subpages, WIP. BUG=709586 ========== to ========== MD Settings: Restore focus after exiting Content settincgs sub-subpages, WIP. BUG=709586 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Restore focus after exiting Content settincgs sub-subpages, WIP. BUG=709586 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Restore focus after exiting Content settings sub-subpages, WIP. BUG=709586 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/settings_animated_pages.js (right): https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/settings_animated_pages.js:66: e.detail.item.id == 'site-settings')) { !e.detail.item.matches('neon-animatable, settings-subpage#site-settings') https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings_page/site_settings_page.js (right): https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:86: }; 'getDeepSelector()'.length > "'* /deep/ ".length // true https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:88: this.focusConfig.set( why does this object already exist? https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:90: getDeepSelector('#cookies .subpage-arrow')); can we just inline the /deep/ calls? '* /deep/ #cookies .subpage-arrow'
Description was changed from ========== MD Settings: Restore focus after exiting Content settings sub-subpages, WIP. BUG=709586 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Restore focus after exiting Content settings sub-subpages,. BUG=709586 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) 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...
https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings_page/site_settings_page.js (right): https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:86: }; On 2017/04/12 at 01:05:26, Dan Beam wrote: > 'getDeepSelector()'.length > "'* /deep/ ".length // true Not applicable anymore. https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:88: this.focusConfig.set( On 2017/04/12 at 01:05:26, Dan Beam wrote: > why does this object already exist? This is now explained in the comment. The |focusConfig| map belongs to the parent <settings-animated-pages> element. I am passing it down to this element to add some additional entries to it. Note that passing it down is not necessary, but I think it would be very odd if we start referring to DOM element IDs in site_settings_page.html from privacy_page.html directly. Populating the map with additional entries here seems more appropriate. https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:90: getDeepSelector('#cookies .subpage-arrow')); On 2017/04/12 at 01:05:26, Dan Beam wrote: > can we just inline the /deep/ calls? > > '* /deep/ #cookies .subpage-arrow' See latest patch. https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings_page/site_settings_page.js (right): https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:72: // elements residing in this element's Shadow DOM. Populating this.focusConfig only after it is set to an initial value has two benefits 1) It ensures that there are no race conditions (always executes after the parent sets the initial value). 2) In site_settings_page_browsertest.js tests, where a settings-animated-pages is created on its own and has no parent, this code never executes on purpose.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Ah, need to resolve conflicts with https://codereview.chromium.org/2806203004.
i guess this lgtm https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings_page/site_settings_page.js (right): https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:52: /** @type {!Map<string, string>} */ @override maybe? https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:60: * @param {!Map<string, string>} current nit: newConfig, oldConfig https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:68: } assert(!oldConfig);
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...
Patchset #3 (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...
https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_page/settings_animated_pages.js (right): https://codereview.chromium.org/2813033004/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_page/settings_animated_pages.js:66: e.detail.item.id == 'site-settings')) { On 2017/04/12 at 01:05:26, Dan Beam wrote: > !e.detail.item.matches('neon-animatable, settings-subpage#site-settings') Done. https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings_page/site_settings_page.js (right): https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:52: /** @type {!Map<string, string>} */ On 2017/04/12 at 02:32:02, Dan Beam wrote: > @override maybe? Tried @override and compiler seems to be happy either way, but semantically I don't think override is appropriate. This is not inherited from any superclass (or behavior), it is simply passed from the outside. https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:60: * @param {!Map<string, string>} current On 2017/04/12 at 02:32:02, Dan Beam wrote: > nit: newConfig, oldConfig Done. https://codereview.chromium.org/2813033004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings_page/site_settings_page.js:68: } On 2017/04/12 at 02:32:02, Dan Beam wrote: > assert(!oldConfig); Done.
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 dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2813033004/#ps80001 (title: "Address comments.")
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": 80001, "attempt_start_ts": 1492018386737270,
"parent_rev": "8588407dac066cab5eac58b342b9ebd4f75bc848", "commit_rev":
"f6d9d3d52ed411f5c39948cbb48969f1ccf41192"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Restore focus after exiting Content settings sub-subpages,. BUG=709586 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Restore focus after exiting Content settings sub-subpages,. BUG=709586 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2813033004 Cr-Commit-Position: refs/heads/master@{#464085} Committed: https://chromium.googlesource.com/chromium/src/+/f6d9d3d52ed411f5c39948cbb489... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f6d9d3d52ed411f5c39948cbb489... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
