342619 - Don't use scroll arrows for context menus when repositioning would allow showing all menu items

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[James B. Higgs]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.4) Gecko/20060516 SeaMonkey/1.0.2 Mnenhy/0.7.4.0
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.4) Gecko/20060516 SeaMonkey/1.0.2 Mnenhy/0.7.4.0

Up and Down scroll arrows on the Right-Click Context Menu for a link is
unacceptable for *any* professional application when there is enough vertical
real estate to display the entire list.


Reproducible: Always

Steps to Reproduce:
1. Right-click on a link with multiple extensions installed.

Actual Results:  
Right-click context menu is cut off when it contains too many items (items
added by extensions), with scroll arrows at top and bottom to allow access to
menu items not visible, even when there is plenty of vertical screen space to
display the entire list.


Expected Results:  
Scroll Arrows should only be displayed if the entire vertical window, from top
to bottom, is insufficient to display the entire list of context menu items.

See heated discussions in Bug #245163

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is definitely a good idea.  ccing some folks who might be interested in making this stuff better.

Comment 2

[Tony Mechelynck [:tonymec]]

Any fix to this bug should of course avoid breaking the fix to bug 245163 when the menus are _not_ displayed in their entirety (see bug 373078).

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Using philor's "Context menu bloater, v.2" extension posted in bug 283679, I'm seeing context menus with as many as nearly 50 items now without scroll arrows. Does that mean this is fixed or is there still an instance where this needs work?

Comment 4

[Phil Ringnalda (:philor)]

No, it's not fixed: you were seeing bug 388112, no scroll arrows ever. This bug is asking for, after the existing steps of "try to fit the whole menu from the click point to the bottom of the display" and "try to fit the whole menu from the click point to the top of the display", a third step before adding scroll arrows, "try to fit the whole menu starting at the top of the display, going to the bottom of the display." So testing requires a context menu more than 50% of your display's height, but less than 100%, and then trigger it in the center of the display.

Comment 5

[RenegadeX]

Attachment: Demonstration of current context menu positioning, dependent on mouse position

As per Faaborg via a recent Mozillazine thread asking for Firefox 3.1 'polish' suggestions (http://forums.mozillazine.org/viewtopic.php?f=23&t=938165) - I'm:
- posting a screenshot of this Bug's issue
- requesting someone with proper Bug permissions to cc: [e-mail] to the Bug if not already done,
- and to same, to add the following Keywords to this bug (less single quotes): '[polish-hard]', '[polish-visual]', '[polish-interactive]' and '[polish-high-visibility]'

Comment 6

[RenegadeX]

.. oh, I see I can now add faaborg to the cc list, so have just done so.

Comment 7

[Tony Mechelynck [:tonymec]]

keyword, see comment #5. Only one "polish" keyword is defined, see https://bugzilla.mozilla.org/describekeywords.cgi for the full list with descriptions. If you want more freedom than offered by available keywords, maybe the Whiteboard is what you want.

Comment 8

[RenegadeX]

(In reply to comment #7)
> maybe the Whiteboard is what you want.

Oh damn, you're right.. it was Whiteboard terms I wanted, not Keyword.
Sorry.

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

>Only one "polish" keyword is defined

Here is the descriptions of the whiteboard terms we are starting to use to track polish problems:

http://blog.mozilla.com/faaborg/2008/10/28/polishing-firefox-week-n-special-edition/

RenegadeX: thanks for the really detailed screen shots and descriptions.

While we still need to fix this for Core, I really hope we never end up with a context menu in Firefox that has so many items that this becomes an issue :)

Comment 10

[RenegadeX]

(In reply to comment #9)
> While we still need to fix this for Core, I really hope we never end up with a
> context menu in Firefox that has so many items that this becomes an issue :)

Your smiley face leaves me puzzled.. either:
- a) you are just making light of the fact that the many Firefox users *are already* affected by a poorly-positioned/poorly designed context menu, and have been for over 4½-years (see Bug 245163.. same root cause as this Bug);
- b) you personally don't have many extensions installed that have added items to the context menu, and therefore assume that just because the Bug doesn't affect you, it doesn't affect any/many Firefox users.. but, aware of the poor design, agree that it is still a Bug that should, at some point, be fixed;
- c) your comment was referring to the 'default' extension-less context menu, in which case you are correct and I would agree with you, though the Bug complaint clearly indicates that the problem is evident only due to the installation of multiple extensions.

