context menu regressions WRT richlistbox

RESOLVED FIXED

Status

()

Core
XUL
--
minor
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Simon Bünzli, Assigned: Neil Deakin)

Tracking

({regression})

Trunk
x86
Windows XP
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.
Severity: normal → minor
Version: unspecified → Trunk
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.
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;}
Created attachment 273468 [details]
screenshot of the problem with the select boxes

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

Comment 4

10 years ago
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).
(Reporter)

Comment 5

10 years ago
Drivers: This issue affects among others both Add-ons and Downloads Manager.
Flags: blocking1.9?
Assignee: nobody → enndeakin
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 6

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

Updated

10 years ago
Component: XUL Widgets → XP Toolkit/Widgets: Menus
Product: Toolkit → Core
(Assignee)

Comment 7

10 years ago
Created attachment 280081 [details] [diff] [review]
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.
(Assignee)

Updated

10 years ago
Attachment #280081 - Flags: superreview?(neil)
Attachment #280081 - Flags: review?(Olli.Pettay)

Comment 8

10 years ago
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.

Updated

10 years ago
Depends on: 395426
(Assignee)

Comment 9

10 years ago
(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

10 years ago
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

10 years ago
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]
"
(Assignee)

Comment 12

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

Comment 13

10 years ago
Created attachment 281222 [details] [diff] [review]
skip menulists
Attachment #280081 - Attachment is obsolete: true
Attachment #281222 - Flags: superreview?(neil)
Attachment #281222 - Flags: review?(Olli.Pettay)
Attachment #280081 - Flags: superreview?(neil)
Attachment #280081 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 14

10 years ago
Comment on attachment 281222 [details] [diff] [review]
skip menulists

Actually, I'll update the test now that bug 395426 is done
Attachment #281222 - Flags: superreview?(neil)
Attachment #281222 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 15

10 years ago
Created attachment 283422 [details] [diff] [review]
update with using button 0 for keyboard tests
Attachment #281222 - Attachment is obsolete: true
Attachment #283422 - Flags: superreview?(neil)
Attachment #283422 - Flags: review?(Olli.Pettay)

Comment 16

10 years ago
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?
Attachment #283422 - Flags: superreview?(neil) → superreview+

Comment 17

10 years ago
Neil D. could you still explain how bug 279703 caused this?
(Assignee)

Comment 18

10 years ago
> 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

10 years ago
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.
Attachment #283422 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

10 years ago
Attachment #283422 - Flags: approval1.9?
Comment on attachment 283422 [details] [diff] [review]
update with using button 0 for keyboard tests

a1.9=dbaron
Attachment #283422 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 21

10 years ago
Fixed, but had to disable the test for now due to an issue on Mac.
 
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

Updated

9 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xul.widgets → xptoolkit.widgets
(Assignee)

Updated

6 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.