387109 - context menu regressions WRT richlistbox

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Simon Bünzli]

Of course, these might just be symptoms of an underlying (showPopup; cf. URL) regression. Anyway:

From bug 310474 comment #6:
> 1. Select the first richlistitem and hit [App] or [Shift]+[F10]
> 
> Expected: The context menu opens above or below the item (so that the item
> itself can still be seen).
> Actual: The context menu opens over the item.
> 
> 2. Select the second richlistitem and hit [App]
> 
> Expected: Same as above.
> Actual: The context menu opens in the very same place as the first item (i.e.
> relative to the richlistbox and not the richlistitem).
> 
> 3. Select the first item and move the window to the very bottom of the screen
> before hitting [App]
> 
> Expected: The context menu opens fully visible at the bottom of the screen.
> Actual: The context menu may be partially hidden or not visible at all,
> depending on how long it is and how far below the visible screen the
> richlistbox is located.

Comment 1

[Dave Townsend [:mossop]]

I think this is the same as a bug I'm experiencing with my extensions code. There I call showPopup directly to display a context menu and it appears all the way at the top of the window, not where it used to before the popup rewrite.

Comment 2

[Ria Klaassen (not reading all bugmail)]

I see a problem with the select menus; they open on the wrong spot under certain circumstances. For instance when you specify a dotted outline on focus and then click on the select arrow it opens far away.
I noticed this several times and I could narrow the cause of the problem down to a line in my userContent.css: :focus { outline: 1px dotted black !important;}

Comment 3

[Ria Klaassen (not reading all bugmail)]

Attachment: screenshot of the problem with the select boxes

Sorry Zeniko if this is not the same as or related to your bug.

Comment 4

[Simon Bünzli]

Ria: This bug only about the richlistbox widget which you'll only find in chrome due to the widget being XUL-only. Please file a new bug, if you haven't done so already (and if it wasn't bug 388317).

Comment 5

[Simon Bünzli]

Drivers: This issue affects among others both Add-ons and Downloads Manager.

Comment 6

[Neil Deakin]

There are two issues here:
I'm going to guess that the code in the contextmenu handler in richlistbox.xml is trying to open a keyboard-triggered context menu aligned underneath the current item 

1. The check for (event.button != 2) is always true. It seems that bug 361376 made the as if mouse button was pressed for keyboard context menus.
2. The richlistbox.xml code seems try to open a popup yet seems to not care that the code which normally handles the context attribute also runs. In this case, that happens beforehand and thus the popup has already opened.

The right way to do this is to determine the proper means of distinguishing between mouse and keyboard triggered context menus, then add this code in nsXULPopupListener::LaunchPopup

Comment 7

[Neil Deakin]

Attachment: handle this internally

There already is code to adjust the key-invoked context menu event point to the current item in the tree. This patch extends that behaviour to other lists.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I think I want to change back button to be 0 when context menu is opened
using key. But that doesn't seem to fix this bug.

Comment 9

[Neil Deakin]

(In reply to comment #8)
> Hmm, I think I want to change back button to be 0 when context menu is opened
> using key. But that doesn't seem to fix this bug.
> 

That was of concern for an earlier attempt at this bug, but I don't think that difference affects this bug or patch any more. Point 2 in comment 6 is the main issue.

Comment 10

[[e-mail]]

Comment on attachment 280081 [details] [diff] [review]
handle this internally

>+    nsCOMPtr<nsIDOMXULSelectControlElement> select =
>+      do_QueryInterface(aCurrentEl);
>+    if (select)
>+      select->GetSelectedItem(getter_AddRefs(item));
Won't this do strange things with menulists?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 280081 [details] [diff] [review]
handle this internally

This doesn't seem to fix
 "> 3. Select the first item and move the window to the very bottom of the screen
> before hitting [App]
"

Comment 12

[Neil Deakin]

(In reply to comment #11)
> (From update of attachment 280081 [details] [diff] [review])
> This doesn't seem to fix
>  "> 3. Select the first item and move the window to the very bottom of the
> screen
> > before hitting [App]
> "
> 

That should be filed as a different bug.

Comment 13

[Neil Deakin]

Attachment: skip menulists

Comment 14

[Neil Deakin]

Comment on attachment 281222 [details] [diff] [review]
skip menulists

Actually, I'll update the test now that bug 395426 is done

Comment 15

[Neil Deakin]

Attachment: update with using button 0 for keyboard tests

Comment 16

[[e-mail]]

Comment on attachment 283422 [details] [diff] [review]
update with using button 0 for keyboard tests

>+  else if (focusedContent->Tag() != nsGkAtoms::menulist) {
>+    checkLineHeight = PR_FALSE;
>+    nsCOMPtr<nsIDOMXULSelectControlElement> select =
>+      do_QueryInterface(aCurrentEl);
>+    if (select)
>+      select->GetSelectedItem(getter_AddRefs(item));
>+  }
I'm not sure this is ideal; QI to an nsIDOMXULMenuListElement perhaps?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Neil D. could you still explain how bug 279703 caused this?

Comment 18

[Neil Deakin]

> Neil D. could you still explain how bug 279703 caused this?

The contextmenu handler in richlistbox.xml is attempting to show a popup that is already being opened by the normal handling for the 'context' attribute. Before bug 279703, calling showPopup a second time seemed to just reopen the popup, whereas now this is prevented.

> I'm not sure this is ideal; QI to an nsIDOMXULMenuListElement perhaps?

Sure.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 283422 [details] [diff] [review]
update with using button 0 for keyboard tests

ok, but would be great if someone could come up with a solution to remove all the xul specific code from ELM.

Comment 20

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

Comment on attachment 283422 [details] [diff] [review]
update with using button 0 for keyboard tests

a1.9=dbaron

Comment 21

[Neil Deakin]

Fixed, but had to disable the test for now due to an issue on Mac.