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

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: pretzer, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {regression})

unspecified
Firefox 30
All
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P3][qa+])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8372267 [details]
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'.
(Reporter)

Comment 1

3 years ago
Created attachment 8372268 [details]
Option 2 mockup
(Reporter)

Updated

3 years ago
Whiteboard: [Australis:P?]
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.
OS: All → Windows XP
Keywords: regression
Whiteboard: [Australis:P?] → [Australis:P3]
(Assignee)

Comment 3

3 years ago
Created attachment 8373279 [details] [diff] [review]
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.
Attachment #8373279 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
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.
Attachment #8373279 - Flags: review?(jaws) → review-
(Assignee)

Comment 5

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

This is better.
Attachment #8374060 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Attachment #8373279 - Attachment is obsolete: true
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 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.
Attachment #8374060 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 8

3 years ago
Created attachment 8374329 [details] [diff] [review]
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...
Attachment #8374329 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Attachment #8374060 - Attachment is obsolete: true
Attachment #8374329 - Flags: review?(jaws) → review+
(Assignee)

Comment 9

3 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/9b5f75a2463d
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
(Assignee)

Comment 10

3 years ago
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...
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
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.
(Assignee)

Comment 12

3 years ago
(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...
(Assignee)

Comment 13

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 971705
(Assignee)

Comment 14

3 years ago
Relanded with bug 971705 to avoid orange,

remote:   https://hg.mozilla.org/integration/fx-team/rev/5f3546636bcc
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
(Assignee)

Comment 15

3 years ago
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
(Assignee)

Updated

3 years ago
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Comment 17

3 years ago
Created attachment 8375560 [details] [diff] [review]
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 .
Attachment #8375560 - Flags: review?(jaws)
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.
Attachment #8375560 - Flags: review?(jaws) → review+
(Assignee)

Comment 19

3 years ago
(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
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e69b0b501564
https://hg.mozilla.org/mozilla-central/rev/4c629a023860
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
Can I assume this (and bug 971705) are things we want on Aurora as well?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 22

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

3 years ago
Whiteboard: [Australis:P3] → [Australis:P3][dont-autoland]
(Assignee)

Comment 23

3 years ago
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
Attachment #8374329 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 24

3 years ago
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
Attachment #8375560 - Flags: approval-mozilla-aurora?
Attachment #8374329 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8375560 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 25

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/77cea7e85efa
https://hg.mozilla.org/releases/mozilla-aurora/rev/eae4e4b72da9
Whiteboard: [Australis:P3][dont-autoland] → [Australis:P3]
(Assignee)

Updated

3 years ago
status-firefox29: --- → fixed
status-firefox30: --- → fixed

Updated

3 years ago
Keywords: verifyme
Whiteboard: [Australis:P3] → [Australis:P3][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).
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.