Last Comment Bug 387109 - context menu regressions WRT richlistbox
: context menu regressions WRT richlistbox
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows XP
-- minor (vote)
: ---
Assigned To: Neil Deakin
:
: Neil Deakin
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on: 395426
Blocks: 279703
  Show dependency treegraph
 
Reported: 2007-07-06 07:28 PDT by Simon Bünzli
Modified: 2011-09-29 08:10 PDT (History)
8 users (show)
benjamin: blocking1.9+
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot of the problem with the select boxes (28.44 KB, image/png)
2007-07-23 14:50 PDT, Ria Klaassen (not reading all bugmail)
no flags Details
handle this internally (17.96 KB, patch)
2007-09-07 10:08 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
skip menulists (18.01 KB, patch)
2007-09-17 12:57 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
update with using button 0 for keyboard tests (16.12 KB, patch)
2007-10-03 13:38 PDT, Neil Deakin
bugs: review+
neil: superreview+
dbaron: approval1.9+
Details | Diff | Splinter Review

Description User image Simon Bünzli 2007-07-06 07:28:45 PDT
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 User image Dave Townsend [:mossop] 2007-07-08 03:21:23 PDT
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 User image Ria Klaassen (not reading all bugmail) 2007-07-23 14:01:18 PDT
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 User image Ria Klaassen (not reading all bugmail) 2007-07-23 14:50:16 PDT
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.
Comment 4 User image Simon Bünzli 2007-07-31 17:25:59 PDT
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 User image Simon Bünzli 2007-08-22 06:55:09 PDT
Drivers: This issue affects among others both Add-ons and Downloads Manager.
Comment 6 User image Neil Deakin 2007-09-04 13:30:36 PDT
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 User image Neil Deakin 2007-09-07 10:08:11 PDT
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.
Comment 8 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2007-09-07 12:18:38 PDT
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 User image Neil Deakin 2007-09-07 13:16:02 PDT
(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 User image neil@parkwaycc.co.uk 2007-09-08 06:18:59 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2007-09-10 14:18:04 PDT
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 User image Neil Deakin 2007-09-17 08:23:10 PDT
(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 User image Neil Deakin 2007-09-17 12:57:52 PDT
Created attachment 281222 [details] [diff] [review]
skip menulists
Comment 14 User image Neil Deakin 2007-09-18 05:00:28 PDT
Comment on attachment 281222 [details] [diff] [review]
skip menulists

Actually, I'll update the test now that bug 395426 is done
Comment 15 User image Neil Deakin 2007-10-03 13:38:56 PDT
Created attachment 283422 [details] [diff] [review]
update with using button 0 for keyboard tests
Comment 16 User image neil@parkwaycc.co.uk 2007-10-04 12:52:54 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2007-10-04 15:16:44 PDT
Neil D. could you still explain how bug 279703 caused this?
Comment 18 User image Neil Deakin 2007-10-05 04:24:30 PDT
> 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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2007-10-05 04:32:58 PDT
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 User image David Baron :dbaron: ⌚️UTC-8 2007-10-08 14:16:18 PDT
Comment on attachment 283422 [details] [diff] [review]
update with using button 0 for keyboard tests

a1.9=dbaron
Comment 21 User image Neil Deakin 2007-10-09 10:24:32 PDT
Fixed, but had to disable the test for now due to an issue on Mac.
 

Note You need to log in before you can comment on or make changes to this bug.