969376 - [Australis] Make it easier to hit the menu button (apply Fitts' law)

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Peter Retzer (:pretzer)]

Attachment: Option 1 mockup

The click area of the menu button can be hard to hit at the moment. Applying Fitts' law, like we did for the back button (bug 571454), could improve the usability a lot.

We know the menu button will always be at the far right end of the toolbar, since it cannot be customized to a different location. Therefore it would be nice to increase the click area of the menu button to the right edge of the toolbar.  See the yellow area in 'Option 1 mockup'

Another option would be to increase the click area of the menu button to the respective 'edges' on all four sides. On the left side that would mean increasing it to the vertical separator, that separates the menu button from the rest of the toolbar. If this option is pursued it might make sense to update the hover and click styling of the menu button to better indicate the actual click area. I've made a quick and dirty mockup of how hovering the menu button could look like in 'Option 2 mockup'.

Comment 1

[Peter Retzer (:pretzer)]

Attachment: Option 2 mockup

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

We can do this, but it should only be done for Windows. We can take a similar approach as 571454. It is not a convention on OSX for buttons to be larger than their visual size, as well as clicking on a toolbar on OSX allows the user to drag the application. Linux is different enough that it is not worth doing something special there.

Comment 3

[:Gijs (he/him)]

Attachment: correctly size Australis' menu button, make it hug right border on Windows and Linux,

I realized that the margin was wrong, and then that even as padding it didn't match the spec on Windows. I've checked, and the OS X values are correct. I'm using the Windows values for Linux as well as there's padding on the button and icon there, too.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8373279 [details] [diff] [review]
correctly size Australis' menu button, make it hug right border on Windows and Linux,

This should use -moz-margin/padding-start/end.

Comment 5

[:Gijs (he/him)]

Attachment: correctly size Australis' menu button, make it hug right border on Windows and Linux,

This is better.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8374060 [details] [diff] [review]
correctly size Australis' menu button, make it hug right border on Windows and Linux,

>--- a/browser/themes/linux/customizableui/panelUIOverlay.css
>+++ b/browser/themes/linux/customizableui/panelUIOverlay.css
>@@ -1,14 +1,20 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> %include ../../shared/customizableui/panelUIOverlay.inc.css
> 
>+#PanelUI-menu-button {
>+  /* Override the default styling in the #nav-bar which uses an ID + descendant selector */
>+  -moz-padding-start: 7px !important;
>+  -moz-padding-end: 5px !important;
>+}

This looks like it belongs in browser.css.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8374060 [details] [diff] [review]
correctly size Australis' menu button, make it hug right border on Windows and Linux,

Review of attachment 8374060 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This works great. Although I'm not too sure we should do this for Linux.

Can you please include a test for this? It's one of those things that can break and be broken for a long time before someone notices. I wrote a similar test for the back button in /browser/base/content/test/general/browser_backButtonFitts.js.

Comment 8

[:Gijs (he/him)]

Attachment: correctly size Australis' menu button, make it hug right border on Windows and Linux,

In browser.css, now with a test, still only enabled on Windows because I suspect that whether or not this works on Linux will depend on window manager stuff, so I suspect we shouldn't do that...

Comment 9

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/9b5f75a2463d

Comment 10

[:Gijs (he/him)]

Backed out for Windows-only mochitest-bc orange.

remote:   https://hg.mozilla.org/integration/fx-team/rev/5b9e48b491d0

https://tbpl.mozilla.org/php/getParsedLog.php?id=34518905&tree=Fx-Team#error0


I can reproduce locally (even when running just the test I added, the test in between, and the tabopen_reflow test), but have no idea what's causing this. The panel test runs before the newtab one, and succeeded, apparently, so I'm not sure why it's affecting the other test. Jared, if you have ideas...

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8374329 [details] [diff] [review]
correctly size Australis' menu button, make it hug right border on Windows and Linux,

Review of attachment 8374329 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_menuButtonFitts.js
@@ +29,3 @@
>    });
> +  PanelUI.panel.addEventListener("popupshown", onPopupShown);
> +  EventUtils.synthesizeMouseAtPoint(xPixel, yPixel, {}, window);

Instead of doing all of the onPopupShown/onPopupHidden work, can you add a capturing 'click' event listener to the button that stops propagation and prevents default (and also passes the test)? That should stop the panel from opening which is likely the cause for the reflow test failing.

Comment 12

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #11)
> Comment on attachment 8374329 [details] [diff] [review]
> correctly size Australis' menu button, make it hug right border on Windows
> and Linux,
> 
> Review of attachment 8374329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/general/browser_menuButtonFitts.js
> @@ +29,3 @@
> >    });
> > +  PanelUI.panel.addEventListener("popupshown", onPopupShown);
> > +  EventUtils.synthesizeMouseAtPoint(xPixel, yPixel, {}, window);
> 
> Instead of doing all of the onPopupShown/onPopupHidden work, can you add a
> capturing 'click' event listener to the button that stops propagation and
> prevents default (and also passes the test)? That should stop the panel from
> opening which is likely the cause for the reflow test failing.

