Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841873004:
MD Settings: Fix subpage navigation focus for bluetooth+internet (Closed)
|
Created:
3 years, 8 months ago by stevenjb Modified:
3 years, 7 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-elements_chromium.org, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg+watch-elements_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Fix subpage navigation focus for bluetooth+internet
This CL:
* Fixes the subpage arrow for bluetooth and focus it correctly.
* Provides focus logic for internet-page -> internet-subpage ->
internet-detail-page.
* Fixes keyboard navigation to details page from cr-network-list-item
BUG=714350
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2841873004
Cr-Commit-Position: refs/heads/master@{#467783}
Committed: https://chromium.googlesource.com/chromium/src/+/bfd9716d05641238fc5e00e0f83ec8862928974a
Patch Set 1 #Patch Set 2 : . #
Total comments: 20
Patch Set 3 : Feedack #
Total comments: 6
Patch Set 4 : Feedback 2 #Patch Set 5 : Fix browser tests #Messages
Total messages: 20 (8 generated)
Description was changed from ========== MD Settings: Fix subpage navigation focus for bluetooth+internet This CL: * Fixes the subpage arrow for bluetooth and focus it correctly. * Provides focus logic for internet-page -> internet-subpage -> internet-detail-page. * Fixes keyboard navigation to details page from cr-network-list-item BUG=714350 ========== to ========== MD Settings: Fix subpage navigation focus for bluetooth+internet This CL: * Fixes the subpage arrow for bluetooth and focus it correctly. * Provides focus logic for internet-page -> internet-subpage -> internet-detail-page. * Fixes keyboard navigation to details page from cr-network-list-item BUG=714350 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html (right): https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:36: <template is="dom-if" if="[[bluetoothToggleState_]]"> Was this pointing to a non-existent Polymer property? https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_page.js (right): https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:93: * @type {!Map<string, string>} Ni: Use shorthand syntax, @private {!Map<string, string>} https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:181: } else if (route != settings.Route.INTERNET && route.path != '/') { Is this equivalent to the following? } else if (route != settings.Route.INTERNET && route != settings.Route.BASIC) { https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:187: if (oldRoute == settings.Route.NETWORK_DETAIL || Is this equivalent to the following? if (!settings.Route.INTERNET.contains(oldRoute)) return; // Do remaining stuff here https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:191: if (route == settings.Route.INTERNET_NETWORKS) { Nit (optional): This can be compacted as follows: // Comment here var selector = route == settings.Route.INTERNET_NETWORKS ? '* /deep/ #networkList' : '* /deep/ #' + this.detailType_ + ' /deep/ .subpage-arrow'; https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:199: if (selector && this.querySelector(selector)) When would querySelector(selector) return nothing here? https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:200: this.focusConfig_.set(oldRoute.path, selector); I don't fully understand the updates to the focusConfig_ map here, but it seems that we might be leaving an obsolete entry in the map. Shouldn't there be an else { this.focusConfig_.delete(oldRoute.path); } https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/network/cr_network_list_item.html:93: <iron-a11y-keys keys="space enter" on-keys-pressed="fireShowDetails_"> I don't fully understand the need for this. Where is the focus when "space will trigger on-tap but enter will not"? I don't recall having encountered this issue elsewhere in the Settings code, but the phrasing makes it sound that this is a generic problem. Is it equivalent to just move the on-tap handler to the div above?
PTAL https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html (right): https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.html:36: <template is="dom-if" if="[[bluetoothToggleState_]]"> On 2017/04/25 21:25:19, dpapad wrote: > Was this pointing to a non-existent Polymer property? Correct. A renamed property to be specific. https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_page.js (right): https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:93: * @type {!Map<string, string>} On 2017/04/25 21:25:19, dpapad wrote: > Ni: Use shorthand syntax, > @private {!Map<string, string>} Done. https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:181: } else if (route != settings.Route.INTERNET && route.path != '/') { On 2017/04/25 21:25:19, dpapad wrote: > Is this equivalent to the following? > > } else if (route != settings.Route.INTERNET && route != settings.Route.BASIC) { Looks like it. Done. https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:187: if (oldRoute == settings.Route.NETWORK_DETAIL || On 2017/04/25 21:25:19, dpapad wrote: > Is this equivalent to the following? > > if (!settings.Route.INTERNET.contains(oldRoute)) > return; > > // Do remaining stuff here Handy. Yes. Done. https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:191: if (route == settings.Route.INTERNET_NETWORKS) { On 2017/04/25 21:25:19, dpapad wrote: > Nit (optional): This can be compacted as follows: > > // Comment here > var selector = route == settings.Route.INTERNET_NETWORKS ? > '* /deep/ #networkList' : > '* /deep/ #' + this.detailType_ + ' /deep/ .subpage-arrow'; Acknowledged. https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:199: if (selector && this.querySelector(selector)) On 2017/04/25 21:25:19, dpapad wrote: > When would querySelector(selector) return nothing here? If an Ethernet cable were unplugged while visiting an Ethernet detail page. https://codereview.chromium.org/2841873004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:200: this.focusConfig_.set(oldRoute.path, selector); On 2017/04/25 21:25:19, dpapad wrote: > I don't fully understand the updates to the focusConfig_ map here, but it seems > that we might be leaving an obsolete entry in the map. Shouldn't there be an > > else { > this.focusConfig_.delete(oldRoute.path); > } Done. (I took this approach because of the dynamic nature of the Network section. Originally I had ambitions to highlight the subpage button for the corresponding network when returning from a details subpage, but iron-list behavior kicked my ass so I am punting for now and just focusing the entire list in that case. We still have they dynamic nature of the available types mentioned above though). https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/network/cr_network_list_item.html:93: <iron-a11y-keys keys="space enter" on-keys-pressed="fireShowDetails_"> On 2017/04/25 21:25:19, dpapad wrote: > I don't fully understand the need for this. Where is the focus when "space will > trigger on-tap but enter will not"? I don't recall having encountered this issue > elsewhere in the Settings code, but the phrasing makes it sound that this is a > generic problem. > > Is it equivalent to just move the on-tap handler to the div above? OK, after way too much time investigating, I discovered that iron-list is eating 'enter'. I don't really understand why that is preventing on-tap, but I think that 'enter' firing on-tap is a Polymer thing so maybe that is why? I will document this better. Also, I think this may be the only place where we have an embedded button in an iron-list item that doesn't just do the default action for the list entry. (We have some embedded menus, but I think they have explicit keyboard handling).
https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/network/cr_network_list_item.html:93: <iron-a11y-keys keys="space enter" on-keys-pressed="fireShowDetails_"> On 2017/04/25 at 23:32:53, stevenjb wrote: > On 2017/04/25 21:25:19, dpapad wrote: > > I don't fully understand the need for this. Where is the focus when "space will > > trigger on-tap but enter will not"? I don't recall having encountered this issue > > elsewhere in the Settings code, but the phrasing makes it sound that this is a > > generic problem. > > > > Is it equivalent to just move the on-tap handler to the div above? > > OK, after way too much time investigating, I discovered that iron-list is eating 'enter'. I don't really understand why that is preventing on-tap, but I think that 'enter' firing on-tap is a Polymer thing so maybe that is why? I will document this better. > > Also, I think this may be the only place where we have an embedded button in an iron-list item that doesn't just do the default action for the list entry. (We have some embedded menus, but I think they have explicit keyboard handling). I filed https://bugs.chromium.org/p/chromium/issues/detail?id=715353 to track that issue and possibly get some help from scottchen@ who has dealt a lot with iron-lists. Also the keyboard behavior of this iron-list seems different than other iron-lists on our page, which is why I suggest that we don't do anything here to side-step the problem until we understand why it is happening in the 1st place. WDYT? https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:195: var button = this.$$('#buttonDiv .primary-button:not([hidden])'); Nit (optional): You can also use a single selector as follows. var button = this.$$( '#buttonDiv .primary-button:not([hidden]), ' + '#buttonDiv .secondary-button:not([hidden])'); assert(button); // Expecting at least one button to always be showing. button.focus(); https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:303: this.maybeSetFocus_(); I missed this file completely in previous iteration. Couple of questions: 1) The changes in this file seem to not be related with the "exiting a subpage" case. Instead here it tries to focus something when we are entering a subpage. Is my understanding correct? Also why do we need to call focus related code in getPropertiesCallback_ and getStateCallback_? Per my understanding, when we set |networkProperties| the observer networkPropertiesChanged_ is fired, which in turn causes some kind of page transition. Can we move the focus() call inside |networkPropertiesChanged_| instead? The we might not need the "maybeSetFocus()" logic either.
https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/network/cr_network_list_item.html:93: <iron-a11y-keys keys="space enter" on-keys-pressed="fireShowDetails_"> On 2017/04/26 00:22:38, dpapad wrote: > On 2017/04/25 at 23:32:53, stevenjb wrote: > > On 2017/04/25 21:25:19, dpapad wrote: > > > I don't fully understand the need for this. Where is the focus when "space > will > > > trigger on-tap but enter will not"? I don't recall having encountered this > issue > > > elsewhere in the Settings code, but the phrasing makes it sound that this is > a > > > generic problem. > > > > > > Is it equivalent to just move the on-tap handler to the div above? > > > > OK, after way too much time investigating, I discovered that iron-list is > eating 'enter'. I don't really understand why that is preventing on-tap, but I > think that 'enter' firing on-tap is a Polymer thing so maybe that is why? I will > document this better. > > > > Also, I think this may be the only place where we have an embedded button in > an iron-list item that doesn't just do the default action for the list entry. > (We have some embedded menus, but I think they have explicit keyboard handling). > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=715353 to track > that issue and possibly get some help from scottchen@ who has dealt a lot with > iron-lists. > > Also the keyboard behavior of this iron-list seems different than other > iron-lists on our page, which is why I suggest that we don't do anything here to > side-step the problem until we understand why it is happening in the 1st place. > WDYT? I don't think there is anything actually different about the keyboard behavior in this iron-list. Sorry if I wasn't clear, but this is why enter doesn't work: https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... I'll comment on the issue you created. https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:195: var button = this.$$('#buttonDiv .primary-button:not([hidden])'); On 2017/04/26 00:22:38, dpapad wrote: > Nit (optional): You can also use a single selector as follows. > > var button = this.$$( > '#buttonDiv .primary-button:not([hidden]), ' + > '#buttonDiv .secondary-button:not([hidden])'); > > assert(button); // Expecting at least one button to always be showing. > button.focus(); Does that perform the first query and then the second one? We want a primary button if not hidden and a secondary button only if there is no visible primary. (The primary buttons are not necessarily first in the DOM ordering). The documentation for querySelector says 'depth-first pre-order traversal' with no information about how multiple selectors work. I will add the assert though. https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:303: this.maybeSetFocus_(); On 2017/04/26 00:22:38, dpapad wrote: > I missed this file completely in previous iteration. Couple of questions: > > 1) The changes in this file seem to not be related with the "exiting a subpage" > case. Instead here it tries to focus something when we are entering a subpage. > Is my understanding correct? > > Also why do we need to call focus related code in getPropertiesCallback_ and > getStateCallback_? > > Per my understanding, when we set |networkProperties| the observer > networkPropertiesChanged_ is fired, which in turn causes some kind of page > transition. Can we move the focus() call inside |networkPropertiesChanged_| > instead? The we might not need the "maybeSetFocus()" logic either. 1. Which buttons are visible may change after we set |this.networkProperties|. 2. We still need didSetFocus_ because this will get called if the network properties change and we don't want to change focus later (the user may have changed focus themselves at that point). Moving this to networkPropertiesChanged_ however makes sense so I will do that. https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_page.js (right): https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_page.js:201: this.detailType_ = ''; While testing I noticed that this causes problems when navigating back from detail page -> subpage (detailType_ gets cleared) -> main page (detailType_ is now unset). Leaving this set provides a breadcrumb and won't cause any problems.
https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/network/cr_network_list_item.html:93: <iron-a11y-keys keys="space enter" on-keys-pressed="fireShowDetails_"> > I don't think there is anything actually different about the keyboard behavior in this iron-list. > Compare for example the search engines iron-list (chrome://md-settings/searchEngines) with this one. Repro steps to showcase the difference 1) Tab to the 1st item in the iron-list, see http://imgur.com/a/lnqHJ: Search engines: The 1st "dots" icon is focused. networks: The entire row is focused. 2) Hit tab one more time, see http://imgur.com/a/Y8XzI: Search engines: The focus goes to the 1st item *after* the iron list. networks: The focus goes to the button within the row. Are you not observing the described behavior above? https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2841873004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:195: var button = this.$$('#buttonDiv .primary-button:not([hidden])'); On 2017/04/26 at 17:53:36, stevenjb wrote: > On 2017/04/26 00:22:38, dpapad wrote: > > Nit (optional): You can also use a single selector as follows. > > > > var button = this.$$( > > '#buttonDiv .primary-button:not([hidden]), ' + > > '#buttonDiv .secondary-button:not([hidden])'); > > > > assert(button); // Expecting at least one button to always be showing. > > button.focus(); > > Does that perform the first query and then the second one? We want a primary button if not hidden and a secondary button only if there is no visible primary. (The primary buttons are not necessarily first in the DOM ordering). The documentation for querySelector says 'depth-first pre-order traversal' with no information about how multiple selectors work. > > I will add the assert though. Hm, I think it returns the 1st item that matches either of the two conditions, so I understand now that might not be equivalent. Please ignore.
https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/network/cr_network_list_item.html (right): https://codereview.chromium.org/2841873004/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/network/cr_network_list_item.html:93: <iron-a11y-keys keys="space enter" on-keys-pressed="fireShowDetails_"> On 2017/04/26 18:14:49, dpapad wrote: > > I don't think there is anything actually different about the keyboard behavior > in this iron-list. > > > > Compare for example the search engines iron-list > (chrome://md-settings/searchEngines) with this one. Repro steps to showcase the > difference > > 1) Tab to the 1st item in the iron-list, see http://imgur.com/a/lnqHJ: > Search engines: The 1st "dots" icon is focused. > networks: The entire row is focused. > 2) Hit tab one more time, see http://imgur.com/a/Y8XzI: > Search engines: The focus goes to the 1st item *after* the iron list. > networks: The focus goes to the button within the row. > > Are you not observing the described behavior above? Yes, that is what I am seeing. It sounds like you mean that the focus behavior specifically is different. Unfortunately most of our iron-list implementations are slightly different in their behavior. The focus behavior for networks is as previously discussed with UX: * Tab focuses the list (which focuses the current -defaults to first- list item). ** Enter/tap performs the default action for the item (connect or view details if already connected). * A second tab focuses the 'details' button ** Enter / tap / space (because its a button) views details. * A third tab focuses the next element (i.e. not the list). This is consistent with most of our iron-list implementations, with the exception that this list has both actionable items and an actionable button. Other lists have one or the other.
LGTM
The CQ bit was checked by stevenjb@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2841873004/#ps80001 (title: "Fix browser tests")
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": 1493318577946880,
"parent_rev": "32d29a10153865441252e100a9781648b8df795b", "commit_rev":
"bfd9716d05641238fc5e00e0f83ec8862928974a"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Fix subpage navigation focus for bluetooth+internet This CL: * Fixes the subpage arrow for bluetooth and focus it correctly. * Provides focus logic for internet-page -> internet-subpage -> internet-detail-page. * Fixes keyboard navigation to details page from cr-network-list-item BUG=714350 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Fix subpage navigation focus for bluetooth+internet This CL: * Fixes the subpage arrow for bluetooth and focus it correctly. * Provides focus logic for internet-page -> internet-subpage -> internet-detail-page. * Fixes keyboard navigation to details page from cr-network-list-item BUG=714350 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2841873004 Cr-Commit-Position: refs/heads/master@{#467783} Committed: https://chromium.googlesource.com/chromium/src/+/bfd9716d05641238fc5e00e0f83e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/bfd9716d05641238fc5e00e0f83e... |
