Closed Bug 1322032 Opened 3 years ago Closed 3 years ago

Contacts sidebar: Fix wrong selection and context menu behaviour, and implement sidebar AB context menu for "New Contact / New List"

Categories

(MailNews Core :: Address Book, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 56.0

People

(Reporter: bugzilla2007, Assigned: bugzilla2007)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: polish)

Attachments

(3 files, 22 obsolete files)

10.03 KB, image/png
Details
24.68 KB, image/png
Details
24.24 KB, patch
aceman
: review+
Paenglab
: ui-review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #196135 +++

Contacts sidebar has a number of odd and useless behavious wrt selection and card context menu, violating internal and external ux-consistency. What it doesn't have is a context menu on the results pane itself (the AB whitespace). This bugs seeks to fix the wrong and useless behaviours, and to add a useful pane context menu. If we look at contacts side bar as a miniature AB, it comes as a surprise that whilst you can easily manage all of your existing contacts here, if you want to create a new contact or new list, you get stuck and are practically forced to open the main AB for that. Which obviously violates UX-efficiency quite a lot.

STR

1) In contacts side bar, select any contact(s), e.g. the first.
For comparison, try the same in Windows Explorer to see the correct and useful behaviour.
2a) left-click outside selection.
2a) right-click outside selection
3) Try to create a new card or new list in the address book which is right in front of you.
4) Deselect all contacts, then right-click outside selection, or press context menu button, or F10 (Windows alternative keyboard shortcut for context men).

Actual Result
2a) left-click outside selection should deselect, but doesn't.
2b) right-click outside selection shows the context menu of selection. This is unexpected, because selection context menus should only occur on selections.
3) It's not possible to create a list; creating a contact only very clumsily via File > New > Address Book Contact > then choose the correct addressbook.
Grrrr. Whilst my addressbook where I want to create the new contact is already right in front of me, possibly even with whitespace.
4) Even without selection, we still offer the card context menu when right-clicking outside rows, and all Add-To... commands are actually enabled, but disfunctional as there's no selection. Useless.

Expected Result:

2a) left-click outside selection must deselect. This is useful, important, and totally standard method to get rid of selection, as seen in Windows Explorer.
2b) right-click outside selection mustn't show the context menu of selection, but the context menu of the underlying item instead. In Windows explorer, you get the folder context menu on folder whitespace where you can use NEW to create new files in-place.
3) Contacts side bar is a miniature AB. It's unexpected and it totally sucks that you can't easily create new contacts and lists here, which looks like a very likely action while composing mail. The easiest, non-clutter way of realizing this is a pane context menu on Ab results whitespace.
4) Without selection, obviously we can't show the context menu for selections. Instead, the pane context menu should be shown with useful AB commands like New Contact, New List.
(In reply to Thomas D. (needinfo?me) from comment #0)

> 4) Deselect all contacts, then right-click outside selection, or press
> context menu button, or F10 (Windows alternative keyboard shortcut for
> context menu).

Sorry, that's SHIFT+F10, useful e.g. for keyboards which don't have a context menu key.
Note that from a workflow perspective, at least if you are the organized type and not in a complete rush, it makes SO MUCH more sense to FIRST create a proper new contact in an address book of your choice, rather than relying on TB to dump it in Collected Addresses, with all the known shortcomings wrt First/Last Name when auto-creating new contacts, and then having to go back there and edit and move your contact into the correct AB. Hence ability of creating new contacts from contacts sidebar easily is more than just a convenience feature, it's actually quite essential, and quite a gap now.
Summary: Contacts sidebar: Fix wrong selection and context menu behaviour, and implement sidebar context menu for "New Contact / New List" → Contacts sidebar: Fix wrong selection and context menu behaviour, and implement sidebar AB context menu for "New Contact / New List"
Notes for implementation:

a) Add a bit of extra whitespace below result rows
AB context menu on results pane whitespace is quite good already, helps much and doesn't hurt, but it's not sufficient. Whitespace is not always available, for perfection we should ensure a minimum of whitespace, say one blank row, below results pane card rows. We used to have that in attachment pane, too, until some thoughtless person removed it (but it's different there because you can usually still get it by just expanding composition header pane, but here you can't expand the results view). Maybe Richard could assist with that.

b) Add a [≡] menu button for easy and more discoverable access to the new context menu
Whitespace context menu is a totally standard method for the initiated; but it may not be discoverable enough for the uninitiated. I propose adding a mini [≡] button above the address book picker. That's minimally intrusive yet very helpful for discovery of this new feature; i.e. excellent cost-benefit ratio. If we are brave, we might even think of adding more items to that context menu, e.g. AB properties, print selected card(s) etc. Remember, contacts side bar is nothing but a miniature AB.
Interestingly, the methods for doing this are already present in source code, i.e. this was already there or planned, but not realized in the current UI.

https://dxr.mozilla.org/comm-central/search?q=file%3Aabcontactspanel.js&redirect=false
> function AbPanelNewCard()
> {
>   goNewCardDialog(abList.value);
> }
>
> function AbPanelNewList()
> {
>   goNewListDialog(abList.value);
> }
(In reply to Thomas D. (needinfo?me) from comment #0)

> Expected Result:
> ...
> 2b) right-click outside selection mustn't show the context menu of
> selection, but the context menu of the underlying item instead. In Windows
> explorer, you get the folder context menu on folder whitespace where you can
> use NEW to create new files in-place.

...and right-click outside selection must also deselect, just like left-click ouside selection. Try it in Windows Explorer.
Here we go, working patch on current trunk tip which normalizes the selection and context menu behaviour, and also implements the new AB context menu on results pane whitespace.

I'd need help from Richard to address comment 3 a), and to properly design/align the proposed minimal menu button which makes the new functionality discoverable and accessible.
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #8816752 - Flags: ui-review?(richard.marti)
Attachment #8816752 - Flags: review?(jorgk)
I'm not around this afternoon but you can try this in abContactsPanel.css:

#abResultsTree > .tree-stack > .tree-rows > .tree-bodybox {
  padding-bottom: 10px;
}
Comment on attachment 8816752 [details] [diff] [review]
Patch V.1 - Fix selection behaviour, implement sidebar AB context menu

Review of attachment 8816752 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abContactsPanel.xul
@@ +101,5 @@
>      <separator class="thin"/>
> +    <hbox id="AbPickerHeader" class="toolbar">
> +      <label value="&addressbookPicker.label;" accesskey="&addressbookPicker.accesskey;" control="addressbookList"/>
> +      <spacer flex="1"/>
> +      <label id="AbContextMenuButton" value="≡" style="font-size:1.25em;padding-right:6px;padding-left:6px;" onclick="showSidebarContextMenu(event, [this, 'after_end', 0, 0, false, false]);"/>

If you have an ANSI encoded patch (due to the "ü"), you'll get into trouble with other non-ASCII characters. Plus, no one can import ANSI encoded patches, so I suggest to drop that idea.
(In reply to Jorg K (GMT+1) from comment #8)
> > +      <label id="AbContextMenuButton" value="≡" style="font-size:1.25em;padding-right:6px;padding-left:6px;" onclick="showSidebarContextMenu(event, [this, 'after_end', 0, 0, false, false]);"/>

Hmmm, that "≡" was supposed to be "≡" and I actively tried changing the encoding to UTF-8, but for some reason apparently it didn't save the change.
> 
> If you have an ANSI encoded patch (due to the "ü"), you'll get into trouble
> with other non-ASCII characters. Plus, no one can import ANSI encoded
> patches, so I suggest to drop that idea.

I'm not sure which idea you want me to drop. We've used ANSI encoded patches all the time, isn't it? The problem is now that patch requires UTF-8, but the ü in patch comment requires ANSI so that's the tricky bit. However it has been done before as many correct patches are showing so let's find out how it works. Iirc, ü must look "wrong" on the patch to come out "right" when landing. There are two ways of looking wrong, and I believe one of them used to work. Like inserting an ANSI ü into the UTF-8 patch or so, then it gets mishandled, but lateron coming out correctly again. Unfortunately, encodings are not my field of expertise. I also imagine that while landing the patch, patch comment can be manually edited?
Please supply all patches in UTF-8, it's a pain having to import them manually since |hg qimport bz:NNNN| doesn't work with ANSI imported patches. ANSI is also too risky since a garbled character in a string can just slip in if we're not 100% careful.

In Notepad++: Encoding > Convert to UTF-8 without BOM.
It would be great if you could settle for "Duellmann", otherwise we need to get a Linux/Mac person to land your UTF-8 patches.
OK. UTF-8 + ue

@Richard: one hardcoded style on the ≡, pls remove to css. I failed to move the Address Books label down again, and to align vertically with the ≡ . Those styling issues need the expert. Thanks for providing the whitespace addition, that works perfectly and is nice even to really know that the list is at end here. I just dumped he rule somewhere which looked good, pls verify.
Attachment #8816752 - Attachment is obsolete: true
Attachment #8816752 - Flags: ui-review?(richard.marti)
Attachment #8816752 - Flags: review?(jorgk)
Attachment #8816801 - Flags: ui-review?(richard.marti)
Attachment #8816801 - Flags: review?(jorgk)
Comment on attachment 8816801 [details] [diff] [review]
Patch V.1.1 - Fix selection behaviour, implement sidebar AB context menu

Review of attachment 8816801 [details] [diff] [review]:
-----------------------------------------------------------------

Effects observed with this patch:
1) Left-click in empty area below search results (in empty area)
   clears the selection.
2) Right-click in empty area below search results (in empty area)
   now shows a useful menu, with or without selection.
3) Hamburger menu that lets you added list or contact.

Those are all nice improvements since the current implementation seems a little confused.

Since this patch still needs some input from Richard, I'll look at it when it's done. There are also some nits below.

::: mail/components/addrbook/content/abContactsPanel.js
@@ +16,2 @@
>  {
> +  let paneClick;

Please move this down to where you're using it.

@@ +28,5 @@
>    }
> +  // Click target is gAbResultsTree view (rows or whitespace).
> +  if (target.localName == "treechildren") {
> +    let row = gAbResultsTree.treeBoxObject.getRowAt(aEvent.clientX, aEvent.clientY);
> +    console.log('onclick-row = ' + row);

This needs to go on the final patch.

@@ +33,5 @@
> +    if (row == -1 || row > gAbResultsTree.view.rowCount-1) {
> +      // Click on results tree whitespace.
> +      paneClick = true;
> +    }
> +    if (!paneClick && aEvent.button == 0 && aEvent.detail == 2)

Can you please reorganise the logic here.

if (paneClick) {
 ...
} else {
 ...
}

In fact, you don't need the variable at all and can do this in the preceding if.

@@ +35,5 @@
> +      paneClick = true;
> +    }
> +    if (!paneClick && aEvent.button == 0 && aEvent.detail == 2)
> +      // Double-click on a row: Go ahead and add the entry
> +      addSelectedAddresses('addr_to');

