Open Bug 1395871 Opened 2 years ago Updated Last month

Open toolbar menus on mousedown, rather than oncommand

Categories

(Firefox :: Toolbars and Customization, defect, P3)

defect

Tracking

()

Tracking Status
firefox57 --- fix-optional

People

(Reporter: zbraniecki, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open)

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → gandalf
Blocks: 1369693
Status: NEW → ASSIGNED
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;"

??
Attachment #8903504 - Flags: review?(gijskruitbosch+bugs) → review-
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.
Flags: needinfo?(dao+bmo)
(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 :)
Flags: needinfo?(dao+bmo)
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?
Attachment #8903504 - Flags: review?(gijskruitbosch+bugs)
> 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.
Attachment #8903504 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs (queue backed up, slow) from comment #14)
> or ones with iframes where we don't.

Err, *do*. Negations are hard.
Attachment #8903504 - Attachment is obsolete: true
Attachment #8903504 - Flags: review?(gijskruitbosch+bugs)
Attachment #8904845 - Attachment is obsolete: true
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.
Attachment #8904846 - Flags: review?(gijskruitbosch+bugs)
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
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8904846 [details]
Bug 1395871 - Open toolbar menus on mousedown, rather than oncommand.

https://reviewboard.mozilla.org/r/176612/#review181668
Attachment #8904846 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Regression Bug 1398390 (Right click on Library or Download icon produces TWO menus instead of one) pointed here. Possibly related?
yes, thank you! Taking.
Depends on: 1398390
P3 in that we wouldn't block 57 on this.
Priority: -- → P3
Depends on: 1502576
Flags: needinfo?(kmaglione+bmo)

The leave-open keyword is there and there is no activity for 6 months.
:gandalf, maybe it's time to close this bug?

Flags: needinfo?(gandalf)

Nah. It's still there and we should make it consistent.

Assignee: gandalf → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(gandalf)

The leave-open keyword is there and there is no activity for 6 months.
:Gijs, maybe it's time to close this bug?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #35)

The leave-open keyword is there and there is no activity for 6 months.
:Gijs, maybe it's time to close this bug?

No, it's just not done yet.

Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.