Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801453002:
MD Settings: mouse movements should focus cr-action-menu items (Closed)
|
Created:
3 years, 8 months ago by scottchen Modified:
3 years, 8 months ago CC:
chromium-reviews, dbeam+watch-elements_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: mouse movements should focus cr-action-menu items
BUG=703975
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2801453002
Cr-Commit-Position: refs/heads/master@{#462674}
Committed: https://chromium.googlesource.com/chromium/src/+/12399c275d9261a7cb37816faffd1b7e35fbbd6b
Patch Set 1 #Patch Set 2 : experimental debounce #
Total comments: 1
Patch Set 3 : mimic context menu on mouse out #
Total comments: 8
Patch Set 4 : fix off-by-one error when nothing is focused #Patch Set 5 : add tests #
Total comments: 1
Patch Set 6 : updates based on comments #Patch Set 7 : Merge branch 'iron-test-helpers' into hover-menu #
Total comments: 12
Patch Set 8 : optimize event firing #Patch Set 9 : use e.path to target element, add note regarding screen-reader #Patch Set 10 : move off-by-one fix to another CL #
Total comments: 2
Patch Set 11 : remove unnecessary dependency #Patch Set 12 : remove unnecessary test code #
Total comments: 8
Patch Set 13 : updates based on comments #
Total comments: 5
Patch Set 14 : change forloop to do-while for readability #Patch Set 15 : added flag to avoid multiple mousemove listeners #
Total comments: 2
Patch Set 16 : format #
Total comments: 2
Patch Set 17 : move mousemove listener to right before switching focus #Patch Set 18 : add missing import #
Dependent Patchsets: Messages
Total messages: 64 (27 generated)
Description was changed from ========== MD Settings: mouse movements should focus cr-action-menu items BUG=703975 ========== to ========== MD Settings: mouse movements should focus cr-action-menu items BUG=703975 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2801453002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:92: if(!this.debounceFlusher_) { Need this, otherwise if the user keeps moving their mouse on the menu non-stop, the hover will never change target.
On 2017/04/04 20:43:12, scottchen wrote: > https://codereview.chromium.org/2801453002/diff/20001/ui/webui/resources/cr_e... > File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): > > https://codereview.chromium.org/2801453002/diff/20001/ui/webui/resources/cr_e... > ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:92: > if(!this.debounceFlusher_) { > Need this, otherwise if the user keeps moving their mouse on the menu non-stop, > the hover will never change target. dbeam@ not sure if the debounce overhead is worth. Thoughts?
Patchset #5 (id:80001) has been deleted
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
Can we avoid adding a mousemove listener on the dialog? I am concerned about performance - mousemove fires too many times - mousemove fires even when the mouse is outside of the action-menu (because the entire viewport is considered part of the dialog). Proposing the following alternative - add a 'mouseenter' event on every item individually - Change selection when mouseenter fires. - For the corner case where the mouse is hovering over an item right after the menu is opened, we can add a mousemove only on that element with document.elementFromPoint(), and remove that mousemove handler as soon as it fires once. The latter approach will result in way less event handlers being triggered. Any thoughts?
https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:88: jsdoc here (@private, @param) https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:92: if(!this.debounceFlusher_) { please use `git cl format --js` and/or stop forgetting spaces before braces or parens https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:99: this.debounce('cr-action-menu-mousemove', function(){ i don't understand why you need this debounce nor what you're doing https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:101: target != document.activeElement) curlies for this if/else
On 2017/04/04 22:31:05, dpapad wrote: > Can we avoid adding a mousemove listener on the dialog? I am concerned about > performance > - mousemove fires too many times > - mousemove fires even when the mouse is outside of the action-menu (because > the entire viewport is considered part of the dialog). > > Proposing the following alternative > - add a 'mouseenter' event on every item individually > - Change selection when mouseenter fires. > - For the corner case where the mouse is hovering over an item right after the > menu is opened, we can add a mousemove only on that element with > document.elementFromPoint(), and remove that mousemove handler as soon as it > fires once. > > The latter approach will result in way less event handlers being triggered. Any > thoughts? do 'mouseenter' and 'mouseleave' work when using keyboard then resuming using mouse? do they work when leaving the native app window?
On 2017/04/04 22:32:54, Dan Beam wrote: > On 2017/04/04 22:31:05, dpapad wrote: > > Can we avoid adding a mousemove listener on the dialog? I am concerned about > > performance > > - mousemove fires too many times > > - mousemove fires even when the mouse is outside of the action-menu (because > > the entire viewport is considered part of the dialog). > > > > Proposing the following alternative > > - add a 'mouseenter' event on every item individually > > - Change selection when mouseenter fires. > > - For the corner case where the mouse is hovering over an item right after > the > > menu is opened, we can add a mousemove only on that element with > > document.elementFromPoint(), and remove that mousemove handler as soon as it > > fires once. > > > > The latter approach will result in way less event handlers being triggered. > Any > > thoughts? > > do 'mouseenter' and 'mouseleave' work when using keyboard then resuming using > mouse? do they work when leaving the native app window? mouseenter and mouseleave does not work when doing mouse -> keyboard -> mouse within the same option again. I left out this corner case when I was chatting with Demetrios offline, my bad. I think only mousemove works with that case.
On 2017/04/04 22:39:03, scottchen wrote: > On 2017/04/04 22:32:54, Dan Beam wrote: > > On 2017/04/04 22:31:05, dpapad wrote: > > > Can we avoid adding a mousemove listener on the dialog? I am concerned about > > > performance > > > - mousemove fires too many times > > > - mousemove fires even when the mouse is outside of the action-menu > (because > > > the entire viewport is considered part of the dialog). > > > > > > Proposing the following alternative > > > - add a 'mouseenter' event on every item individually > > > - Change selection when mouseenter fires. > > > - For the corner case where the mouse is hovering over an item right after > > the > > > menu is opened, we can add a mousemove only on that element with > > > document.elementFromPoint(), and remove that mousemove handler as soon as it > > > fires once. > > > > > > The latter approach will result in way less event handlers being triggered. > > Any > > > thoughts? > > > > do 'mouseenter' and 'mouseleave' work when using keyboard then resuming using > > mouse? do they work when leaving the native app window? > > mouseenter and mouseleave does not work when doing mouse -> keyboard -> mouse > within the same option again. I left out this corner case when I was chatting > with Demetrios offline, my bad. I think only mousemove works with that case. that's what i expected :(
On 2017/04/04 at 22:41:12, dbeam wrote: > On 2017/04/04 22:39:03, scottchen wrote: > > On 2017/04/04 22:32:54, Dan Beam wrote: > > > On 2017/04/04 22:31:05, dpapad wrote: > > > > Can we avoid adding a mousemove listener on the dialog? I am concerned about > > > > performance > > > > - mousemove fires too many times > > > > - mousemove fires even when the mouse is outside of the action-menu > > (because > > > > the entire viewport is considered part of the dialog). > > > > > > > > Proposing the following alternative > > > > - add a 'mouseenter' event on every item individually > > > > - Change selection when mouseenter fires. > > > > - For the corner case where the mouse is hovering over an item right after > > > the > > > > menu is opened, we can add a mousemove only on that element with > > > > document.elementFromPoint(), and remove that mousemove handler as soon as it > > > > fires once. > > > > > > > > The latter approach will result in way less event handlers being triggered. > > > Any > > > > thoughts? > > > > > > do 'mouseenter' and 'mouseleave' work when using keyboard then resuming using > > > mouse? do they work when leaving the native app window? > > > > mouseenter and mouseleave does not work when doing mouse -> keyboard -> mouse > > within the same option again. I left out this corner case when I was chatting > > with Demetrios offline, my bad. I think only mousemove works with that case. > > that's what i expected :( See my suggestion at https://jsfiddle.net/ppLsLz91/6/. Basically every element has a mousenter event. There is no need for mouseleave. Now, when the selection is changed via keyboard, we are adding a mousemove handler on the currently selected element, so that we can reselect it on mousemove (the mousemove listener is removed as soon as it fires once).
https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:88: On 2017/04/04 22:32:00, Dan Beam wrote: > jsdoc here (@private, @param) Done. https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:92: if(!this.debounceFlusher_) { On 2017/04/04 22:32:00, Dan Beam wrote: > please use `git cl format --js` and/or stop forgetting spaces before braces or > parens I've formatted in the latest patch before you commented. Sorry, I just added you as a reviewer early on because I wanted you to check out the debounce, wasn't quite done with the CL yet with formatting/adding tests. Next time I'll just send you the link via chat and not click publish. https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:99: this.debounce('cr-action-menu-mousemove', function(){ On 2017/04/04 22:32:00, Dan Beam wrote: > i don't understand why you need this debounce nor what you're doing mousemove fires a lot, so I was trying to make the code inside run less times. Seems not worth based on offline discussion, removing. https://codereview.chromium.org/2801453002/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:101: target != document.activeElement) On 2017/04/04 22:32:00, Dan Beam wrote: > curlies for this if/else Done. https://codereview.chromium.org/2801453002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2801453002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:136: MockInteractions.move(items[0], {x: 0, y: 0}, {x: 1, y: 1}); BTW, I'm aware .move is not exposed in MockInteractions in current version of iron-test-helpers. I'll be making a separate CL to roll the version for that.
On 2017/04/04 at 23:07:28, scottchen wrote: > I had an unnecessary trackMouseMove() on my previous example, see updated https://jsfiddle.net/ppLsLz91/7/.
https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:94: if (target.classList.contains('dropdown-item') && does this move if you move the mouse inside of a <paper-checkbox>, like in the languages action menu?
https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:94: if (target.classList.contains('dropdown-item') && On 2017/04/04 23:41:13, Dan Beam wrote: > does this move if you move the mouse inside of a <paper-checkbox>, like in the > languages action menu? yes, it focuses the checkbox (ripple shown around the checkbox).
https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:94: if (target.classList.contains('dropdown-item') && On 2017/04/04 23:48:45, scottchen wrote: > On 2017/04/04 23:41:13, Dan Beam wrote: > > does this move if you move the mouse inside of a <paper-checkbox>, like in the > > languages action menu? > > yes, it focuses the checkbox (ripple shown around the checkbox). don't you actually want it to focus the item, not the element inside the item?
https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:94: if (target.classList.contains('dropdown-item') && On 2017/04/04 23:49:59, Dan Beam wrote: > On 2017/04/04 23:48:45, scottchen wrote: > > On 2017/04/04 23:41:13, Dan Beam wrote: > > > does this move if you move the mouse inside of a <paper-checkbox>, like in > the > > > languages action menu? > > > > yes, it focuses the checkbox (ripple shown around the checkbox). > > don't you actually want it to focus the item, not the element inside the item? The top-most item *is* the <paper-checkbox>, which is the whole row item including the label, and by default the behavior of focusing a paper-checkbox is that the ripple shows around the checkbox. There's not an outer item-container outside the paper-checkbox: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/langua...
On 2017/04/04 23:19:02, dpapad wrote: > On 2017/04/04 at 23:07:28, scottchen wrote: > > > > I had an unnecessary trackMouseMove() on my previous example, see updated > https://jsfiddle.net/ppLsLz91/7/. I think we can even avoid attaching multiple listeners, by attaching a "mouseover" to the cr-action-menu instead, since that will propagate whenever a child receives "mouseenter".
On 2017/04/05 00:42:06, scottchen wrote: > On 2017/04/04 23:19:02, dpapad wrote: > > On 2017/04/04 at 23:07:28, scottchen wrote: > > > > > > > I had an unnecessary trackMouseMove() on my previous example, see updated > > https://jsfiddle.net/ppLsLz91/7/. > > I think we can even avoid attaching multiple listeners, by attaching a > "mouseover" to the cr-action-menu instead, since that will propagate whenever a > child receives "mouseenter". See: https://jsfiddle.net/s0t3v9yg/1/
Just uploaded patch #8 - I took dpapad@'s suggestion fiddle and improved it by using mouseover on the cr-action-menu instead. So now it has the best of both worlds: - Still only use one event handler per-menu, but doesn't fire all day everyday (unlike mousemove) - conditionally attach/remove a mousemove listener only on keydown, so the next-tick mousemove will still steal the focus back. PTAL.
https://codereview.chromium.org/2801453002/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2801453002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:137: menu.flushDebouncer('cr-action-menu-mousemove'); can you replace this with Polymer.dom.flush() or just remove it? https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:92: var target = e.target; i'm still a little confused. I don't think this is bulletproof because it makes assumptions of what can be slotted, which is currently only guaranteed to have a .dropdown-item class, not have shadow DOM (which effects what .target gives us). i'd be more comfortable with something like: var el; for (var i = 0; this.contains(el); el = e.path[i++]) { if (this.options_.indexOf(el) >= 0) { el.focus(); return; } }); https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:94: if (target.classList.contains('dropdown-item') && On 2017/04/05 00:08:19, scottchen wrote: > On 2017/04/04 23:49:59, Dan Beam wrote: > > On 2017/04/04 23:48:45, scottchen wrote: > > > On 2017/04/04 23:41:13, Dan Beam wrote: > > > > does this move if you move the mouse inside of a <paper-checkbox>, like in > > the > > > > languages action menu? > > > > > > yes, it focuses the checkbox (ripple shown around the checkbox). > > > > don't you actually want it to focus the item, not the element inside the item? > > The top-most item *is* the <paper-checkbox>, which is the whole row item > including the label, and by default the behavior of focusing a paper-checkbox is > that the ripple shows around the checkbox. > > There's not an outer item-container outside the paper-checkbox: > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/langua... ok https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:98: this.focus(); // Blur option focus but keep up/down button working. why is this needed? keydown should be fine as long as focus stays inside the dialog. i don't understand how that could change here...
https://codereview.chromium.org/2801453002/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2801453002/diff/140001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:137: menu.flushDebouncer('cr-action-menu-mousemove'); On 2017/04/05 03:29:21, Dan Beam wrote: > can you replace this with Polymer.dom.flush() or just remove it? Yeah, don't need this anymore after removing the debounce code. Nice catch, thanks! https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:92: var target = e.target; On 2017/04/05 03:29:21, Dan Beam wrote: > i'm still a little confused. I don't think this is bulletproof because it makes > assumptions of what can be slotted, which is currently only guaranteed to have a > .dropdown-item class, not have shadow DOM (which effects what .target gives us). > > i'd be more comfortable with something like: > > var el; > for (var i = 0; this.contains(el); el = e.path[i++]) { > if (this.options_.indexOf(el) >= 0) { > el.focus(); > return; > } > }); Acknowledged the concern, I've modified the code to check path. I had to change a couple things from your code: - can't use this.contains(el), since e.path returns the shadowRoots in between elements, and apparently $0.contains($0.shadowRoot) === false. - this.options_ is a NodeList which doesn't have .indexOf() or .contains(). While I *could* convert it to an array and do indexOf, I thought just checking el.classList.contains('dropdown-item') would be sufficient, since this.options_ = querySelectorAll('.dropdown-item') anyway, so it'd be equivalent. https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:98: this.focus(); // Blur option focus but keep up/down button working. On 2017/04/05 03:29:21, Dan Beam wrote: > why is this needed? keydown should be fine as long as focus stays inside the > dialog. i don't understand how that could change here... It's more about blurring the option focus. The comment is more of an explanation of why we can't just do activeElement.blur(). Without this, if you hover over option 1 and move your mouse outside of the menu area, you'd still be focused on option 1 - which debatably is still okay, I just added this to blur from option 1 because: - bug report says "mimic context menu", and context menu does this. - I'd argue this is a better behavior anyway, it might cause confusion if you move your mouse out and option 1 is still hovered. What if the user didn't know and press space/enter/tab?
https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/140001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:98: this.focus(); // Blur option focus but keep up/down button working. On 2017/04/05 20:56:31, scottchen wrote: > On 2017/04/05 03:29:21, Dan Beam wrote: > > why is this needed? keydown should be fine as long as focus stays inside the > > dialog. i don't understand how that could change here... > > It's more about blurring the option focus. The comment is more of an explanation > of why we can't just do activeElement.blur(). > > Without this, if you hover over option 1 and move your mouse outside of the menu > area, you'd still be focused on option 1 - which debatably is still okay, I just > added this to blur from option 1 because: > - bug report says "mimic context menu", and context menu does this. > - I'd argue this is a better behavior anyway, it might cause confusion if you > move your mouse out and option 1 is still hovered. What if the user didn't know > and press space/enter/tab? Also (noting for other readers) we discussed offline this matches the behavior of context menu across all platforms.
https://codereview.chromium.org/2801453002/diff/200001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2801453002/diff/200001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:145: var e = new MouseEvent('mouseover', {bubbles: true}); this is the only code you need
Patchset #11 (id:220001) has been deleted
https://codereview.chromium.org/2801453002/diff/200001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2801453002/diff/200001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:145: var e = new MouseEvent('mouseover', {bubbles: true}); On 2017/04/05 20:59:48, Dan Beam wrote: > this is the only code you need Done.
The CQ bit was checked by scottchen@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/2801453002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2801453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:144: // Moving mouse on the menu (not on option) should focus the menu you should end full sentences with a . https://codereview.chromium.org/2801453002/diff/260001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/260001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:81: this.addEventListener('mousemove', mouseHandler); listenOnce(this, 'mousemove', this.onMouseover_.bind(this)); https://codereview.chromium.org/2801453002/diff/260001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:101: if (el.classList && el.classList.contains('dropdown-item')) { if there's no classList on |el| then it's not actually an element (it seems that path is !Array<!EventTarget>) i suppose this check is fine, but maybe rename |el| to |target|? https://codereview.chromium.org/2801453002/diff/260001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:108: this.focus(); // Blur option focus but keep up/down button working. can we maybe combine both of these comments to: // The user moved the mouse off the options. Reset focus to the dialog.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2801453002/diff/260001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_action_menu_test.js (right): https://codereview.chromium.org/2801453002/diff/260001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_action_menu_test.js:144: // Moving mouse on the menu (not on option) should focus the menu On 2017/04/05 22:00:11, Dan Beam wrote: > you should end full sentences with a . Done. https://codereview.chromium.org/2801453002/diff/260001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/260001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:81: this.addEventListener('mousemove', mouseHandler); On 2017/04/05 22:00:11, Dan Beam wrote: > listenOnce(this, 'mousemove', this.onMouseover_.bind(this)); Awesome, didn't know this existed in the util! https://codereview.chromium.org/2801453002/diff/260001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:101: if (el.classList && el.classList.contains('dropdown-item')) { On 2017/04/05 22:00:11, Dan Beam wrote: > if there's no classList on |el| then it's not actually an element (it seems that > path is !Array<!EventTarget>) > > i suppose this check is fine, but maybe rename |el| to |target|? Yeah console was spewing errors and I found out that path can also contain ShadowRoot which doesn't have classList. Renaming done. Also moving its declaration out of the for-loop - just found out style-guide says one declaration per statement. https://codereview.chromium.org/2801453002/diff/260001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:108: this.focus(); // Blur option focus but keep up/down button working. On 2017/04/05 22:00:11, Dan Beam wrote: > can we maybe combine both of these comments to: > > // The user moved the mouse off the options. Reset focus to the dialog. Done.
The CQ bit was checked by scottchen@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
lgtm but you need to add #util to some compiled_resources2.gyp to appease closure bots https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:97: for (var i = 0; this != target; target = e.path[i++]) { nit: var i = 0; for (var target; this != target; target = e.path[i++]) { ... }
https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:77: listenOnce(this, 'mousemove', this.onMouseover_.bind(this)); We are leaking mousemove listeners. When the user presses for example down, down, down, we are registering 3 of them, but none of them has fired yet. When the user finally moves their mouse, instead of getting one invocation, we get 3 of them. We need to only add a listener if we haven't added one already.
https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:77: listenOnce(this, 'mousemove', this.onMouseover_.bind(this)); On 2017/04/06 00:06:02, dpapad wrote: > We are leaking mousemove listeners. When the user presses for example down, > down, down, we are registering 3 of them, but none of them has fired yet. When > the user finally moves their mouse, instead of getting one invocation, we get 3 > of them. We need to only add a listener if we haven't added one already. sure, that makes sense
https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:77: listenOnce(this, 'mousemove', this.onMouseover_.bind(this)); On 2017/04/06 00:08:35, Dan Beam wrote: > On 2017/04/06 00:06:02, dpapad wrote: > > We are leaking mousemove listeners. When the user presses for example down, > > down, down, we are registering 3 of them, but none of them has fired yet. When > > the user finally moves their mouse, instead of getting one invocation, we get > 3 > > of them. We need to only add a listener if we haven't added one already. > > sure, that makes sense Done. https://codereview.chromium.org/2801453002/diff/280001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:97: for (var i = 0; this != target; target = e.path[i++]) { On 2017/04/05 23:42:30, Dan Beam wrote: > nit: > > var i = 0; > for (var target; this != target; target = e.path[i++]) { > ... > } I changed it to a do-while for readability.
this still lgtm https://codereview.chromium.org/2801453002/diff/320001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/320001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:82: listenOnce(this, 'mousemove', this.onMouseover_.bind(this)); this is fairly similar and easier to comprehend, I think if (!this.hasMousemoveListener_) { this.hasMousemoveListener_ = true; listenOnce(this, 'mousemove', function(e) { this.onMouseover_(e); this.hasMousemoveListener_ = false; }.bind(this)); } plus, it actually requires a 'mousemove' to reset the boolean rather than a 'mouseover' also doing that (was that intentional?)
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
https://codereview.chromium.org/2801453002/diff/320001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/320001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:82: listenOnce(this, 'mousemove', this.onMouseover_.bind(this)); On 2017/04/06 03:03:03, Dan Beam wrote: > this is fairly similar and easier to comprehend, I think > > if (!this.hasMousemoveListener_) { > this.hasMousemoveListener_ = true; > listenOnce(this, 'mousemove', function(e) { > this.onMouseover_(e); > this.hasMousemoveListener_ = false; > }.bind(this)); > } > > plus, it actually requires a 'mousemove' to reset the boolean rather than a > 'mouseover' also doing that (was that intentional?) Not intentional, updated.
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/2801453002/diff/340001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/340001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:80: if (!this.hasMousemoveListener_) { Does this need to be above the following check? Can we move it below that instead? if (e.key !== 'ArrowDown' && e.key !== 'ArrowUp') return;
The CQ bit was checked by scottchen@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by scottchen@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/2801453002/diff/340001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): https://codereview.chromium.org/2801453002/diff/340001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:80: if (!this.hasMousemoveListener_) { On 2017/04/06 18:00:31, dpapad wrote: > Does this need to be above the following check? Can we move it below that > instead? > > if (e.key !== 'ArrowDown' && e.key !== 'ArrowUp') > return; Acknowledged. I moved it into the if(nextOption) statement, so that a listener is attached only when we're about to move the focus().
On 2017/04/06 at 21:40:23, scottchen wrote: > https://codereview.chromium.org/2801453002/diff/340001/ui/webui/resources/cr_... > File ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js (right): > > https://codereview.chromium.org/2801453002/diff/340001/ui/webui/resources/cr_... > ui/webui/resources/cr_elements/cr_action_menu/cr_action_menu.js:80: if (!this.hasMousemoveListener_) { > On 2017/04/06 18:00:31, dpapad wrote: > > Does this need to be above the following check? Can we move it below that > > instead? > > > > if (e.key !== 'ArrowDown' && e.key !== 'ArrowUp') > > return; > > Acknowledged. I moved it into the if(nextOption) statement, so that a listener is attached only when we're about to move the focus(). LGTM
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 scottchen@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/2801453002/#ps380001 (title: "add missing import")
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": 380001, "attempt_start_ts": 1491519954521490,
"parent_rev": "04bec3956c7f19339e9fb1ff04b348922331753d", "commit_rev":
"12399c275d9261a7cb37816faffd1b7e35fbbd6b"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: mouse movements should focus cr-action-menu items BUG=703975 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: mouse movements should focus cr-action-menu items BUG=703975 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2801453002 Cr-Commit-Position: refs/heads/master@{#462674} Committed: https://chromium.googlesource.com/chromium/src/+/12399c275d9261a7cb37816faffd... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:380001) as https://chromium.googlesource.com/chromium/src/+/12399c275d9261a7cb37816faffd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