I could try, but it doesn't make sense to me that the reflow test fails... the panel should be closed by the time it runs, and even if it wasn't, it shouldn't be resizing (having _syncContainer... called) because of a new tab opening...

Comment 13

[:Gijs (he/him)]

So this is because we're adding the observer for changes in the menu and mostly don't remove it. We shouldn't be doing that. I'll file a dep and put up a patch for review in a sec.

Comment 14

[:Gijs (he/him)]

Relanded with bug 971705 to avoid orange,

remote:   https://hg.mozilla.org/integration/fx-team/rev/5f3546636bcc

Comment 15

[:Gijs (he/him)]

Annnnd backed out again because there's another test that failed (search bar in panel focus yada yada).

remote:   https://hg.mozilla.org/integration/fx-team/rev/dd161d0840c6

Comment 16

[:Gijs (he/him)]

So the second test is failing because when the search code opens the panel, it immediately closes again. This happens before the promise.resolve() executed by PanelUI.js results in the function passed to .then() being executed by the search bar code.

I don't know why the panel is being hidden. Nobody is calling PanelUI.hide (or, by extension, PanelUI.toggle), which also rules out PanelUI's own keypress or mousedown event handlers. Doing a console.trace() (or using a debugger; statement and checking the debugger) from the event handler in PanelUI just shows the event handler, with no other JS on the stack, which I guess means the event was triggered from native code.

The other thing that I've figured out is that this only happens if the test I added and browser_880164_customization_context_menus.js are executed before the search bar test. That test also uses mouse event synthesizing, so I suppose it's related to that aspect of the test, but I haven't been able to figure out why or how.

Comment 17

[:Gijs (he/him)]

Attachment: fix all the tests,

So this fixes a bunch of bogus stuff in our tests. ISTR it was the Promise.jsm removal that seemed to fix the test for me locally. I've pushed the menu button patch plus the test fixes as https://tbpl.mozilla.org/?tree=Try&rev=a94c0aac17ef, and just these test fixes as https://tbpl.mozilla.org/?tree=Try&rev=c1b610479a6b .

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8375560 [details] [diff] [review]
fix all the tests,

Review of attachment 8375560 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but I would like to see some of the PanelUI.toggle calls kept around.

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js
@@ -77,5 @@
>    is(placement.area, CustomizableUI.AREA_PANEL, "Should be in panel");
>  
> -  let shownPanelPromise = promisePanelShown(window);
> -  PanelUI.toggle({type: "command"});
> -  yield shownPanelPromise;

I thought it was nice that this test used PanelUI.toggle as it's another code path that was being tested.

Comment 19

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #18)
> Comment on attachment 8375560 [details] [diff] [review]
> fix all the tests,
> 
> Review of attachment 8375560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me but I would like to see some of the PanelUI.toggle calls kept around.
> 
> :::
> browser/components/customizableui/test/
> browser_880164_customization_context_menus.js
> @@ -77,5 @@
> >    is(placement.area, CustomizableUI.AREA_PANEL, "Should be in panel");
> >  
> > -  let shownPanelPromise = promisePanelShown(window);
> > -  PanelUI.toggle({type: "command"});
> > -  yield shownPanelPromise;
> 
> I thought it was nice that this test used PanelUI.toggle as it's another
> code path that was being tested.

Left those in, shouldn't make a difference as far as I can tell.

remote:   https://hg.mozilla.org/integration/fx-team/rev/e69b0b501564
remote:   https://hg.mozilla.org/integration/fx-team/rev/4c629a023860

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e69b0b501564
https://hg.mozilla.org/mozilla-central/rev/4c629a023860

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Can I assume this (and bug 971705) are things we want on Aurora as well?

Comment 22

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #21)
> Can I assume this (and bug 971705) are things we want on Aurora as well?

Yes.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8374329 [details] [diff] [review]
correctly size Australis' menu button, make it hug right border on Windows and Linux,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis/870605
User impact if declined: users can't hit the menu button using the end-most pixel on the screen on a maximized window on Windows
Testing completed (on m-c, etc.): on m-c, has automated test
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 24

[:Gijs (he/him)]

Comment on attachment 8375560 [details] [diff] [review]
fix all the tests,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: n/a, required test fixes to make the test for the other patch not fall over
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 25

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/77cea7e85efa
https://hg.mozilla.org/releases/mozilla-aurora/rev/eae4e4b72da9

Comment 26

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 5.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 5.2; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0

Verified this issue as fixed on Firefox 29 beta 1 (build ID: 20140318013849) and on latest Firefox Aurora (build ID: 20140318004002).