As part of the effort around bug 1369693, the first step is to make all toolbar menu buttons that open menus trigger the menu opening on mousedown. Not only this opens up ability to get us to consistent behavior and ability to navigate menus with mousedown/up only, but it also feels much faster.

Comment on attachment 8903504 [details] Bug 1395871 - Open toolbar menus on mousedown, rather than oncommand. https://reviewboard.mozilla.org/r/175340/#review180724 Shouldn't this also update the overflow button? And the page actions button, and the buttons that creates? ::: browser/base/content/browser.xul:945 (Diff revision 1) > <toolbarbutton id="library-button" class="toolbarbutton-1 chromeclass-toolbar-additional" > removable="true" > - oncommand="PanelUI.showSubView('appMenu-libraryView', this, null, event);" > + onmousedown="PanelUI.showSubView('appMenu-libraryView', this, null, event);" > closemenu="none" > cui-areatype="toolbar" > - tooltiptext="&libraryButton.tooltip;" > + tooltiptext="A&libraryButton.tooltip;" ??

Updated all places I could find around main toolbar that open menus. The only place left is the identity box on the left-most side of the URL bar because it has both, click and drag operations and it seems that the dragging has been discussed in bug 587901 7 years ago. :dao was involved. :dao - do you think we still need the code to drag&drop the identity box? If I understand the bug linked above, the reason was to allow for d&d a website onto desktop as a link. I'm not sure if the current visual information in that button is coherent with that operation or if there's a value in supporting it this way.

If we decide to remove the drag&drop and move it to mousedown, the code is here: http://searchfox.org/mozilla-central/rev/2aa0806c598ec433e431728f5ddd3a6847c1a417/browser/base/content/browser.xul#801

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5) > Updated all places I could find around main toolbar that open menus. > > The only place left is the identity box on the left-most side of the URL bar > because it has both, click and drag operations and it seems that the > dragging has been discussed in bug 587901 7 years ago. :dao was involved. > > :dao - do you think we still need the code to drag&drop the identity box? If > I understand the bug linked above, the reason was to allow for d&d a website > onto desktop as a link. I'm not sure if the current visual information in > that button is coherent with that operation or if there's a value in > supporting it this way. You can also drag/drop it to a bookmark, a new tab... it's not just for dragging to the desktop.

Oh, ok. Then taking off NI on :dao, and the patch is ready for review :)

Comment on attachment 8903504 [details] Bug 1395871 - Open toolbar menus on mousedown, rather than oncommand. https://reviewboard.mozilla.org/r/175340/#review180978 I think this still misses the star button, but also I would really like to see a green trypush. I expect you'll need to update some tests, and that might not be entirely trivial because IME button.doCommand() is pretty reliable irrespective of the layout state of the button, but I expect that synthesizing mousedown won't be. ::: browser/base/content/browser.xul:918 (Diff revision 3) > hidden="true" > class="urlbar-icon-wrapper urlbar-page-action" > role="button" > context="pageActionPanelContextMenu" > oncontextmenu="BrowserPageActions.onContextMenu(event);" > onclick="BrowserPageActions.bookmark.onUrlbarNodeClicked(event);"> Why did you not update the star button?

> but also I would really like to see a green trypush Will work on that now then :) > Why did you not update the star button? Because star is a button. According to most HIGs I'm familiar with you don't want to move actions to mousedown. Keeping them on mouseup allows the user to move the mouse outside of the area after mousedown and in result cancel the action. I only wanted to move buttons that do not have any action except of opening the submenu. Do you agree?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10) > > but also I would really like to see a green trypush > > Will work on that now then :) > > > Why did you not update the star button? > > Because star is a button. According to most HIGs I'm familiar with you don't > want to move actions to mousedown. Keeping them on mouseup allows the user > to move the mouse outside of the area after mousedown and in result cancel > the action. > > I only wanted to move buttons that do not have any action except of opening > the submenu. > > Do you agree? By that logic, I think the pocket button should also not have this treatment, and you did change the pocket button, right? Both of these show a panel in addition the action they undertake. I'm fine with keeping or changing both - but they should be consistent.

> By that logic, I think the pocket button should also not have this treatment, and you did change the pocket button, right? Both of these show a panel in addition the action they undertake. I'm fine with keeping or changing both - but they should be consistent. Good point. I think I'll remove the pocket from my patch then. It's easier to add it later if we want.