Add braces, this is unreadable with the comment.

@@ +65,5 @@
> +    case "i":
> +      if (aEvent.ctrlKey)
> +        goDoCommand("cmd_properties");
> +      break;
> +    case "F10":

Does this get triggered? I'd hope that Mozilla is smart enough to map this to "ContextMenu".

@@ +73,5 @@
> +      // Shift+F10 is alternative Windows shortcut key for context menu.
> +    case "ContextMenu":
> +      // Show sidebar context menu when there's no selection
> +      if (gAbResultsTree.view.selection.count == 0) {
> +        //aEvent.originalTarget = gAbResultsTree

Left-over test code.
Attachment #8816801 - Flags: review?(jorgk) → feedback+
Comment on attachment 8816801 [details] [diff] [review]
Patch V.1.1 - Fix selection behaviour, implement sidebar AB context menu

ui-r- because the button created from the label doesn't work ui wise with no feedback. Also needs the label a lot of styling to look like a button.

Also I don't like the padding at the bottom of the address list. This padding is on the bottom of the visible tree frame and not at the end of the list. This makes the last row of the view cut off. And I bet shortly after landing this, someone opens a bug because he thinks, something is broken - and it looks like this. We have with this bug the ≡-button which is enough in the case of a full tree.

The not deselecting of a treeitem on clicking on a free area is a global issue of the tree widget. Shouldn't this be fixed globally than here and maybe later on an other place and then an other...?
Attachment #8816801 - Flags: ui-review?(richard.marti) → ui-review-
Attached patch 1322032_sidebarContext.patch (obsolete) — Splinter Review
This patch uses a toolbarbutton for the ≡+button. I only implemented the windows styling. You can do this for Linux and macOS. Then I can test it there.
This patch addresses Jorg's review from comment 12.
I also incorporated Richard's styling on windows (nice!), and the toolbar-button.

Richard, I don't know enough about the stylings in general and on Linux and OSX in particular, where I wouldn't be able to test the outcome so just randomly copying styles feels wrong. Maybe you can do that yourself as the expert in the field...

On windows styling, could you push the buttontext 1 or two px to the left so that it looks center-aligned with the v dropdown arrow of the address book list, but the hover-style should remain right-aligned.
Attachment #8816801 - Attachment is obsolete: true
Attachment #8816930 - Attachment is obsolete: true
Flags: needinfo?(richard.marti)
Comment on attachment 8817412 [details] [diff] [review]
Patch V.2 - (code-cleanup) Fix selection behaviour, implement sidebar AB context menu

Review of attachment 8817412 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I haven't run it in the new version but I will once review is requested.

::: mail/components/addrbook/content/abContactsPanel.js
@@ +37,5 @@
> +      // and show sidebar context menu. This will also trigger on the
> +      // first click of right-dblclick, but that's ok.
> +      gAbView.selection.clearSelection();
> +      showSidebarContextMenu(aEvent);
> +      }

if block not indented.

