Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104013004:
MD WebUI: Reimplement cr-shared-menu using iron-dropdown. (Closed)
|
Created:
4 years, 5 months ago by tsergeant Modified:
4 years, 5 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, dbeam+watch-elements_chromium.org, jlklein+watch-closure_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-closure_chromium.org, hcarmona Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD WebUI: Reimplement cr-shared-menu using iron-dropdown.
This change brings the implementation much closer to that of
paper-menu-button, and fixes a number of longstanding bugs. In
particular,
* Focus is correctly restored to the element which opened the menu after
the menu is closed.
* It is only necessary to press escape once to close the menu.
* It is possible to close the menu by clicking anywhere else on the
page.
* Tab focusing is trapped inside the menu, rather than allowing tab to
go to a different part of the page.
* Bold text is no longer applied the menu item which was last clicked.
The main trade-off of this approach is that we now have to use neon-animation
to animate the menu out, and due to the way that the menu is positioned
it will animate in from the top-left corner (the same as
paper-menu-button).
BUG=589362, 603454, 616074, 619839, 620038
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/9aecdea1b2386028f8b3ad5e47fd6d98a6add2d9
Cr-Commit-Position: refs/heads/master@{#405048}
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 4
Patch Set 3 : Reword comment #
Total comments: 25
Patch Set 4 : Address dbeam@ review #
Total comments: 4
Patch Set 5 : Nits #
Messages
Total messages: 26 (11 generated)
Description was changed from ========== MD WebUI: Reimplement cr-shared-menu using iron-dropdown. This change brings the implementation much closer to that of paper-menu-button, and fixes a number of longstanding bugs. In particular, * Focus is correctly restored to the element which opened the menu after the menu is closed. * It is only necessary to press escape once to close the menu. * It is possible to close the menu by clicking anywhere else on the page. * Tab focusing is trapped inside the menu, rather than allowing tab to go to a different part of the page. * Bold text is no longer applied the menu item which was last clicked. The main trade-off of this approach is that we now have to use neon-animation to animate the menu out, and due to the way that the menu is positioned it will animate in from the top-left corner (the same as paper-menu-button). BUG=589326,603454,616074,619839,620038 ========== to ========== MD WebUI: Reimplement cr-shared-menu using iron-dropdown. This change brings the implementation much closer to that of paper-menu-button, and fixes a number of longstanding bugs. In particular, * Focus is correctly restored to the element which opened the menu after the menu is closed. * It is only necessary to press escape once to close the menu. * It is possible to close the menu by clicking anywhere else on the page. * Tab focusing is trapped inside the menu, rather than allowing tab to go to a different part of the page. * Bold text is no longer applied the menu item which was last clicked. The main trade-off of this approach is that we now have to use neon-animation to animate the menu out, and due to the way that the menu is positioned it will animate in from the top-left corner (the same as paper-menu-button). BUG=589326,603454,616074,619839,620038 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
tsergeant@chromium.org changed reviewers: + calamity@chromium.org
+calamity for a local pass
Description was changed from ========== MD WebUI: Reimplement cr-shared-menu using iron-dropdown. This change brings the implementation much closer to that of paper-menu-button, and fixes a number of longstanding bugs. In particular, * Focus is correctly restored to the element which opened the menu after the menu is closed. * It is only necessary to press escape once to close the menu. * It is possible to close the menu by clicking anywhere else on the page. * Tab focusing is trapped inside the menu, rather than allowing tab to go to a different part of the page. * Bold text is no longer applied the menu item which was last clicked. The main trade-off of this approach is that we now have to use neon-animation to animate the menu out, and due to the way that the menu is positioned it will animate in from the top-left corner (the same as paper-menu-button). BUG=589326,603454,616074,619839,620038 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Reimplement cr-shared-menu using iron-dropdown. This change brings the implementation much closer to that of paper-menu-button, and fixes a number of longstanding bugs. In particular, * Focus is correctly restored to the element which opened the menu after the menu is closed. * It is only necessary to press escape once to close the menu. * It is possible to close the menu by clicking anywhere else on the page. * Tab focusing is trapped inside the menu, rather than allowing tab to go to a different part of the page. * Bold text is no longer applied the menu item which was last clicked. The main trade-off of this approach is that we now have to use neon-animation to animate the menu out, and due to the way that the menu is positioned it will animate in from the top-left corner (the same as paper-menu-button). BUG=589326,603454,616074,619839,620038 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
/cc hcarmona@ as fyi
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
https://codereview.chromium.org/2104013004/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (right): https://codereview.chromium.org/2104013004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:59: // opening. Does this mean this test could flake? Or is the behavior initialized by the time it's open? https://codereview.chromium.org/2104013004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2104013004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:124: this.lastFocus_ = focusableChildren[focusableChildren.length - 1]; Do you need to recompute this if new elements are added? I think iron-pages does something like that.
https://codereview.chromium.org/2104013004/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (right): https://codereview.chromium.org/2104013004/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:59: // opening. On 2016/07/06 06:49:15, calamity wrote: > Does this mean this test could flake? Or is the behavior initialized by the time > it's open? Yeah, it's done between when you tap the button and when iron-overlay-opened is fired. I've updated the comment to reflect this. https://codereview.chromium.org/2104013004/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2104013004/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:124: this.lastFocus_ = focusableChildren[focusableChildren.length - 1]; On 2016/07/06 06:49:15, calamity wrote: > Do you need to recompute this if new elements are added? I think iron-pages does > something like that. Ideally, yes. However, I'd like to keep this as simple as possible until iron-focus-wrap-behavior is available (which hopefully will be soonish -- there's a PR out at the moment).
lgtm. Separate rebase and comment fix patches in future.
On 2016/07/07 05:57:16, calamity wrote: > lgtm. Separate rebase and comment fix patches in future. Oops, sorry
tsergeant@chromium.org changed reviewers: + dbeam@chromium.org
dbeam, can you please take a look?
i don't think bug 589326 is right (in your CL desc) in general, I guess I'm OK with moving to <iron-dropdown>, but we were asked to <paper-listbox> for a reason (I think performance?). I don't love that we're rolling Yet Another Focus Manager :(. https://codereview.chromium.org/2104013004/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right): https://codereview.chromium.org/2104013004/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:116: cr_shared_menu.registerTests(); you can just load the tests globally (i.e. rather than making registerTests, then calling registerTests() here, just expose them globally) and leave the mocha.run() call https://codereview.chromium.org/2104013004/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (right): https://codereview.chromium.org/2104013004/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:26: 'chrome://resources/cr_elements/icons.html'), why do you need this? https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.html (right): https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.html:24: vertical-align="auto" horizontal-align="right" opened="{{menuOpen}}" does this horizontal-align flip automagically in RTL? https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:29: do all of these need /** @override */? https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:104: * @type {?Element} nit: @private {?Element} https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:119: var focusableChildren = Polymer.dom(this).querySelectorAll( why are you doing this only once on attached? what if the DOM changes? :( https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:120: '[tabindex],button'); what if the thing is [disabled] or [hidden]? this is gonna get real hairy real fast https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:135: * be used when the menu action causes another element to be focused. can we combine both closeMenu*() and just restore focus if our sub-DOM is still "active" (i.e. still has focus)? https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:177: var keyEvent = e.detail.keyboardEvent; declare vars as low as possible https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:178: if (this.firstFocus_ && this.lastFocus_) { arguable nit: just bail if either of these aren't set https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:181: this.lastFocus_.focus(); make a variable |toFocus| (or something) and prevent the default and call .focus() if it exists after this block https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:186: } tl;dr - my suggestions combined if (!this.firstFocus_ || !this.lastFocus_) return; var toFocus; var keyEvent = e.detail.keyboardEvent; if (keyEvent.shiftKey && keyEvent.target == this.firstFocus_) toFocus = this.lastFocus_; else if (keyEvent.target == this.lastFocus_) toFocus = this.firstFocus_; if (!toFocus) return; e.preventDefault(); toFocus.focus(); https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:197: } no curlies
Description was changed from ========== MD WebUI: Reimplement cr-shared-menu using iron-dropdown. This change brings the implementation much closer to that of paper-menu-button, and fixes a number of longstanding bugs. In particular, * Focus is correctly restored to the element which opened the menu after the menu is closed. * It is only necessary to press escape once to close the menu. * It is possible to close the menu by clicking anywhere else on the page. * Tab focusing is trapped inside the menu, rather than allowing tab to go to a different part of the page. * Bold text is no longer applied the menu item which was last clicked. The main trade-off of this approach is that we now have to use neon-animation to animate the menu out, and due to the way that the menu is positioned it will animate in from the top-left corner (the same as paper-menu-button). BUG=589326,603454,616074,619839,620038 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Reimplement cr-shared-menu using iron-dropdown. This change brings the implementation much closer to that of paper-menu-button, and fixes a number of longstanding bugs. In particular, * Focus is correctly restored to the element which opened the menu after the menu is closed. * It is only necessary to press escape once to close the menu. * It is possible to close the menu by clicking anywhere else on the page. * Tab focusing is trapped inside the menu, rather than allowing tab to go to a different part of the page. * Bold text is no longer applied the menu item which was last clicked. The main trade-off of this approach is that we now have to use neon-animation to animate the menu out, and due to the way that the menu is positioned it will animate in from the top-left corner (the same as paper-menu-button). BUG=589362,603454,616074,619839,620038 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
> i don't think bug 589326 is right (in your CL desc) oops, transposed a pair of characters. Fixed > in general, I guess I'm OK with moving to <iron-dropdown>, but we were asked to > <paper-listbox> for a reason (I think performance?). We were asked to used paper-listbox instead of paper-menu. It's not incompatible with iron-dropdown, so we could still use it, but at the moment it causes a couple of bugs (particularly https://github.com/PolymerElements/iron-menu-behavior/issues/56) and messes with tabindexes a bit. > I don't love that we're rolling Yet Another Focus Manager :(. Me neither. See comment below. https://codereview.chromium.org/2104013004/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right): https://codereview.chromium.org/2104013004/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:116: cr_shared_menu.registerTests(); On 2016/07/07 17:38:24, Dan Beam wrote: > you can just load the tests globally (i.e. rather than making registerTests, > then calling registerTests() here, just expose them globally) and leave the > mocha.run() call Done. https://codereview.chromium.org/2104013004/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (right): https://codereview.chromium.org/2104013004/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:26: 'chrome://resources/cr_elements/icons.html'), On 2016/07/07 17:38:24, Dan Beam wrote: > why do you need this? Oops, leftover from a previous version of the test. Done. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.html (right): https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.html:24: vertical-align="auto" horizontal-align="right" opened="{{menuOpen}}" On 2016/07/07 17:38:24, Dan Beam wrote: > does this horizontal-align flip automagically in RTL? Yup! https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js (right): https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:29: On 2016/07/07 17:38:24, Dan Beam wrote: > do all of these need /** @override */? keyEventTarget should have one, the others should not. Done. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:104: * @type {?Element} On 2016/07/07 17:38:24, Dan Beam wrote: > nit: @private {?Element} Done. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:120: '[tabindex],button'); On 2016/07/07 17:38:24, Dan Beam wrote: > what if the thing is [disabled] or [hidden]? this is gonna get real hairy real > fast My hope was to keep this as simple as possible until iron-overlay-behavior extracts out their focus trapping into a reusable behavior (https://github.com/PolymerElements/iron-overlay-behavior/pull/178). This simple version is enough to work with all current uses of cr-shared-menu. I can still implement these fixes if you'd prefer, but like you say, it will get hairy quickly. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:135: * be used when the menu action causes another element to be focused. On 2016/07/07 17:38:24, Dan Beam wrote: > can we combine both closeMenu*() and just restore focus if our sub-DOM is still > "active" (i.e. still has focus)? Good idea, Done. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:177: var keyEvent = e.detail.keyboardEvent; On 2016/07/07 17:38:24, Dan Beam wrote: > declare vars as low as possible Done. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:178: if (this.firstFocus_ && this.lastFocus_) { On 2016/07/07 17:38:24, Dan Beam wrote: > arguable nit: just bail if either of these aren't set Done. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:181: this.lastFocus_.focus(); On 2016/07/07 17:38:24, Dan Beam wrote: > make a variable |toFocus| (or something) and prevent the default and call > .focus() if it exists after this block Done. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:186: } On 2016/07/07 17:38:24, Dan Beam wrote: > tl;dr - my suggestions combined > > if (!this.firstFocus_ || !this.lastFocus_) > return; > > var toFocus; > var keyEvent = e.detail.keyboardEvent; > if (keyEvent.shiftKey && keyEvent.target == this.firstFocus_) > toFocus = this.lastFocus_; > else if (keyEvent.target == this.lastFocus_) > toFocus = this.firstFocus_; > > if (!toFocus) > return; > > e.preventDefault(); > toFocus.focus(); Done. https://codereview.chromium.org/2104013004/diff/60001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_shared_menu/cr_shared_menu.js:197: } On 2016/07/07 17:38:24, Dan Beam wrote: > no curlies Done.
lgtm w/optional nits https://codereview.chromium.org/2104013004/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (right): https://codereview.chromium.org/2104013004/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:16: menu.addEventListener('iron-overlay-opened', callback); nit: menu.addEventListener('iron-overlay-opened', function f() { menu.removeEventListener('iron-overlay-opened', f); callback(); }); https://codereview.chromium.org/2104013004/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:119: nit: remove
https://codereview.chromium.org/2104013004/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js (right): https://codereview.chromium.org/2104013004/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:16: menu.addEventListener('iron-overlay-opened', callback); On 2016/07/12 02:11:38, Dan Beam wrote: > nit: > > menu.addEventListener('iron-overlay-opened', function f() { > menu.removeEventListener('iron-overlay-opened', f); > callback(); > }); Done. https://codereview.chromium.org/2104013004/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_shared_menu_tests.js:119: On 2016/07/12 02:11:38, Dan Beam wrote: > nit: remove Done.
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2104013004/#ps100001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== MD WebUI: Reimplement cr-shared-menu using iron-dropdown. This change brings the implementation much closer to that of paper-menu-button, and fixes a number of longstanding bugs. In particular, * Focus is correctly restored to the element which opened the menu after the menu is closed. * It is only necessary to press escape once to close the menu. * It is possible to close the menu by clicking anywhere else on the page. * Tab focusing is trapped inside the menu, rather than allowing tab to go to a different part of the page. * Bold text is no longer applied the menu item which was last clicked. The main trade-off of this approach is that we now have to use neon-animation to animate the menu out, and due to the way that the menu is positioned it will animate in from the top-left corner (the same as paper-menu-button). BUG=589362,603454,616074,619839,620038 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD WebUI: Reimplement cr-shared-menu using iron-dropdown. This change brings the implementation much closer to that of paper-menu-button, and fixes a number of longstanding bugs. In particular, * Focus is correctly restored to the element which opened the menu after the menu is closed. * It is only necessary to press escape once to close the menu. * It is possible to close the menu by clicking anywhere else on the page. * Tab focusing is trapped inside the menu, rather than allowing tab to go to a different part of the page. * Bold text is no longer applied the menu item which was last clicked. The main trade-off of this approach is that we now have to use neon-animation to animate the menu out, and due to the way that the menu is positioned it will animate in from the top-left corner (the same as paper-menu-button). BUG=589362,603454,616074,619839,620038 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/9aecdea1b2386028f8b3ad5e47fd6d98a6add2d9 Cr-Commit-Position: refs/heads/master@{#405048} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9aecdea1b2386028f8b3ad5e47fd6d98a6add2d9 Cr-Commit-Position: refs/heads/master@{#405048} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