Comment on attachment 8903504 [details] Bug 1395871 - Open toolbar menus on mousedown, rather than oncommand. https://reviewboard.mozilla.org/r/175340/#review181216 Looks like try has issues in both downloads and webextensions tests... ::: browser/base/content/browser-pageActions.js:363 (Diff revision 4) > if (action.nodeAttributes) { > for (let name in action.nodeAttributes) { > buttonNode.setAttribute(name, action.nodeAttributes[name]); > } > } > - buttonNode.addEventListener("click", event => { > + const activationEvent = action.subview ? "mousedown" : "click"; This does the right thing for now ('send to device' is the only subview-y action right now, I think), but as the bookmarks and pocket case illustrates, it might not in future (it just so happens that bookmarks has a custom implementation and pocket uses an iframe). I don't think we need to do anything about that here, but maybe add a comment explaining that we may want to add an explicit marker to action definitions in the future, either when we get actions with subviews for which we don't want this behaviour, or ones with iframes where we don't.

(In reply to :Gijs (queue backed up, slow) from comment #14) > or ones with iframes where we don't. Err, *do*. Negations are hard.

I divided this into three patches: 1) toolbar buttons 2) CustomizableUI and browserAction 3) pageAction The first one has a green try and is ready for review: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b724f1d2c50d6211088d9c3236f37de026b2423b&selectedJob=128781186 The second one is problematic and causes errors on: browser/components/extensions/test/browser/browser_ext_browserAction_popup_preload.js browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js browser/components/extensions/test/browser/browser_ext_openPanel.js It's a bit tricky for me to debug the tests but it seems that it's something about either not capturing the closing of the subview (??) or somehow triggering an open on close (because the open did both open/close cycle?). I'm going to NI :kmag to take a look since he knows this code. The third one wasn't tested yet.

Kris, do you have any instant idea on why some of the tests struggle with the open/close cycle with my second (customizableUI/browserAction) patch? An example is browser/components/extensions/test/browser/browser_ext_openPanel.js which errors with: Browser Chrome Test Summary Passed: 17 Failed: 1 Todo: 0 Mode: e10s *** End BrowserChrome Test Results *** The following tests failed: 47 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_openPanel.js | message queue is empty - Got ["ready","panel-opened"], expected [] Stack trace: chrome://mochikit/content/browser-test.js:test_is:1007 chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:27 chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest<:416 testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1099:11 run@chrome://mochikit/content/browser-test.js:1036:9 Buffered messages finished SUITE-END | took 6s and browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js which errors with: Browser Chrome Test Summary Passed: 28 Failed: 4 Todo: 0 Mode: e10s *** End BrowserChrome Test Results *** The following tests failed: 65 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js | Should have seen the popupShown count increment. - 0 == 1 - JS frame :: chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js :: assertOnlyOneTypeSet :: line 30 Stack trace: chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js:assertOnlyOneTypeSet:30 chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js:testBrowserActionTelemetryResults:142 66 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js | Should only be 1 collected value. - 0 == 1 - JS frame :: chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js :: assertOnlyOneTypeSet :: line 34 Stack trace: chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js:assertOnlyOneTypeSet:34 chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js:testBrowserActionTelemetryResults:142 67 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_telemetry.js | Should have seen the popupShown count increment. - 0 == 1 - JS frame :: chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_telemetry.js :: assertOnlyOneTypeSet :: line 30 Stack trace: chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_telemetry.js:assertOnlyOneTypeSet:30 chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_telemetry.js:testBrowserActionTelemetryResults:142 68 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_telemetry.js | Should only be 1 collected value. - 0 == 1 - JS frame :: chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_telemetry.js :: assertOnlyOneTypeSet :: line 34 Stack trace: chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_telemetry.js:assertOnlyOneTypeSet:34 chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_telemetry.js:testBrowserActionTelemetryResults:142 Buffered messages finished

Comment on attachment 8904846 [details] Bug 1395871 - Open toolbar menus on mousedown, rather than oncommand. https://reviewboard.mozilla.org/r/176612/#review181668

Ok, I got a verbal agreement from :kmag that he's ok with us moving in this direction. Since the first patch is ready to land, I'm going to wait for a green try and land it as it already improves the UX for some buttons. For the second and third we'll wait for Kris.

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4fe078dc013 Open toolbar menus on mousedown, rather than oncommand.

The try looks good, so I pushed the first patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=abd9306811127426c62e296fdd47818ee87877f7

Regression Bug 1398390 (Right click on Library or Download icon produces TWO menus instead of one) pointed here. Possibly related?

yes, thank you! Taking.

P3 in that we wouldn't block 57 on this.