@@ +57,5 @@
>  }
>  
>  function contactsListOnKeyPress(aEvent)
>  {
>    switch (aEvent.key) {

Maybe a comment here: <alt><enter> and <ctrl>i show the properties. Do we do that elsewhere? I've never used this.

@@ +66,5 @@
> +    case "i":
> +      if (aEvent.ctrlKey)
> +        goDoCommand("cmd_properties");
> +      break;
> +    case "F10":

Repeating question from previous review: Does this get triggered? I'd hope that Mozilla is smart enough to map this to "ContextMenu". I could try it out and I will, but let's hear the author first.
Attachment #8817412 - Flags: feedback+
(In reply to Jorg K (GMT+1) from comment #16)
> Comment on attachment 8817412 [details] [diff] [review]
> Patch V.2 - (code-cleanup) Fix selection behaviour, implement sidebar AB
> context menu
> 
> Review of attachment 8817412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, I haven't run it in the new version but I will once review is
> requested.

Thank u 4 fast feedback and review offer.

> 
> Maybe a comment here: <alt><enter> and <ctrl>i show the properties. Do we do
> that elsewhere? I've never used this.

Yeah, as I explained on respective bugs,
Alt+Enter is default shortcut key for properties on windows. Works on all sorts of items. Where it doesn't work, some windows items even display an apologetic error msg. 
Ctrl+I or Cmd+I respectively is the equivalent default properties shortcut on MAC.
Yes, we have those in main AB, but again not consistently. I think I've already filed respective bugs or comments. I believe it makes sense to allow both shortcuts platform-independently.
I guess the Ctrl+I which I implemented here won't work on MAC as Cmd+I. I can't test that and I'm not much interested (volun~teer's privildege to be selective hehe). If others are missing something on their OS, they are free to add the missing pieces, which will probably require rewriting this as a key (keys have that inbuilt modifier stuff for Win/MAC).

> 
> @@ +66,5 @@
> > +    case "i":
> > +      if (aEvent.ctrlKey)
> > +        goDoCommand("cmd_properties");
> > +      break;
> > +    case "F10":
> 
> Repeating question from previous review: Does this get triggered? I'd hope
> that Mozilla is smart enough to map this to "ContextMenu". I could try it
> out and I will, but let's hear the author first.

Yes and no. Without this switch, the default contextmenu defined on contextmenu attribute of the tree element gets triggered on Shift+F10, probably without custom routines but inbuilt (which is good and practical). However, here we're explicitly checking for event keys. I don't think it would be correct or useful to pretend that Shift+F10 is the same as pressing the contextMenu key on windows keyboards. These are different keys, even if they should do the same. So yes, this gets triggered, and is needed for the current construction to trigger the *non-default* context menu. If there was an easy way we could just hook the default context menu to rows only, that would be cool, but I wouldn't know how, so we are here to disambiguate and either use default for rows clicks or re-route to the other context menu for outside rows clicks.
Added the styles for Linux and macOS. I enlarged the ≡ to 1.5em to have a straight scale to prevent scaling blurriness.
Flags: needinfo?(richard.marti)
(In reply to Thomas D. (needinfo?me) from comment #17)
> I guess the Ctrl+I which I implemented here won't work on MAC as Cmd+I. I
> can't test that and I'm not much interested (volun~teer's privildege to be
> selective hehe). If others are missing something on their OS, they are free
> to add the missing pieces, which will probably require rewriting this as a
> key (keys have that inbuilt modifier stuff for Win/MA)

Hmmm. I just use the proper key element and added keyset around it, then it all works correctly.
So this should be implemented as key to be platform-independent.
Thanks Richard for providing the platform css styling for the new sidebar AB context menu button.

This patch is now feature-complete.

While we are here: Per comment 19, I corrected the shortcut key implementation of cmd_properties, Alt+Enter/Ctrl+I (Windows/Linux) vs. Cmd+I (Mac OS), to be platform-dependent and working correctly on each platform. So this now incorporates a more correct implementation of bug 1320272.

On MAC OS, default keyboard shortcut for cmd_properties is Cmd+I. I'm not sure if the alternative (Windows) keyboard shortcut Alt+Enter has ever worked on MAC OS with the current code, but for now I've excluded that in view of this caveat in documentation:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/key#a-modifiers
> alt: The Alt key. On the Macintosh, this is the Option key. On Macintosh this
> can only be used in conjunction with another modifier, since Alt+Letter
> combinations are reserved for entering special characters in text.

I'd assume that this particular shortcut, Alt+Enter, might still work on MAC without breaking anything, but I can't test, so I omitted it.
Note that keys might trigger more broadly than commands, so our key here might cover the searchbox, where character input using Alt might be needed.
Alt+Enter is probably not used for character input, but I don't know, and well, it's not vital as long as Cmd+I does the trick...
Attachment #8817412 - Attachment is obsolete: true
Attachment #8817646 - Attachment is obsolete: true
Attachment #8817780 - Flags: ui-review?(richard.marti)
Attachment #8817780 - Flags: review?(jorgk)
Same as V.3, with following nitfixes:
- abContextMenuButton now with localized label and tooltip
- general renaming spree (without touching functionality) to replace sidebarContext with abContext.
Attachment #8817780 - Attachment is obsolete: true
Attachment #8817780 - Flags: ui-review?(richard.marti)
Attachment #8817780 - Flags: review?(jorgk)
Attachment #8817791 - Flags: ui-review?(richard.marti)
Attachment #8817791 - Flags: review?(jorgk)
Comment on attachment 8817791 [details] [diff] [review]
Patch V.3.1 - (nitfixes, naming) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

(In reply to Thomas D. (needinfo?me) from comment #20)
> While we are here: Per comment 19, I corrected the shortcut key
> implementation of cmd_properties, Alt+Enter/Ctrl+I (Windows/Linux) vs. Cmd+I
> (Mac OS), to be platform-dependent and working correctly on each platform.
> So this now incorporates a more correct implementation of bug 1320272.
> 
> On MAC OS, default keyboard shortcut for cmd_properties is Cmd+I. I'm not
> sure if the alternative (Windows) keyboard shortcut Alt+Enter has ever
> worked on MAC OS with the current code, but for now I've excluded that in
> view of this caveat in documentation:

On macOS it worked already before with ALT+ENTER and now with CMD+I too. The menuitem still shows Alt+Enter. Alt+Enter should do nothing on macOS.

Sorry to not checking before but right click on empty area shows always both contextmenus on Linux and macOS. Tested with your first patch and it happens too.
Attachment #8817791 - Flags: ui-review?(richard.marti) → ui-review-
Attached image macOSdoubleMenu.png (obsolete) —
Right click in empty area shows both contextmenus.
Comment on attachment 8817791 [details] [diff] [review]
Patch V.3.1 - (nitfixes, naming) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Richard, can you lend a hand to fix this? Would be really need to test which platform we're on?
Attachment #8817791 - Flags: review?(jorgk)
(In reply to Jorg K (GMT+1) from comment #24)
> Comment on attachment 8817791 [details] [diff] [review]
> Patch V.3.1 - (nitfixes, naming) Fix selection behaviour, implement sidebar
> AB context menu, cmd_properties cleanup
> 
> Richard, can you lend a hand to fix this? Would be really need to test which
> platform we're on?

I can help with testing. My knowledge is too low to find the issue.
Does it happen on linux?
Yes, Linux and macOS.
(In reply to Richard Marti (:Paenglab) from comment #22)
> Comment on attachment 8817791 [details] [diff] [review]
> On macOS it worked already before with ALT+ENTER and now with CMD+I too. The
> menuitem still shows Alt+Enter.

How is that possible? The following code only shows/activates Alt+Enter for non-MAC-OS systems, doesn't it?

> +#ifdef XP_MACOSX
> +    <key id="key_properties" modifiers="accel" key="&propertiesCmd.key;"
> command="cmd_properties"/>
> +#else
> +    <key id="key_properties" modifiers="alt" keycode="VK_RETURN" 
> command="cmd_properties"/>
> +    <key id="key_properties2" modifiers="accel" key="&propertiesCmd.key;" 
> command="cmd_properties"/>
> +#endif


> Alt+Enter should do nothing on macOS.

As an UX rule of thumb, I always prefer doing something useful over doing nothing. Does it hurt to allow Alt+Enter as an *alternative* shortcut for cmd_properties on macOS? Anyway, for the moment I took it out, but I'd love to see it back. There might be users swapping between different OS and they'll be grateful if that shortcut which they remember from the other platform just works.

> Sorry to not checking before but right click on empty area shows always both
> contextmenus on Linux and macOS. Tested with your first patch and it happens
> too.

No idea why it would do that only on the non-windows platforms, but I tried to properly suppress the default context menu by combining these now:

aEvent.preventDefault();
aEvent.stopImmediatePropagation();

Beyond that, I wouldn't know what else to do to prevent two context menus from popping up. Are you sure that you had the aEvent.preventDefault(); method included in this function: showAbContextMenu()?
Attachment #8817791 - Attachment is obsolete: true
Attachment #8817816 - Flags: feedback?(richard.marti)
Comment on attachment 8817816 [details] [diff] [review]
Patch V.3.1 - (preventDefault + stopImmediatePropagation) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Sorry, forgot to copy changes from local to patch.
Attachment #8817816 - Flags: feedback?(richard.marti)
See my comment 28.
I also added a handful of return statements to improve performance.

If this succeeds to block double menu, please try the following one by one:

Instead of...
> aEvent.preventDefault; aEvent.stopImmediatePropagation;
...use:
> aEvent.preventDefault; aEvent.stopPropagation;
...then, if that works, test the minimalist solution again:
> aEvent.preventDefault;
Attachment #8817816 - Attachment is obsolete: true
Attachment #8817818 - Flags: feedback?(richard.marti)
Comment on attachment 8817818 [details] [diff] [review]
Patch V.3.2 - (preventDefault + stopImmediatePropagation) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

(In reply to Thomas D. (needinfo?me) from comment #30)
> Created attachment 8817818 [details] [diff] [review]
> Patch V.3.2 - (preventDefault + stopImmediatePropagation) Fix selection
> behaviour, implement sidebar AB context menu, cmd_properties cleanup
> 
> See my comment 28.
> I also added a handful of return statements to improve performance.
> 
> If this succeeds to block double menu, please try the following one by one:

Double menu still shown.
Attachment #8817818 - Flags: feedback?(richard.marti)
I'm having a hard time to imagine why the combination(!) of preventDefault + stopImmediatePropagation would succeed to block the default menu on Windows only, but not on other OS. Wouldn't this imply that the implementation of these DOM methods is broken on those OS?

Or maybe this is a testing artefact or some sort of anomaly or incorrect/incomplete patch application problem on Richard's installations?

Whichever, what is the way forward here? It has worked on my installation (and I can't see a reason why it should not on other OS), and I can't test on other OS. Does anybody apart from Richard see the double context menus?
Flags: needinfo?(jorgk)
I wanted to look at it but can't right now :(
I was going to see what happens on Windows, but the patch doesn't apply any more, most likely after bug 1325924.
Flags: needinfo?(jorgk)
I tried this refreshed patch.

(In reply to Richard Marti (:Paenglab) from comment #23)
> Right click in empty area shows both contextmenus.
Right click in an empty area on Windows shows the "Add to To field" menu, not the "New Contact" one. Do you believe me or do you want to see a screenshot? So somehow the fix go lost since this worked before, IIRC.

Richard, what do you see on Windows?
Attached image WinDoubleMenu.png (obsolete) —
Checked again on all platforms.
Right click on empty area below the last entry shows on all platforms both menu. On Linux and macOS the "New Contact/New List" menu is before the other and on Win10 it's behind and almost not visible.
(In reply to Richard Marti (:Paenglab) from comment #37)
> Oops, completely wrong patch. Now the correct one.
You successfully tricked me, I didn't look at the patch when applying.

(In reply to Richard Marti (:Paenglab) from comment #38)
> On Linux and macOS the "New Contact/New List" menu is before the other
> and on Win10 it's behind and almost not visible.
On Windows 7 the small menu is behind as well. Good that all platforms behave the same, so Thomas can test and fix this.
(In reply to Jorg K (GMT+1) from comment #39)

> (In reply to Richard Marti (:Paenglab) from comment #38)
> > On Linux and macOS the "New Contact/New List" menu is before the other
> > and on Win10 it's behind and almost not visible.
> On Windows 7 the small menu is behind as well. Good that all platforms
> behave the same, so Thomas can test and fix this.

Hmmm. Strangely, in my local install on Windows 10 this is still working perfectly, seeing only the "New Contact/New List" menu and no other menu after right-click on contacts side bar whitespace below contacts. I can't find any difference between patch code and my local install.
So the only difference I could think of is that my local install is slightly older: 51.0a1 (2016-09-05). I'd think it's unlikely that a newer build breaks this - what do you think?

So I'm clueless how to further test or fix this.
I don't know how you work. We all build TB from the source, and we're currently at TB 53 of 29/12/2016.
I think it's generally known that I don't build, and I don't have a build environment set up. Fortunately, no obligations as I'm a volunteer. I'm working on a flattened install, where changing any of the locally installed xul/js files immediately reflects at runtime, which works well for developing/testing. I can't test compiled changes, which is ok because I rarely venture into c++ code. It's a bit hard (albeit rare) to apply the patches of others. I use Tortoise Hg to create my patches, so I get the latest source files from there and inject the new code. That's quite sufficient for my needs, and I haven't had any problems so far to deliver appropriate patches... That said, I should see to it that I can start out from an updated testing installation...
<tree id="abResultsTree" flex="1" context="cardProperties" class="plain" sortCol="GeneratedName" persist="sortCol"
          onclick="contactsListOnClick(event);"

Are we sure that a XUL context menu can be suppressed with a preventDefault() in the onclick handler?

Thomas, do you still have your 51.0a1 environment? What happens if you remove the call to preventDefault()? Do you then get the two menus?
(In reply to Jorg K (GMT+1) from comment #43)

> Thomas, do you still have your 51.0a1 environment? What happens if you
> remove the call to preventDefault()? Do you then get the two menus?

Yes. Removing preventDefault(), even with keeping aEvent.stopImmediatePropagation(), shows original big context menu in front of desired, small context menu.
Thanks. Interesting. preventDefault() is the function to prevent the default, stopPropagation() does something else, I'm not familiar with stopImmediatePropagation(). So only preventDefault() should be used like in the original patch.

From which date is your built? I tried the first patch here the day I gave the f+, 2016-12-04, so unless I'll really old and silly, that worked on that day. I'm pretty sure I tried the right-click in an empty area.

Can you find the regression? With only a month, that would be 5 steps to bisect. So how do you get your flattened install? Download a debug build and unpack the omni.ja? One only needs to add only line preventDefault() into the click-handler to always suppress the context menu and see when that stopped working.
Hey, I'm now doing it the T/D way (no offence intended at all), works nicely.
Downloaded ZIP files of 2016-12-14 and 2016-12-04, unpacked the omni.ja and removed it. TB starts. Nice.
I edited C:\Temp\2016-12-04\thunderbird\chrome\messenger\content\messenger\addressbook\abContactsPanel.js: 
function contactsListOnClick(event)
{
  CommandUpdate_AddressBook();
  event.preventDefault();
and lo and behold, on 4th Dec. no context menu shows, but on 14th Dec. it does.

So I will bisect this quickly to see what's going on.
Last good: 53.0a1 (2016-12-13) (32-bit)
First bad: 53.0a1 (2016-12-14) (32-bit)
on Windows.

Range:
https://hg.mozilla.org/comm-central/pushloghtml?startdate=2016-12-13+00%3A00&enddate=2016-12-14+23%3A00
https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2016-12-13+00%3A00&enddate=2016-12-14+23%3A00
(maybe that can be narrowed down a little looking at the build times).

Strangely Richard complained about a double right-click menu already on the 2016-12-10 (comment #23). So something fishy is going on here.
Maybe a silly proposal. What about using the default menu also for the new menu entries and then hide the not matching entries depending the area of the right click?
Not silly, I thought the same. The current approach is quite simple. Throw up your own small menu and suppress the other one, that's what preventDefault() is about. Why it stopped working, is a bit of a mystery.

Richard, if you want to help us, can you do me a favour. Download the Mac version of Dec. 13. Does it have an omni.ja file? If so, unpack it and delete it. Does TB still start? If so, do the edit described in comment #46. Does the context-menu still show?
I couldn't made it work on macOS but as I wrote this happens on Linux too. So I tested it on a real OS and the menu is still shown on Linux with event.preventDefault(); set. To be sure it's executed I added a dump after this line and this was shown in terminal.
Thanks Richard, but can you please clarify. Which builds/dates are you using? As I said, on Windows, preventDefault() in the click-handler reliably suppressed the context menu until the 13th of Dec. 2016. And what is a "real OS"? Not one of your virtual machines, right?

So can you use Thomas' method of unpacking an omni.ja and applying the change from comment #46 on a build of early Dec.? In fact, I have an old Linux build here which I could try, too.
It was a c-c build from 13.12. downloaded from Mozilla. Still seeing the menu on a 2.12. Daily on Linux.

The macOS is on a real Mac Mini. But I hate when an OS tries to hide everything to the user, like macOS, and he needs to go to the terminal (I admit, I'm a mostly mouse user and too lazy to use the CLI for everything). Okay, on Linux I used the terminal to unzip omni.ja ;-).

I suspect, unpacking omni.ja, and also repacking and exchanging it, doesn't work on macOS because the app is signed and then the checksum isn't correct (I've done this in the past on my older OS X VM's without the signing check).
I'm writing from Linux now where I have a build from the end of September 2016. I added preventDefault() to the click-handler and the context menu comes up just the same. I guess that's what Richard tried to say (and in fact said in the previous comment which arrived while I was writing this).

So in summary: Looks like preventing the default context menu with preventDefault() doesn't work and Windows has been brought inline with the other platforms in early December.

However, I still find this whole thing puzzling. So let's ask a XUL person.

Hi Neil,
first of all, I wish you a happy New Year and thanks for all the support in 2016.

I have a question. We have this bit of XUL:
<tree id="abResultsTree" flex="1" context="cardProperties" class="plain" sortCol="GeneratedName" persist="sortCol"
          onclick="contactsListOnClick(event);"

In the onclick-handler I simply call event.preventDefault(). This does not seem to suppress the context menu on MacOS and Linux, but it did suppress the context menu on Windows until Dec. 13th, 2016. From Dec. 14th, 2016 all platforms behave the same. Two questions:

1) Should it be possible to suppress the context-menu by calling preventDefault() in the onclick-handler?
2) Are you aware of any changes that landed on Dec. 13th/14th, 2016 that changed the behaviour on Windows.
Are we sure .stopPropagation() didn't work?
But the two menus are run from the same element (<tree id="abResultsTree">, but one via context and one via onclick) so there is no propagation to parent elements. And maybe you can't prevent execution of code from one attribute via prevent* call in another attribute. Or maybe you simply can't prevent showing the menu from the special "context" attribute. It may be ignoring the normal event preventing rules. And the "context" menu is shown wherever you click in the whole tree element (so whether there is an AB item (row) or not).

If we can get confirmation from XUL people on this, maybe Paenglab's/Jorg's idea would be the way to go. the items in sidebarAbContextMenu would be part of "cardProperties" menupopup and the contactsListOnClick() would decide which items are relevant to show based on where in the tree the click was done.
(In reply to :aceman from comment #54)
> If we can get confirmation from XUL people on this, maybe Paenglab's/Jorg's
> idea would be the way to go. the items in sidebarAbContextMenu would be part
> of "cardProperties" menupopup and the contactsListOnClick() would decide
> which items are relevant to show based on where in the tree the click was
> done.

Actually contactsListOnClick would only show menu for leftclick events.

And context="cardProperties" would on rightclick (or invoking of context menu) show the menupopup id="cardProperties", however you would add onpopupshowing="filteringFunction()" to that menupopup and the function would show/hide items depending on what was clicked on.
> 1) Should it be possible to suppress the context-menu by calling
> preventDefault() in the onclick-handler?

No, since the click event means the mouse is pressed and released, and context menus on Mac and Linux open on mousedown.

> 2) Are you aware of any changes that landed on Dec. 13th/14th, 2016 that
> changed the behaviour on Windows.

I don't, but we should look for a regression range. Do you have a non-Thunderbird testcase showing the problem?
Flags: needinfo?(enndeakin)
Thank you, Neil.

(In reply to Neil Deakin from comment #56)
> > 2) Are you aware of any changes that landed on Dec. 13th/14th, 2016 that
> > changed the behaviour on Windows.
> I don't, but we should look for a regression range. Do you have a
> non-Thunderbird testcase showing the problem?
I don't. Would you be very unhappy if I didn't try to reproduce this in FF? I'm not the assignee here and I think you have clarified our problem very well in your answer 1) so we know that the suggested approach of displaying our own right-click menu and suppressing the "regular" context menu won't fly. That's all we need to know.
Well could you explain what it is you're trying to do? The patch here doesn't make sense to me; it appears to be manually trying to determine when and how to display a context menu which is almost certainly wrong, and it doesn't contain the example code you gave in comment 53.

The only way to show context menus is using the "context" (or contextmenu") attribute; any other mechanism is certainly going to be wrong in some way.

What is the "regular context menu"? Are you trying to show one set of commands in one case and another set in another situation? The way to do that is to show/hide the commands you want when needed.
Neil, please note that I'm not the patch author here.

<tree id="abResultsTree" flex="1" context="cardProperties" class="plain" sortCol="GeneratedName" persist="sortCol"
          onclick="contactsListOnClick(event);"

is existing code here:
https://dxr.mozilla.org/comm-central/rev/bbb39b5643e82496474bcf9c7dbfaaf34bf0e89f/mail/components/addrbook/content/abContactsPanel.xul#118

The same context menu is displayed regardless of whether the user clicks on a tree element or free-space below. The patch author wants to display two different menus, one, the "regular" context menu, when clicking on a tree element and another one when clicking in free-space.

The free-space menu was "hand-crafted" and needed to cancel the context menu in order to avoid two menus. That didn't work on Mac and Linux, which always showed two menus. It did however work on Windows until the 13th of December 2016 when magically Windows was brought inline with the other two platforms.

Now we know that the approach proposed initially isn't correct and will implement a solution where items are shown/hidden on the "regular/normal/stock-standard/out-of-the-box" context menu.

The only thing that would interest me personally is what changed on the 13th of Dec. 2016 which brought Windows inline with the other platforms. Someone must have fixed this bug:
  "On Windows, the click handler of a XUL tree can cancel the context menu
   by calling preventDefault() (when it shouldn't be allowed to)."

Once again many thanks for looking at it an advising us.
Thank you very much to Jörg and others for the collaborative input, which was helpful to overcome the mysterious change in behaviour.

So here's a new patch which works correctly on my Win 10 installation of TB 53.0a1 (2017-01-04) (64-bit), proving that my original simple approach of having two separate context menus is still 100% feasible after just tweaking it a little - no double menus seen.

I'm now using dedicated oncontextmenu event (new in HTML 5), where preventDefault() obviously works to suppress any context menu attached via default context(menu) attribute. However, I no longer need that because I use customized contextSelection and contextNoSelection attributes only, thus omitting the default context attribute (which globally applies to all the different UI parts of the tree and is thus pretty impractical for a separate menus approach...). I prefer dedicated, separate context menus over a one-for-all mishmash context menu requiring hiding/showing of menu items.

Please note that whichever approach we'd use, we'd have to manually position the AB context menu into the upper-left corner of AB tree widget when it's triggered by keyboard (as I do), which is what made me use openPopup method instead of just using contextmenu attribute directly. I actually tried another approach of swapping the context menu ID in the global contextmenu attribute before opening the context menu, and it worked correctly apart from the context menu positioning problem in case of keyboard triggering.

Please note that this patch also fixes general shortcomings of the XUL tree implementation wrt selection behaviour, and some other unrelated minor touches.

In terms of code cleanup (existing code and new patch code), I was able to remove contactsListOnKeyPress() function entirely by employing/adjusting respective <keys> instead, and condense my target selectors.
Attachment #8822385 - Attachment is obsolete: true
Attachment #8824510 - Flags: ui-review?(richard.marti)
Comment on attachment 8824510 [details] [diff] [review]
Patch V.4 - (oncontextmenu, code cleanup) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Review of attachment 8824510 [details] [diff] [review]:
-----------------------------------------------------------------

This works nicely. However, Neil said in comment #58:
===
The only way to show context menus is using the "context" (or "contextmenu") attribute; any other mechanism is certainly going to be wrong in some way. ... Are you trying to show one set of commands in one case and another set in another situation? The way to do that is to show/hide the commands you want when needed.
===

I'm not expert enough to decide this.

::: mail/components/addrbook/content/abContactsPanel.xul
@@ -114,5 @@
>      </vbox>
>  
>      <separator class="thin"/>
>  
> -    <tree id="abResultsTree" flex="1" context="cardProperties" class="plain" sortCol="GeneratedName" persist="sortCol"

So you really don't want to use the XUL inbuilt context menu and do your own?
Attachment #8824510 - Flags: feedback?(enndeakin)
Attachment #8824510 - Flags: feedback?(acelists)
Comment on attachment 8824510 [details] [diff] [review]
Patch V.4 - (oncontextmenu, code cleanup) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Review of attachment 8824510 [details] [diff] [review]:
-----------------------------------------------------------------

Tested on Linux, macOS and Win10 and works on all platforms.

::: mail/themes/windows/mail/addrbook/abContactsPanel.css
@@ -30,3 @@
>  }
>  
> -@media (-moz-windows-default-theme) {

Don't delete this line.
Comment on attachment 8824510 [details] [diff] [review]
Patch V.4 - (oncontextmenu, code cleanup) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Review of attachment 8824510 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abContactsPanel.xul
@@ -114,5 @@
>      </vbox>
>  
>      <separator class="thin"/>
>  
> -    <tree id="abResultsTree" flex="1" context="cardProperties" class="plain" sortCol="GeneratedName" persist="sortCol"

Yes, I'd also like to know why. Did my proposal not work? The code that you have in contactsListOnContextMenu could be run from onpopupshowing handler on cardProperties menupopup. And merge cardProperties and sidebarAbContextMenu into one menupopup.

On the other hand, oncontextmenu seems to be standard too, but then why do you need the new attributes of contextSelection and contextNoSelection? Seem to be unneeded abstraction. But first try the method above.
(In reply to Jorg K (GMT+1) from comment #61)
> > -    <tree id="abResultsTree" flex="1" context="cardProperties" class="plain" sortCol="GeneratedName" persist="sortCol"
> 
> So you really don't want to use the XUL inbuilt context menu and do your own?

I would have loved to follow what Neils claims to be the "only way to show context menus... using the "context" (or contextmenu") attribute". However, the current implementation of XUL <tree> and other multi-part elements like split menus makes it unnecessarily hard to use that approach. It would be much easier if XUL provided dedicated context menu attributes for the various parts of multi-part elements, e.g. 'contextrows' and 'contextwhitespace' attribute on <treechildren>; same for tooltips by the way (e.g. XUL currently forces having the same tooltip for the main button part and dropdown part of a split menu button, which is obviously not useful).

Let's be honest, XUL is designed to be simple, and requiring any extra script magic to show a simple pre-defined context menu on a clearly distinct UI part adds significant complexity to the code. In an ideal world, the XUL should probably look like this:

(1) <tree>
(2)   <treecols context="columnHeaderContextMenu">           
(3)   <treerows context="rowContextMenu">                   
(4)   <treechildren context="treebodyWhitespaceContextMenu">

(1) <tree context="..."> Having the same context menu for the entire tree widget including column header and whitespace doesn't make sense, hence the unfortunate need for scripting to get this right.
(2) <treecols context="..."> Context menu on column header works as expected, and setting context="" will successfully suppress the global tree context menu on the column header.
(3) <treerows context="..."> Treerows tag doesn't exist, so I can't think of any direct convenient way to add a context attribute which only applies to any <treerow> element, but not to the treebody whitespace. Adding a context menu on every <treerow> isn't efficient, claimed to be not working, and kinda hard when you don't have explicit <treerow> elements because we somehow just connect our row data as a bunch.
(4) <treechildren context="..."> Whoever implemented this tag was not aware of the concept of "treebody whitespace"; by comparison, on modern OS like Windows whitespace has a clearly different function versus the items it contains:
- Clicking on whitespace must deselect contained items.
- Whitespace must have its own context menu.
Both of these are not realized in the current XUL implementation, hence our complications here. Moreover, in my tests, adding context menu attribute on <treechildren> fails to trigger on context menu key, so it's not even usable. The easiest solution to this problem would be to fix the tree selection algorithm and context menu key trigger, and then add a dedicated context menu attribute for treebody whitespace, something like this:
<treechildren context="becomesRowContextMenu" contextwhitespace="treeBodyWhitespaceContextMenu">
Alternatively, and harder, implement a <treerows> tag which allows a dedicated context menu for all rows.

In view of the shortcomings of the current implementation, there's no "clean" solution whereby we could just add context attributes as required and then it just works; one way or the other, we end up scripting around the implementation shortcomings. Two possible solutions have been presented on this bug:

1) Use default context menu attribute on <tree>, global mishmash <menupopup>, add onpopupshowing scripting to disentangle the mishmash menupopup.
+ clean default context tag which gets used
- dirty mishmash menu
- requires scripting to disentangle mishmash menu according to event target

2) Use dedicated custom context menu attributes, separate dedicated <menupopup>s, add oncontextmenu scripting to pick the right context menu according to event target.
+ clean custom contextmenu tags
+ clean separate context menus, easy to maintain/change (e.g. if we want cmd_properties on the AB context menu, we just add a single line to the context menu, no further scripting required).
+ easy to customize by addons
- requires scripting to select the right context menu according to event target

So it becomes a question of philosophy which solution to use; imho, having separate, dedicated context menus which are easy to maintain clearly wins over just using the default context menu attribute for the sake of it, and then having to sort out a messy mishmash context menu in the process. The 'oncontextmenu' event is a reliable standard implementation, so there's nothing wrong with using that.

Finally, as hinted before, please note the following:

The reason for using "openPopup()" instead of just letting the context menu open itself from context menu attribute is NOT the fact that I'm using custom context menu attributes; it's because regardless of which solution we use, we need to reposition the whitespace context menu when it's activated by keyboard; otherwise, it'll show up at the currently focused row which is rather irritating because it's NOT a row context menu, so that kinda spoils the purpose.

It is possible to have clean, separate, easy-to-maintain context menus (as in my solution), use the default context menu attribute (as in Neil's solution), and then use scripting to change the context menu attribute according to event target before the context menu pops up. Then, the current context menu attribute (now filled with a custom context menu attribute value) will actually be used for showing the right context menu. The tricky part is to reposition the context menu to the upper left corner of AB tree element if and only if it's the whitespace context menu triggered by keyboard. I could not find another way for that except using openPopup() method, which in turn will no longer use the default context menu attribute flow to open the context menu. Maybe there are other ways which I haven't tried.

To sum up:
- Due to XUL implementation shortcomings, there's no 100% "clean and simple" solution; any solution has "dirty" (non-orthogonal) notations and requires scripting for what should ideally be simple and straightforward.
- The solution presented in my current patch is technically 100% correct using latest HTML 5 oncontextmenu event, and works 100% correctly on all platforms as confirmed by Richard's comment 62.
- We are also using custom attributes in other scenarios (e.g. dynamic labels), nothing wrong with that, in fact it's quite intelligent, transparent, and easy to maintain.
- Having separate context menus and separate context menu attributes is obviously conceptually easier/cleaner code and easier to |maintain/change / customize by addons| than global mishmash context menu which requires more scripting for any change.

So my current proposal is ready and looks better to me, and I don't see the need to spend more time on implementing this in another way which is just different, but altogether neither better nor "more correct" than mine.
What you write seems plausible, sadly I'm not expert enough to make a decision here.

(In reply to Thomas D. (needinfo?me) from comment #64)
> The reason for using "openPopup()" instead of just letting the context menu
> open itself from context menu attribute is NOT the fact that I'm using
> custom context menu attributes; it's because regardless of which solution we
> use, we need to reposition the whitespace context menu when it's activated
> by keyboard; otherwise, it'll show up at the currently focused row which is
> rather irritating because it's NOT a row context menu, so that kinda spoils
> the purpose.
I tried <shift>F10 while nothing was selected. So the "white-space context menu" is not really popped-up in the white-space below the entries, but instead it covers the column heading and the first entry. That's intentional?
Comment on attachment 8824510 [details] [diff] [review]
Patch V.4 - (oncontextmenu, code cleanup) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Review of attachment 8824510 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abContactsPanel.js
@@ +28,5 @@
> +    contextMenuID = gAbResultsTree.getAttribute("contextNoSelection");
> +    // If "sidebarAbContextMenu" menu was activated by keyboard,
> +    // position it in the topleft corner of gAbResultsTree.
> +    if (!aEvent.button) {
> +      positionArray=[gAbResultsTree, 'topleft', 0, 0, true];

Use overlap instead of topleft. Then the menu is shown at the correct position on rtl locales too.
It would be fairly easy to add a separate context menu attribute for blank non-row areas of the tree. I's always better to add support for something than build complex workarounds.
Comment on attachment 8824510 [details] [diff] [review]
Patch V.4 - (oncontextmenu, code cleanup) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Cancelling ui-r until the comments are addressed and the final implementation is decided.

As first feedback: it looks good until now.
Attachment #8824510 - Flags: ui-review?(richard.marti)
(In reply to Jorg K (GMT+1) from comment #65)
> I tried <shift>F10 while nothing was selected. So the "white-space context
> menu" is not really popped-up in the white-space below the entries, but
> instead it covers the column heading and the first entry. That's intentional?

Yes. It's the same behaviour as in Windows Explorer: You'll only get the context menu on whitespace if you right-click there. For keyboard-only-interaction, the default position of context menus is the upper left corner of the element (or overlap to cater for RTL), and it also covers the header. We don't even know how much whitespace is there, and the context menu would end up in a different position each time, sometimes falsely look associated to contacts, and sometimes very far down, which is unwanted for keyboard use.
Fine, thanks for the explanation. But we still need to decide where we what to go technically, see comment #63 (last sentence) and comment #67. Personally, I have no emotional connection to this, so anything is fine by me.

Aceman, could you assist with doing something in the vein of comment #67. Are we talking about a XUL M-C change here or some tricky customisation (in XML?).
Yes, I can see if it can be added in the XML code of the tree widget.
But I do not see even the existing "context" attribute in https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tree.xml . If it is implemented deeper in the XUL C++ code then we are out of luck.
Flags: needinfo?(acelists)
(In reply to Jorg K (GMT+1) from comment #70)
> Fine, thanks for the explanation. But we still need to decide where we what
> to go technically, see comment #63 (last sentence)

That's answered by my comment 64.

> and comment #67.
> Aceman, could you assist with doing something in the vein of comment #67.
> Are we talking about a XUL M-C change here or some tricky customisation (in
> XML?).

I thought Neil's comment 67 suggests to implement this in XUL M-C, which looks like the right place to do this if the original context attribute is implemented there, per Aceman's comment 71.
So where does that XUL C++ code for trees live?

As for the way forward, I suggest that we land this first as-is (with one nit fixed, overlap), because we never know how long it'll take to get the XUL change right and actually have that available for our purposes.
I volunteer to fix this using the new XUL attributes when they are available, supposing we'll be under no pressure to do the transition because with my patch here it already works.
This patch isn't big, and very concentrated (not distributed), so it's easy to land and change later.

Jörg?
Plus the other nit where I accidentally lost a css row, comment 62.
I thought we do a review first before landing anything and that hasn't been requested yet.

Anyway, I let Aceman decide what he wants to do here. He might even want to take the review.
With nitfixes as requested by Richard.
After review, pls forward to Richard for ui-r.

My proposal for procedure: see comment 72.
Attachment #8817811 - Attachment is obsolete: true
Attachment #8822389 - Attachment is obsolete: true
Attachment #8824510 - Attachment is obsolete: true
Attachment #8824510 - Flags: feedback?(enndeakin)
Attachment #8824510 - Flags: feedback?(acelists)
Attachment #8826860 - Flags: review?(acelists)
Blocks: 1331229
No longer depends on: 196135, 1319409
I filed bug 1331377 on improving the context menu behaviour on trees, in handling both clicking on empty areas, and how the selection is handled.
Depends on: 1331377
(In reply to Neil Deakin from comment #76)
> I filed bug 1331377 on improving the context menu behaviour on trees, in
> handling both clicking on empty areas, and how the selection is handled.

Thanks a lot to Neil for filing bug 1331377, which targets fixing the root causes in XUL <tree> element for the symptoms addressed here in this bug 1322032. I have just commented on bug 1331377 with an intention of clarifying and narrowing down the direction of that bug. I don't think it's totally hard to fix that bug for someone who is intimate with the internals of XUL; however, two months after filing, that bug has seen no activity except my comments today, and is not yet assigned to anybody. The most intuitive solution for that bug, with maximum ease-of-coding for the resulting XUL, is implementing a dedicated context menu attribute for tree-whitespace, e.g.

<tree context="rowContextMenu"
      contextwhitespace="whitespaceContextMenu"/>

Which shouldn't be totally hard to implement into XUL because it should be structurally very much the same as the existing context menu, only with a different spot where it triggers. But it needs careful design and attribute naming to be future-proof (a bold thing to say given the uncertain future of XUL...), so the devil is certainly in the detail.

Bottom line wrt this TB bug 1322032 here, I think that waiting for XUL bug 1331377 will not be a good idea as we have no idea if or when that bug is going to be fixed AND available for TB.

So I'll repeat my suggestion of comment 72, that notwithstanding future improvements via XUL bug 1331377, we should get the current patch of this TB bug 1322032 (attachment 8826860 [details] [diff] [review]) reviewed now and then landed, so that we improve the poor UX for TB users asap. I would hate to see my carefully crafted patch here just rot around for long whilst we continue exposing our users to poor UX in this area, and not improving on the obvious shortcoming that it's way to hard to create a new contact or list from contacts side bar, which looks like a pretty common and basic scenario for what otherwise looks and feels like a miniature AB.

(In reply to Jorg K (GMT+1) from comment #74)
> Anyway, I let Aceman decide what he wants to do here. He might even want to
> take the review.

Aceman?
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #64)
> 1) Use default context menu attribute on <tree>, global mishmash
> <menupopup>, add onpopupshowing scripting to disentangle the mishmash
> menupopup.
> + clean default context tag which gets used
> - dirty mishmash menu
> - requires scripting to disentangle mishmash menu according to event target

But to put this into perspective, in this case you would mark the menuitem elements in the XUL file with an attribute, e.g. contextset="1"
and contextset="2" to indicate which set of items you want (which menu is being opened). Then hiding the unwanted items could be a 1-2 line script code (like for(let item of menulist.querySelectorAll('[contextset="2"]')) item.collapsed=true). Without any need to add new code or hardcode the item IDs when new ones are added to the menu.

> 2) Use dedicated custom context menu attributes, separate dedicated
> <menupopup>s, add oncontextmenu scripting to pick the right context menu
> according to event target.
> + clean custom contextmenu tags
> + clean separate context menus, easy to maintain/change (e.g. if we want
> cmd_properties on the AB context menu, we just add a single line to the
> context menu, no further scripting required).
> + easy to customize by addons
> - requires scripting to select the right context menu according to event
> target

> So it becomes a question of philosophy which solution to use; imho, having
> separate, dedicated context menus which are easy to maintain clearly wins
> over just using the default context menu attribute for the sake of it, and
> then having to sort out a messy mishmash context menu in the process. The
> 'oncontextmenu' event is a reliable standard implementation, so there's
> nothing wrong with using that.

Yes, it is philosophy now. Option 1) is the way this was implemented in TB so far, e.g. in the context menu of a message. Yes, that may be a different case as you always operate on a message just toggle items depending on the message type/attributes. So your case may be different as you click different objects (just in a single tree) so want to open conceptually different menus.

I'm open to trying out your implementation (by landing it) and see how it fares.

> So my current proposal is ready and looks better to me, and I don't see the
> need to spend more time on implementing this in another way which is just
> different, but altogether neither better nor "more correct" than mine.

You are probably right. We have 2 non-ideal solutions to choose from:) Until bug 1331377 is implemented.
Flags: needinfo?(acelists)
Comment on attachment 8826860 [details] [diff] [review]
Patch V.4.1: Patch V.4 - (nitfixes) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Review of attachment 8826860 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.
Thanks for the feature, this seems basically done.
I have only some simple nits to polish it.

On Linux, right click outside the selection does not clear it and still shows the context menu of the selection (case 2b).
But it was your proposal, I won't insist on fixing it. Also the main folder pane tree does behave in this way.

::: mail/components/addrbook/content/abContactsPanel.js
@@ +50,3 @@
>                          kDefaultAscending : kDefaultDescending;
>      SortAndUpdateIndicators(target.id, sortDirection);
> +    return;

This now changes sort direction also on right click, which is a bit unusual. In that case, also the direction arrow does not work right. So Maybe just put the check of event.button != 0 inside the block.

@@ +53,5 @@
>    }
> +  // Click target is gAbResultsTree view (rows or whitespace).
> +  if (target.localName == "treechildren") {
> +    let row = gAbResultsTree.treeBoxObject.getRowAt(aEvent.clientX, aEvent.clientY);
> +    if (row == -1 || row > gAbResultsTree.view.rowCount-1) {

Add spaces, gAbResultsTree.view.rowCount - 1.

@@ +65,5 @@
> +    } else {
> +      // Click on results tree rows.
> +      if (aEvent.button == 0 && aEvent.detail == 2) {
> +        // Double-click on a row: Go ahead and add the entry
> +        addSelectedAddresses('addr_to');

Please change to double quotes when touching this.

::: mail/locales/en-US/chrome/messenger/addressbook/abContactsPanel.dtd
@@ +11,5 @@
>  <!ENTITY contactPropertiesMenu.accesskey    "i">
>  <!ENTITY mailingListPropertiesMenu.label    "Mailing List Properties">
>  <!ENTITY mailingListPropertiesMenu.accesskey "i">
>  
> +<!ENTITY abContextMenuButton.label          "≡">

This special character is irregular in every display I see it, in the patch and also in the UI in the Contacts panel. The spaces between the lines are not the same and it looks ugly to me. Can we use the appmenu hamburger icon here?
Attachment #8826860 - Flags: review?(acelists) → feedback+
(In reply to :aceman from comment #79)
> Comment on attachment 8826860 [details] [diff] [review]
> Patch V.4.1: Patch V.4 - (nitfixes) Fix selection behaviour, implement
> sidebar AB context menu, cmd_properties cleanup
> 
> Review of attachment 8826860 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks!

> On Linux, right click outside the selection does not clear it and still
> shows the context menu of the selection (case 2b).
> But it was your proposal, I won't insist on fixing it. Also the main folder
> pane tree does behave in this way.

It was Neil's and my suggestion to fix that unexpected behaviour on whichever OS it occurs. It's a true surprise and undesirable that there are OS-specific differences in such basic control behaviour.
Any ideas of fixing this welcome.

> ::: mail/components/addrbook/content/abContactsPanel.js
> @@ +50,3 @@
> >                          kDefaultAscending : kDefaultDescending;
> >      SortAndUpdateIndicators(target.id, sortDirection);
> > +    return;
> 
> This now changes sort direction also on right click, which is a bit unusual.
> In that case, also the direction arrow does not work right. So Maybe just
> put the check of event.button != 0 inside the block.

Again, this looks like a platform-specific bug. On my Windows 10, arrow changes correctly with sorting direction, and it's surprising how it could be out of sync.
Yes, I only made right-click change sorting direction according to my policy of "doing something on an unused click is better than doing nothing". But it's a bit unexpected indeed, so it's fine to remove that.
I'd like to show the column picker menu upon right-click on column header, as we do in message list.

> > +<!ENTITY abContextMenuButton.label          "≡">
> 
> This special character is irregular in every display I see it, in the patch
> and also in the UI in the Contacts panel. The spaces between the lines are
> not the same and it looks ugly to me. Can we use the appmenu hamburger icon
> here?

Huh? That must be a display problem on your OS or your system, some sort of artefact (which I suspect would occur for an icon in the same manner). Spaces between the lines are 100% equal. Are you using display zoom factor? We need to find out why it doesn't work for you. Can you post a screenshot? On my Windows 10, this character comes out perfectly, crisp contrast, right size, right line weight (slim/de-emphasized/low salience), and blending in nicely because it has the same line-weight as the font of "Address book" header; as seen in screenshot of attachment 8824650 [details]. We can't use exactly the appmenu hamburger icon because that's far too heavy. I don't see why we should use graphics when a simple character can do the trick nicely. Otherwise, maybe this would be something for Richard to investigate, comment on, and adjust if necessary.
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #80)
> > On Linux, right click outside the selection does not clear it and still
> > shows the context menu of the selection (case 2b).
> > But it was your proposal, I won't insist on fixing it. Also the main folder
> > pane tree does behave in this way.
> 
> It was Neil's and my suggestion to fix that unexpected behaviour on
> whichever OS it occurs. It's a true surprise and undesirable that there are
> OS-specific differences in such basic control behaviour.
> Any ideas of fixing this welcome.

I'd like to hear if this works for Paenglab on some platform.

> > This now changes sort direction also on right click, which is a bit unusual.
> > In that case, also the direction arrow does not work right. So Maybe just
> > put the check of event.button != 0 inside the block.
> 
> Again, this looks like a platform-specific bug. On my Windows 10, arrow
> changes correctly with sorting direction, and it's surprising how it could
> be out of sync.

On right click, arrows are only updated when mouse goes out of the column header box. On left click all works fine. Sure, not your bug.

> Yes, I only made right-click change sorting direction according to my policy
> of "doing something on an unused click is better than doing nothing". But
> it's a bit unexpected indeed, so it's fine to remove that.

Please do remove it. Put back the check to only sort on left click. We do not do it elsewhere and you even say you want it to do something else in the future so it may be better to not introduce only a temporary behaviour.

> I'd like to show the column picker menu upon right-click on column header,
> as we do in message list.

Yes, but that implementation has some comments as maybe it is complicated.
 
> > > +<!ENTITY abContextMenuButton.label          "≡">
> > 
> > This special character is irregular in every display I see it, in the patch
> > and also in the UI in the Contacts panel. The spaces between the lines are
> > not the same and it looks ugly to me. Can we use the appmenu hamburger icon
> > here?
> 
> Huh? That must be a display problem on your OS or your system, some sort of
> artefact (which I suspect would occur for an icon in the same manner).

No, using a font will always cause some surprises and you never get 100% pixel perfect image across platforms to what you expect. There are anti-aliasing, subpixel rendering, stem-darkening and other techniques that may be turned on/off even on the same platform. And they affect the rendering of the character in various ways you can never predict.

Yes, scaling an icon may have the same problem, but I don't think we stale icons.
I'll attach the screenshot and let Paenglab decide.
Flags: needinfo?(richard.marti)
Attachment #8845675 - Flags: feedback?(richard.marti)
Yes, using a font can give aliasing problems.
This patch uses a SVG for the menu button. With this the dimensions are fixed and the lines correctly aligned.
Flags: needinfo?(richard.marti)
Attachment #8845675 - Flags: feedback?(richard.marti)
Yes, that looks fine to me. There is just a warning in the console:
"Unexpected value 0 0 11 11 parsing width attribute. 1 menu.svg"
My bad. I changed something and reverted it not enough.
Attachment #8846057 - Attachment is obsolete: true
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #80)
> > On Linux, right click outside the selection does not clear it and still
> > shows the context menu of the selection (case 2b).
> > But it was your proposal, I won't insist on fixing it. Also the main folder
> > pane tree does behave in this way.
> 
> It was Neil's and my suggestion to fix that unexpected behaviour on
> whichever OS it occurs. It's a true surprise and undesirable that there are
> OS-specific differences in such basic control behaviour.
> Any ideas of fixing this welcome.

I haven't got confirmation that this worked for anyone with the patch. Unless there are differences of implementation of the tree (in c++ xul code) across platforms, there is no code in the patch that would implement this for right click. You only do that for left click (even though the comment says for both, but you run it from contactsListOnClick() which only runs on left click. So I think you have a bug.
I'll upload a new patch with this fixed.
Comment on attachment 8846124 [details] [diff] [review]
1322032_sidebarContext.withSVG.patch

Thanks, no more errors.
Attachment #8846124 - Flags: feedback+
This fixes all the problems I have mentioned (right click on free space and on column header).

I also fixed my nits and added some comments. Thomas please check if you like everything.

I removed the entity with the special character. Please see if that is correct.
Attachment #8826860 - Attachment is obsolete: true
Attachment #8846124 - Attachment is obsolete: true
Attachment #8846888 - Flags: ui-review?(richard.marti)
Attachment #8846888 - Flags: review+
Attachment #8846888 - Flags: feedback?(bugzilla2007)
Comment on attachment 8846888 [details] [diff] [review]
Patch V.4.2: Patch V.4 - (nitfixes) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

ui-r- because:
(In reply to :aceman from comment #79)
> On Linux, right click outside the selection does not clear it and still
> shows the context menu of the selection (case 2b).

This still happens on macOS. Linux and Windows are okay. Without this issue there would be a ui-r+.
Attachment #8846888 - Flags: ui-review?(richard.marti) → ui-review-
Debugged this with Paenglab over IRC and it seems OS X does not return event.detail==1 for single right click. So I changed the condition a bit and it works now.
Attachment #8846888 - Attachment is obsolete: true
Attachment #8846888 - Flags: feedback?(bugzilla2007)
Attachment #8847297 - Flags: ui-review?(richard.marti)
Attachment #8847297 - Flags: review+
Attachment #8847297 - Flags: feedback?(bugzilla2007)
Attachment #8847297 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8847297 [details] [diff] [review]
Patch V.4.3: Patch V.4 - (nitfixes) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Review of attachment 8847297 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for polishing the button and nitfixing faulty behaviour on other OS, good catch, neatly done.
I haven't tested this, but for my next patch I fine-tuned some comments and I've got some questions on behaviour.

::: mail/components/addrbook/content/abContactsPanel.js
@@ +13,5 @@
>  }
>  
> +/**
> + * Determines which context menu to show depending on where in the result tree
> + * the right click was done.

Nit (fixed): we don't just determine, we also show the context menu here (albeit using another helper function), and it's not just clicks (see below).

@@ +15,5 @@
> +/**
> + * Determines which context menu to show depending on where in the result tree
> + * the right click was done.
> + *
> + * @param aEvent  Event of the click.

nit (fixed): oncontextmenu event can also be triggered by context menu key, Shift+F10 on windows, etc. So it's not just clicks, which we need to keep in mind for event handling (see question below).

@@ +28,5 @@
> +
> +  let contextMenuID;
> +  let positionArray;
> +
> +  getClickedResultRow(aEvent);

This function expects a click event. how does it behave when passing context menu key event, or Shift+F10?

@@ +52,5 @@
> + * an invalid row.
> + *
> + * @param aEvent  Event of the click.
> + */
> +function getClickedResultRow(aEvent) {

nit(fixed): Row might be misunderstood as row object, so from our desastrous lazy naming experiences with getSelectedDirectory vs. getSelectedDirectoryIndex, I think its wise to be explicit here and add Index to function name where we mean index.

@@ +53,5 @@
> + *
> + * @param aEvent  Event of the click.
> + */
> +function getClickedResultRow(aEvent) {
> +  let row = gAbResultsTree.treeBoxObject.getRowAt(aEvent.clientX, aEvent.clientY);

We also pass context menu events into this function. How does this behave when you pass in a non-click event like context menu key press, or Shift+F10? (see above)

@@ +68,5 @@
> +
> +/**
> + * Does actions on clicks in the results tree.
> + *
> + * @param aEvent  Event of the click.

Nit (fixed): This isn't a sentence, so no trailing dot, and imo no initial capital either. To be grammatical, English phrases with "xxx of yyy" always require an article in front: the|a xxx of yyy. Without article, it sounds quite odd, even for short notations.
Attachment #8847297 - Flags: feedback?(bugzilla2007) → feedback+
Comment nitfixes, as explained in my review above.
Attachment #8847297 - Attachment is obsolete: true
Attachment #8847998 - Flags: ui-review+
Attachment #8847998 - Flags: review+
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #91)

Thanks for the comment fixes.

> @@ +28,5 @@
> > +
> > +  let contextMenuID;
> > +  let positionArray;
> > +
> > +  getClickedResultRow(aEvent);
> 
> This function expects a click event. how does it behave when passing context
> menu key event, or Shift+F10?

Good catch. It pretends to work. These events also simulate a MouseEvent. However event.detail==1 and event.button==0. As if single left click is done (no right click).

What is worse, the event is on the next row, not the selected one. So selecting row 0 (first one) and pressing menu key (or Shift+F10), returns row 1 from this function. On last row it returns -1 thus clearing selection and showing empty-space menu.

We do not use the returned row for anything yet, the "Add to *" commands still use the real selection, but the clearing of the selection on the last item is bad.
(In reply to :aceman from comment #93)

> What is worse, the event is on the next row, not the selected one. So
> selecting row 0 (first one) and pressing menu key (or Shift+F10), returns
> row 1 from this function. On last row it returns -1 thus clearing selection
> and showing empty-space menu.

Would it work if we just clear the selection in onclick event handler as in my original patch? I am not seeing any of these problems with my patch on Windows 10. So I think if we just add your condition tweak, it should work?
I think that might even be considered the more correct location, because clearing selection should happen on any plain single click outside rows (left or right), not on contextmenu event (which is for handling context menu, not the plain right-click).
You can try it.
Thomas, I haven't exactly understood what you mean so can you modify the existing patch to do what you propose?
Is this ready for checkin or is there more to come?
There is still a problem, starting at comment 93.

We could check how the context menu in the message list works, as that behaves correctly when the menu is invoked by keys. It operates on the right message.
(In reply to :aceman from comment #96)
> Thomas, I haven't exactly understood what you mean so can you modify the
> existing patch to do what you propose?

I can try time permitting...

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #94)
> (In reply to :aceman from comment #93)
> 
> > What is worse, the event is on the next row, not the selected one. So
> > selecting row 0 (first one) and pressing menu key (or Shift+F10), returns
> > row 1 from this function. On last row it returns -1 thus clearing selection
> > and showing empty-space menu.
> 
> Would it work if we just clear the selection in onclick event handler as in
> my original patch? I am not seeing any of these problems with my patch on
> Windows 10. So I think if we just add your condition tweak, it should work?
> I think that might even be considered the more correct location, because
> clearing selection should happen on any plain single click outside rows
> (left or right), not on contextmenu event (which is for handling context
> menu, not the plain right-click).

Here's what I mean:
You have changed the entire logic of my patch 4.1, attachment 8826860 [details] [diff] [review], more than required to address your review issues. I clear the selection for both left and right click in onclick event; you introduced a separate function which imo isn't needed, and you then added that to oncontextmenu and onclick event. I think my approach is more correct, because clearing selection on simple blankspace clicks (left or right) has nothing to do with oncontextmenu.
My suggestion is to keep the structural logic of my patch, apply minimal fixes for your review comment 79, add Richard's svg, and then use the following tweak of yours to make things work (as I believe):

My patch:
> (aEvent.detail == 1 && (aEvent.button == 0 || aEvent.button == 2)
Your condition tweak to make things work on Linux/MAC:
> ((aEvent.detail == 1 && aEvent.button == 0) || aEvent.button == 2)
What's the status here?
Flags: needinfo?(bugzilla2007)
Jörg, thanks for the ping.

As outlined in my comment 94 and comment 99, here's another attempt at fixing the remaining nits.

Paenglab, pls check mail/themes/windows/mail/addrbook/abContactsPanel.css, which wasn't up to date in your last patch afasics. This preserves your (Richard's & Aceman's) condition tweak of comment 90, so you should still get the new AB context menu correctly for right clicks on results list blank space in OS X.

Aceman, even for right click, onclick event will still fire before oncontextmenu event (at least on Windows, can't check other OSs). So wrt blank space right clicks, there's no need to clear the selection in oncontextmenu again (and get those new row index problems of comment 93) after we've already and quite correctly cleared it in the generic onclick event (for both left and right click). Then, if we only use this very specific (ID-bound) and row-based clearance code once, imo there's no point in having yet another helper function. I tried to include all other nits found and fixed so far, pls let me know if I overlooked anything.

Thanks.
Attachment #8847998 - Attachment is obsolete: true
Flags: needinfo?(bugzilla2007)
Attachment #8878831 - Flags: ui-review?(richard.marti)
Attachment #8878831 - Flags: review?(acelists)
There's an odd behaviour which I believe is not caused by our patch here:

1) Place cursor in composition, e.g. To-Recipient field.
2) Click on the new context menu button.
3) Use cursor down, expecting to navigate the context menu which must have focus by definition, as long as it's open.
4) Alternatively, use access keys for context menu commands, e.g. press "c" for "New Contact".

Actual result:

3) nothing
4) key presses end up at cursor position

Expected result:

3) navigate context menu entries.
4) trigger context menu commands.

[OT] Generally speaking, focus is far too sticky in recipient fields.
Unrelated to this bug, there's another odd behaviour which I'd consider a UX bug for keyboard-centric users:
- select one contact in side bar
- trigger "Add to to" (esp. with access key), intending to add more single addresses consecutively
-> Keyboard focus jumps away to recipient area and stuck there. Getting back into the list of contacts using keyboard only is a pita, worse it's required after every adding of contact(s) from the list.
Expected: Use cursor to navigate the list of contacts, press Alt+A to add selected contact, cursor down further in the list, add another one with Alt+A and so on until done. And if double-click is add-to-To, then probably Enter must do the same, in spite of our privacy/error-prevention concerns about defaulting to To-field. Maybe last triggered add-to-... button could become default button until another flavor of adding is picked (between To/Cc/Bcc). Anyway, that's another story. Just for the record while we are here.
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #102)

> [OT] Generally speaking, focus is far too sticky in recipient fields.
> Expected: Use cursor to navigate the list of contacts, press Alt+A to add
> selected contact, cursor down further in the list, add another one with
> Alt+A and so on until done.

Fwiw, in support of the expected UX above, getting OUT of contacts sidebar and back into recipient area is pretty easy using keyboard only: Ctrl+Tab or F6.
Comment on attachment 8878831 [details] [diff] [review]
Patch V.5: (various nitfixes, based on 4.1) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

This doesn't work for me. tested on Linux and Windows I get

ReferenceError: invalid assignment left-hand side[Learn More]  abContactsPanel.js:61:6
ReferenceError: AbPanelLoad is not defined[Learn More]  abContactsPanel.xul:1:1

on opening the Composer. No contacts are shown.
On right click in the empty list shows

ReferenceError: contactsListOnClick is not defined[Learn More]  abContactsPanel.xul:1:1
ReferenceError: contactsListOnContextMenu is not defined[Learn More]  abContactsPanel.xul:1:1

Clicking the Hamburger button shows

ReferenceError: showContextMenu is not defined[Learn More]  abContactsPanel.xul:1:1

The sidebar is now completely unusable.
Attachment #8878831 - Flags: ui-review?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #104)
> Comment on attachment 8878831 [details] [diff] [review]
> 
> ReferenceError: invalid assignment left-hand side[Learn More] 
> abContactsPanel.js:61:6

Sorry, accidental use of "=" instead of "==" in that line, I had even fixed that locally but forgot to backport into the patch. This should fix it. Thanks for rapid response!
Attachment #8878831 - Attachment is obsolete: true
Attachment #8878831 - Flags: review?(acelists)
Attachment #8878871 - Flags: ui-review?(richard.marti)
Attachment #8878871 - Flags: review?(acelists)
Comment on attachment 8878871 [details] [diff] [review]
Patch V.5.1: (various nitfixes, based on 4.1) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup [LANDED with nitfixes, see comment 111, comment 109]

Now it works and looks good. Thanks.
Attachment #8878871 - Flags: ui-review?(richard.marti) → ui-review+
(In reply to Richard Marti (:Paenglab) from comment #89)
> (In reply to :aceman from comment #79)
> > On Linux, right click outside the selection does not clear it and still
> > shows the context menu of the selection (case 2b).
This still happens with the current patch on Linux. Click a contact, then right click empty space in the list. Still context menu for the contact so that you can operate on it even if it isn't even in the view.
Rightclicking another contact moves focus/selection to the new one. Just righclicking empty space does not clear selection.
Are we fine with this behaviour?
In this was I never get the new context menu with New contact/New list. Only if I Ctrl+Click the current selection. But nobody will do that.

On the other hand the patch fixes the Context button and Shift+F10. They open the context menu on the currently selected contact.
Ah yes, this works only on Windows. When this is the only issue, could this be fixed in a new bug? It would be hard for Thomas to fix this as he's on Windows.
Comment on attachment 8878871 [details] [diff] [review]
Patch V.5.1: (various nitfixes, based on 4.1) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup [LANDED with nitfixes, see comment 111, comment 109]

Review of attachment 8878871 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, that would work. I can fix the trivial nits when checking in.

::: mail/components/addrbook/content/abContactsPanel.js
@@ +35,5 @@
> +    contextMenuID = gAbResultsTree.getAttribute("contextNoSelection");
> +    // If "sidebarAbContextMenu" menu was activated by keyboard,
> +    // position it in the topleft corner of gAbResultsTree.
> +    if (!aEvent.button) {
> +      positionArray = [gAbResultsTree, 'overlap', 0, 0, true];

Double quotes for strings.

::: mail/themes/osx/mail/addrbook/abContactsPanel.css
@@ +21,5 @@
> +  margin-top: -4px;
> +  margin-inline-end: 1px;
> +  list-style-image: url("chrome://messenger/skin/addressbook/icons/menu.svg");
> +}
> + 

There is some trailing space here.

::: mail/themes/windows/mail/addrbook/abContactsPanel.css
@@ +42,3 @@
>  #abResultsTree {
>    border-inline-end: none;
> +  border-bottom: 1px solid #a9b1b8;

Why changing the case? Is lowercase the style for the colors?
Attachment #8878871 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #109)
> Comment on attachment 8878871 [details] [diff] [review]
> Patch V.5.1: (various nitfixes, based on 4.1) Fix selection behaviour,
> implement sidebar AB context menu, cmd_properties cleanup
> 
> Review of attachment 8878871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/themes/windows/mail/addrbook/abContactsPanel.css
> @@ +42,3 @@
> >  #abResultsTree {
> >    border-inline-end: none;
> > +  border-bottom: 1px solid #a9b1b8;
> 
> Why changing the case? Is lowercase the style for the colors?

It's more "modern" (and I like it more when it's lowercase). There are still a lot uppercase colors in the tree and I change them only when they are really near the other, needed, changes.
https://hg.mozilla.org/comm-central/rev/8f2f953f2ed32a1eb84344ea69a7cb9d09957301
Landed with small issues fixed.

I took the liberty do change
+    if (row == -1 || row > gAbResultsTree.view.rowCount - 1) {
to
+    if (row < 0 || row >= gAbResultsTree.view.rowCount) {
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
Thanks for reviewing, good catch.
This patch tries another approach, hoping to fix the Linux/Mac issue.

So per Aceman's last review, it still doesn't work on Linux. Which reminded me of this comment:

(In reply to Neil Deakin (not available until Aug 9) from comment #56)
> > 1) Should it be possible to suppress the context-menu by calling
> > preventDefault() in the onclick-handler?
> 
> No, since the click event means the mouse is pressed and released, and
> context menus on Mac and Linux open on mousedown.

So when you right-click on blank space in Linux (and MAC), mousedown triggers the context menu already. So maybe onclick (which would require subsequent mouseup event) never fires after that. So our onclick handler can't clear the selection, because it never gets called. But our contextmenu handler chooses the context menu based on selection, i.e. if there are selected rows or not.

Which makes me think what if we do our clearing of selection in onmousedown already instead of onclick?

***
Please try the new patch and see if it works now on Linux and MAC. It's still working on windows.
***

There's another littel change where I moved AB command updating to the end of those handlers, assuming that commands should be updated after everything else has been done, not before.

As a sidenote, context menu on mousedown in Linux (and MAC), what a weird design! Loads of complaints by programmers found, and all of them ignored by the powers that be.

https://ubuntuforums.org/showthread.php?t=810597
https://bugs.launchpad.net/ubuntu/+source/gtk+2.0/+bug/320259
https://bugzilla.gnome.org/show_bug.cgi?id=575071
https://bugs.chromium.org/p/chromium/issues/detail?id=26465

I especially like this comment from the gnome report:
> and really, no insult, it would be very nice to ask which part of body was
> used to produce idea to show context menu on [mouse] down not up.
;p
Attachment #8882840 - Flags: feedback?(richard.marti)
Attachment #8882840 - Flags: feedback?(acelists)
Attachment #8882840 - Attachment description: Patch V.5.1: (trying to fix MAC/Linux via onmousedown) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup → Patch V.6: (trying to fix MAC/Linux via onmousedown) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup
The previous version already got landed on Aceman's request.

So I can either back that out or you give us a "follow-up" patch to address the remaining Linux/Mac issue.
Why did you lose the svg file from the patch? It doesn't work now.

Did you notice we have landed your previous patch already (with some modifications)?
So you can now either make a new patch on top of the current c-c tree (with your patch included). If it is better for you to make the whole patch again, then please at least incorporate our modifications.
Comment on attachment 8882840 [details] [diff] [review]
Patch V.6: (trying to fix MAC/Linux via onmousedown) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

I backed-out the already checked-in patch to try your new patch. Only tested on macOS and only the right click functionality. There was no change, still needed to left click to unselect a contact.
Attachment #8882840 - Flags: feedback?(richard.marti) → feedback-
Comment on attachment 8882840 [details] [diff] [review]
Patch V.6: (trying to fix MAC/Linux via onmousedown) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup

Review of attachment 8882840 [details] [diff] [review]:
-----------------------------------------------------------------

Also for me there is no change with the context menu on linux.
Attachment #8882840 - Flags: feedback?(acelists) → feedback-
I wasn't aware that the previous patch had already landed when I published the new one for testing.
I have no idea how the svg file got lost, I never touched it.
My Tortoise is stuck, when I try to pull incoming changes from c-c, it asks me to qrefresh first, then I qrefresh (without errors), then I pull again, then it still refuses asking me to qrefresh again.
Anyway, according to Richard, new patch doesn't help on MAC, so I guess we can just call it a day.

Richard, in which order do events fire on macOS? I'm truly surprised why onmousedown handler shouldn't succeed to clear selection before contextmenu gets fired.
Does it run onmousedown handler at all?
If yes, does it get inside the conditional which determines blank space based on clientx/clienty?
I don't know how to check. But as Linux still has the same issue, aceman can surely check this.
Maybe we need something like this?

http://www.toptip.ca/2009/11/javascriptfirefox-context-menu-popup.html
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #117)
> Anyway, according to Richard, new patch doesn't help on MAC, so I guess we
> can just call it a day.

Yes, we offer you to close this bug with the feature fully working on Windows. Linux and Mac at least get the new menu button.
We make a new bug for solving Mac and Linux where we can play with it, you do not need to if you only have Windows.
Blocks: 998312
Blocks: 1320272
Attachment #8878871 - Attachment filename: 1322032_sidebarContext.new3-V5.1.patch → 1322032_sidebarContext.new3-V5.1.patch [LANDED with nits fixed, see comment 111, comment 109]
Attachment #8878871 - Attachment description: Patch V.5.1: (various nitfixes, based on 4.1) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup → Patch V.5.1: (various nitfixes, based on 4.1) Fix selection behaviour, implement sidebar AB context menu, cmd_properties cleanup [LANDED with nitfixes, see comment 111, comment 109]
Attachment #8878871 - Attachment filename: 1322032_sidebarContext.new3-V5.1.patch [LANDED with nits fixed, see comment 111, comment 109] → 1322032_sidebarContext.new3-V5.1.patch
Attachment #8882840 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.