433039 - Right clicking on column header show an irrelevant context menu

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: screenshot

Occurred on Ubuntu Linux while testing Shredder Alpha 1 build 1, with build ID
version 3.0a1 (2008050714).

Try right clicking on the column header while a message is selected; instead of getting a context menu for the column header, I get a context menu for the message instead. See screenshot.

Comment 1

[Magnus Melin [:mkmelin]]

That context menu affects the currently selected email (so not totally irrelevant). What would a context menu for the column header display? I suppose it could just do nothing...

Comment 2

[Tuukka Tolvanen (sp3000)]

#thunderbird tells me a common behavior on windows is to show a column picker menu (bug 148545). That would seem to make a fair bit of sense.

Comment 3

[raoul bhatia]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.2pre) Gecko/20090727 Shredder/3.0b4pre ID:20090727034746

is still showing this very menu. i agree with comment #2 and bug 148545.
(shouldn't this be marked as a dupe of bug 148545 then?)

Comment 4

[Magnus Melin [:mkmelin]]

Well it's two different issue, though they should fix each other, so let's just add a dependency.

Comment 5

[raoul bhatia]

so what will happen in regard to this "issue"?

Comment 6

[Dave]

Attachment: screenshot of missing context menu on thunderbird vs one on OE

Comment 7

[Dave]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.8) Gecko/20100227 Thunderbird/3.0.3

Has this behavior been changed? Now, I don't see any menu - just a small square stub (no messages were selected, see screenshot).

With regards to comment #1: Context actions/menus are usually performed on the object under the mouse cursor in the current context, not the selected item. If you select an object/icon on the Desktop and then right-click on a blank space on the Desktop, the context menu would show actions relevant for the Desktop and not the previously selected object/icon.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

I'm seeing that if there is a selected message in the list you'll get the message context menu when right clicking on the column header.  If nothing is selected in the list (because it's empty or otherwise) then you'll get a very small box as seen in the comparison screenshot.

It makes sense to me that we could at least offer the sort order options as OE does.  We don't have a column dialog so the 'Columns...' option wouldn't make sense for us.  Perhaps other bugs like bug 148545 can explore additional options to add if it seems necessary, which I'm not sure it is.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: proposed fix - wip

This fixes it, but mozmill refuses to right click the header :(

Comment 10

[Ludovic Hirlimann [:Usul]]

Sid ^^^ ?

Comment 11

[Rain [:rain1] (inactive)]

hm, we've been using EventUtils.synthesizeMouse directly for this, apparently. <http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#856> I guess asuth would know.

You might try offsetting the click by a few pixels: <https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#rightClick%28%29>

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Right, the right-click event itself is insufficient.  Someone needs to send a contextmenu event like in the link sid gave.

Originally I assumed I was just hacking around stuff there, but from the recent thread in moz.dev.platform titled "Automate testing of a context menu actions" it would appear that it is, in fact, canon.

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Thx for the tips! Offsetting the rightClick a few px worked.

Comment 14

[Rain [:rain1] (inactive)]

Comment on attachment 439310 [details] [diff] [review]
proposed fix, v2

The context menu showing up on the right edge regardless of where you click feels very wrong, but I'm going to let clarkbw make the call on that.

>   setTarget : function CM_setTarget(aNode) {
...
>+        // The column header was clicked, show the column picker.
>+        let treecols = aNode.parentNode;
>+        let nodeList = document.getAnonymousNodes(treecols);
>+        let treecolpicker;

camelCase please.

>+        let popup = document.getAnonymousElementByAttribute(treecolpicker, "anonid", "popup");
>+        treecolpicker.buildPopup(popup);
>+        popup.showPopup(treecolpicker, -1, -1, "popup", "bottomright", "topright");
>+
>+        this.shouldDisplay = false;

This should be outside the block to retain the semantics for when you're not treecol. Is it possible to come up with a test for this? If so, please add one.

> 
>+// make SOLO_TEST=folder-display/test-right-click-middle-click-messages.js

Please remove this comment.

> /**
>+ * Test that clicking on the column header shows the column picker.
>+ */
>+function test_right_click_column_header_shows_col_picker() {
>+  be_in_folder(folder);
>+
>+  // Right click the subject columen header (must use 10 10 offset to make it work!)
>+  // This should show the column picker popup.
>+  mc.rightClick(mc.eid("subjectCol"), 10, 10);

Is it possible for script blocking to make this asynchronous? Our typical idiom is to spin a loop using waitForEval waiting for the event to happen, which in this case is for the popup to be displayed, and fail if it isn't.

>+
>+  let threadCols = mc.window.document.getElementById("threadCols");
>+  let treecolpicker = mc.window.document.getAnonymousNodes(threadCols).item(1);

- camelCase please.

- It isn't immediately clear what makes item(1) so special. Could you please add a comment explaining this?

>+  let popup = mc.window.document.getAnonymousElementByAttribute(
>+                treecolpicker, "anonid", "popup");
>+  // Check that the popup has been opened (or will be once the request finshes).
>+  if (popup.state != "open" && popup.state != "showing")

Yeah, please spin a loop waiting for the popup to be displayed (until its state is "open") instead of this.

I'd like to have another look at the fix, so minusing.

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to comment #14)
> >+        this.shouldDisplay = false;
> 
> This should be outside the block to retain the semantics for when you're not
> treecol. 

Actually no. This was a bug in the original code, but it didn't matter since shouldDisplay was unused. (Having it inside the block would make us have no thread-pane context menu.)

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Addressing review comments

Comment 17

[Rain [:rain1] (inactive)]

Comment on attachment 439777 [details] [diff] [review]
proposed fix, v3

r+ as long as clarkbw okays the position of the menu. Thanks!

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 439777 [details] [diff] [review]
proposed fix, v3

This menu should be showing up under the mouse.  Other than that this looks great, ui-r+ with that change.

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4

For checkin. 
Carrying forward r=sid0, and adding ui-r=clarkbw (changed the popup position)

Comment 20

[Magnus Melin [:mkmelin]]

changeset:   5463:840db1e6bc6e
http://hg.mozilla.org/comm-central/rev/840db1e6bc6e

->FIXED