Incidentally, my demonstration screenshot graphic (attachment posted w Comment #5), shows that just 13 'extra' context menu items were needed (at my system's screen resolution) in order for the Bug to be apparent.

That's not many - currently, my primary Firefox Profile adds 27 extra items!

Final thing: I noticed a TYPO in the text I'd written on that attached demo graphic -- at the bottom, while mentioning Internet Explorer's context menu drawing method, I wrote that its context menu is ".. drawn up however best fits the situation" -- the word 'up' should have been omitted.. as in IE, the context menu is drawn up, down, or up AND down from the mouse pointer's location.

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

>- c) your comment was referring to the 'default' extension-less context menu,
>in which case you are correct and I would agree with you

yeah it was, the smiley face was meant more of "if we add that many context menu items by default, we've got bigger problems with the design."

>though the Bug
>complaint clearly indicates that the problem is evident only due to the
>installation of multiple extensions.

yep, which is why we still need to fix this.

Comment 12

[Neil Deakin]

Attachment: patch

Now that bug 279539 is done, this is a fairly easy fix. Generally, the idea is that for context menus:

- if a context menu would fit as is, use that position and size
- if there is enough space if the menu was flipped up, do that
- otherwise, simply push the menu upwards

Also fixes a bug where a size wasn't being initialized causing the available space calculations to be wrong sometimes.

Tests to come.

Comment 13

[Neil Deakin]

Attachment: with tests

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        if (mIsContextMenu)
+          aScreenPoint = aScreenEnd - aSize;
+        else
+          popupSize = aScreenEnd - aScreenPoint;

+          if (!mIsContextMenu)
+            popupSize = aAnchorBegin - aScreenPoint - aMarginBegin;

{} around these conditional statements.

So with this patch, we never shrink a context menu at all. Is that reasonable? I was sort of expecting there to still be a fourth step that would shrink the menu to the screen height if necessary.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 355404 [details] [diff] [review]
with tests

overall approach sounds great.

Comment 16

[Neil Deakin]

> So with this patch, we never shrink a context menu at all. Is that reasonable?
> I was sort of expecting there to still be a fourth step that would shrink the
> menu to the screen height if necessary.

Menus are shrunk down to be no larger than the screen size just before FlipOrResize is called.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 355404 [details] [diff] [review]
with tests

ok, looks good modulo the {} issue

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This likely caused the orange on Linux mozilla-central unit test

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** 856 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_largemenu.xul | context menu enough space below left - got -389, expected 11

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, looks like a fix was checked in. Please add comments the tinderbox if you find that you cause a regression. Makes life much easier if we know that the problematic patch is known and that someone is working on a fix (or in this case that a fix was already checked in). Hanging out on irc is also appreciated of course.

Comment 21

[Neil Deakin]

Attachment: fix test on linux

Comment 22

[Neil Deakin]

Comment on attachment 355766 [details] [diff] [review]
fix test on linux

The test failure was caused by bug 472675.

Comment 23

[Neil Deakin]

Checked in the linux test fix.

Comment 24

[Shawn Wilsher :sdwilsh]

These don't apply on branch, but we'd like to fix this polish bug on branch.  Any chance we could get a branch patch for approval?

Comment 25

[Neil Deakin]

Not unless you want to check in 279539 as well.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 355404 [details] [diff] [review]
with tests

I'm confused by this approval request given that there isn't one on bug 279539, but comment 25 says we'd need to take it as well.  I'm also highly skeptical of approval requests that don't appear to have the support of the patch author, so I'm curious what Neil thinks.  If Neil is in favor of taking this on branch, I'd prefer if he request approval for both bugs, so denying approval for now, but feel free to re-request.

Comment 27

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P3 - Polish issue that is in a secondary interface, occasionally encountered, or is not easily identifiable.

Secondary UI, and slightly more uncommon than normal context menus since the mouse has to be in a certain regions of the screen to trigger the bug.