912116 - A 6x6-pixel context menu appears when you right-click in a message folder where there are no messages

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Arthur K. (he/him)]

Attachment: small-strange-box.jpg

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:23.0) Gecko/20100101 Firefox/23.0 SeaMonkey/2.20 (Beta/Release)
Build ID: 20130803200314

Steps to reproduce:

I have an GMail account set up via IMAP. One of the sub folders of the [Gmail] folder is called Spam. With no messages in the Spam folder, I right-clicked in the area where messages would normally be and a small context menu (I think) showed up that was 6x6 pixels in dimension.


Actual results:

A small 6x6 box/context menu showed up (see image).


Expected results:

I would think nothing should show when you right-click on blank, empty nothingness.

Comment 1

[Arthur K. (he/him)]

Attachment: Now with drop shadow!

So it's been a while since anything happened with this bug. On TB31 I now see the box has added a drop shadow effect.

Comment 2

[:aceman]

Attachment: patch

This fixes it for me.
The initialization of onEditableArea fixes a "undefined property" warning as the property is queried in the menu even when it is not existing yet.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 8498429 [details] [diff] [review]
patch

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

::: mail/base/content/nsContextMenu.js
@@ +89,5 @@
> +      if (!item.hidden)
> +        someItemsShown = true;
> +    }
> +    if (!someItemsShown)
> +      this.shouldDisplay = false;

You should be able to do this better by some combination of where you are and numSelectedMessages I think?

Comment 4

[Josiah Bruner [:jsbruner]]

Comment on attachment 8498429 [details] [diff] [review]
patch

I don't really think we need a ui-review for this, obviously a 6 by 6 pixel box is bad. :)

Either way though, ui-r+.

Comment 5

[:aceman]

(In reply to Magnus Melin from comment #3)
> You should be able to do this better by some combination of where you are
> and numSelectedMessages I think?

This menu seems to be shown from many different places and on different objects so I would not like to hunt for which contexts produce an empty menu. The function determining that may be so complex that it will become slower than the universal method I proposed. And it would need constant updating of the code as other contexts are added.

Comment 6

[:aceman]

Attachment: patch v1.1

Added an early break so that it is not a big perf hit in the common case.

Comment 7

[Josiah Bruner [:jsbruner]]

Comment on attachment 8499120 [details] [diff] [review]
patch v1.1

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

::: mail/base/content/nsContextMenu.js
@@ +87,5 @@
> +    let someItemsShown = false;
> +    for (let item of contextPopup.children) {
> +      if (!item.hidden) {
> +        someItemsShown = true;
> +        break;

Actually, you could just 'return' here instead. You set someItemsShown to true, so the following if statement will never get called. That's slightly clearer than break statements.

Comment 8

[:aceman]

Attachment: patch v2

Good idea, thanks.

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #5)
> This menu seems to be shown from many different places and on different
> objects so I would not like to hunt for which contexts produce an empty
> menu. 

I don't think there are other places suffering from this, only threadpane + empty folder

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8499128 [details] [diff] [review]
patch v2

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

r=mkmelin

Comment 12

[:aceman]

Thanks.

Comment 13

[Josiah Bruner [:jsbruner]]

https://hg.mozilla.org/comm-central/rev/880a3e26ce32

Comment 15

[Arthur K. (he/him)]

Confirmed. Well done and thanks.