1138478 - 'Write' toolbar button disabled/greyed out after opening the menus in the Saved Files tab

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thunderbird Daily 20150302 on Windows 8.1 (also on Earlybird)

The 'Write' toolbar button is disabled after opening the menus in the Saved Files tab while being offline.

Steps to reproduce:
1. Set Daily to offline (I start it up like this).
Actual and expected result: 'Write' button is enabled
2. Open Tools > Saved Files.
3. In the new tab, click on the menu and move the mouse over all the menus.
4. Switch back to the tab with the 3-pane view or close the tab.
Actual result: 'Write' button disabled.
Expected result: 'Write' button enabled.

Comment 1

[Thomas D. (:thomas8)]

Confirming (I got there without offline).

However, I suppose that menu in the "Saved Files" tab should not even be there, so at least for that case it's temporary. But there might be other similar cases where menus are involved and it could happen there. So we'd want to find out why the write button is *ever* disabled while it should *never* be disabled when there are accounts. Probably a bit of lazyness where we fail to check if it's really the main 3pane in our tests for "write" enabling.

Comment 2

[Thomas D. (:thomas8)]

I'm not even sure if the "Saved Files" tab was intentional at this stage, because bug 724262 is still open...

Comment 3

[Richard Marti (:Paenglab)]

It's intentional. See bug 1087233.

Comment 4

[:aceman]

Yes I can reproduce too. The toolbar button (but also file->new->message) get explicit disabled=true attribute.

I can't find a place that sets it like that as isEnabled("cmd_newMessage") is intended to always return true in command controllers.

I wonder which controller is governing the menus in when the "saved files" tab is open as many items are disabled at that time.

Comment 5

[Thomas D. (:thomas8)]

I can't see any purpose of having the full-blown menu, but 99% disabled, in the Saved Files Tab.
So we can just remove the menu bar entirely for "Saved Files" tab, and that will most likely work around this bug for this particular case (but we should seek to understand this better because there might be other similar cases, even stand-alone msg windows etc.).

Not being able to use the "Write" button for an entire session without recovery is "major" at least.

Comment 6

[Thomas D. (:thomas8)]

:hiro, this looks like a regression from bug 1138478; could you look into this? Tia! :)

Comment 7

[:aceman]

On the other hand, adding full new infrastructure to hide menus (as there isn't one) for this special case is also not very productive IF the current infrastructure for disabling of the menuitems works correctly. It seems to do that EXCEPT for the one case of Write item.

Comment 8

[Kent James (:rkent)]

Yes this should be fixed, but please not by removing the menubar. There is a good reason to have a stable UI element, even if most of the elements are grayed-out. You know where everything you use is located geometrically, which is an important part of UI effectiveness.

That menubar, at least in Windows, is already OFF by default, and we have to enable it (which is the first thing that I do in setting up a test profile). Please do not continue to fight against those of us who want a stable menubar.

Comment 9

[Thomas D. (:thomas8)]

(In reply to Kent James (:rkent) from comment #8)
> Yes this should be fixed, but please not by removing the menubar. There is a
> good reason to have a stable UI element, even if most of the elements are
> grayed-out. You know where everything you use is located geometrically,
> which is an important part of UI effectiveness.

I did not find a single menu element which is
- not greyed out AND
- applicable to saved files.

> That menubar, at least in Windows, is already OFF by default, and we have to
> enable it (which is the first thing that I do in setting up a test profile).
> Please do not continue to fight against those of us who want a stable
> menubar.

I'm totally with Kent that menu bars are very useful and I wouldn't mind re-enabling the menu bar by default for TB (as the burger button does not seem very useful to me...).

However, I'm failing to see the usefulness of this particular menubar for "Saved Files" tab which has only around 4 applicable commands on the contextual menu. So I thought just removing the entire menu bar on the "Saved Files" tab might be an easy way to get rid of all the useless menus *for this tab* AND fix this bug.

Having a menu where 99% of commands are disabled because they don't apply to "Saved Files" is useless-UI, and looks very unprofessional. Let's not forget that this is a simple dialogue/list stuffed into a tab. That handful of commands which is applicable for "Saved Files" can be much better combined into a single menu-button or some such to make them available here.

Kent, if you really believe that "Saved Files" tab should have a menu bar, could you positively point out and give us some idea which applicable commands would be found there and how they should be grouped together in a way that is useful (where searching for a single enabled command between 20 disabled commands does not look useful to me).

Comment 10

[Thomas D. (:thomas8)]

OK, so this menu bar is only shown when you've enabled the menu bar in the main screen.
So by default, it's OFF both for the main screen and for "Saved Files".
Pls note there has never been a menu for "Saved Files" so far.
So if we want to introduce a menu now, it should offer some value.
Anything which is not applicable must at least be disabled at the highest possible level. There's really no point of e.g. offering an enabled "Message" main menu with all commands disabled when there aren't any messages on this tab, and probably never will be.
FF bookmarks manager (Ctrl+Shift+B), which seems like an equivalent UI, has a single "Organize..." menu where the few relevant commands have been assembled in a dropdown menu button.

Comment 11

[Kent James (:rkent)]

"Kent, if you really believe that "Saved Files" tab should have a menu bar, could you positively point out and give us some idea which applicable commands would be found there and how they should be grouped together in a way that is useful (where searching for a single enabled command between 20 disabled commands does not look useful to me)."

I do not believe that the grouping of the menubar should be tab-specific. I also believe that this menubar should always be present for whatever tab is being displayed (and I think that is current behavior). Previously Saved Files was a popup, not a tab.

This is a universal issue, why are you bringing this up in the context of the Saved Files tab? The menubar is there and mostly irrelevant on the Addon Managers tab, on the Lightning tabs, etc.

I really think we need to focus this bug on the issue of the disabled Write button, and if you want to discuss removing or modify the menubar from a particular tab, move that discussion to another bug. I do not believe that special casing the Saved Items tab is a reasonable approach.

Comment 12

[Thomas D. (:thomas8)]

(In reply to Kent James (:rkent) from comment #11)

> This is a universal issue, why are you bringing this up in the context of
> the Saved Files tab? The menubar is there and mostly irrelevant on the Addon
> Managers tab, on the Lightning tabs, etc.

Thanks for pointing out the universal nature of this problem, that indeed we have message reader's main menu bar occuring on all sorts of dedicated tabs, where it's usually mostly contextually irrelevant or even misleading. I wonder what happens when I use "Undo" from "Addons" tab - will it undo my last addon?

> I really think we need to focus this bug on the issue of the disabled Write
> button, and if you want to discuss removing or modify the menubar from a
> particular tab, move that discussion to another bug. I do not believe that
> special casing the Saved Items tab is a reasonable approach.

Fair enough, in view of the above. Sorry for making noise in the wrong place.
So this bug only wants to prevent the Write button from getting wrongly disabled, along comment 4:

(In reply to :aceman from comment #4)
> Yes I can reproduce too. The toolbar button (but also file->new->message)
> get explicit disabled=true attribute.
> 
> I can't find a place that sets it like that as isEnabled("cmd_newMessage")
> is intended to always return true in command controllers.
> 
> I wonder which controller is governing the menus in when the "saved files"
> tab is open as many items are disabled at that time.

Comment 13

[:aceman]

It is probably fine that we disable New message while in Downloads tab. Some more commands could be disabled, but that is for another bug.
We do not update the cmd_newMessage when changing to other tab so the buttons can get enabled again.

I think I am on the right track in my experiments.

Comment 14

[:aceman]

Attachment: patch

This works for me.
I haven't found any other command by visual inspection, that would need the same treatment.

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #13)
> It is probably fine that we disable New message while in Downloads tab. 

I don't see why it should be disabled, it makes perfect sense for it to always work. E.g. you're looking at some attachments you got and you want to write someone about an issue in a file etc.

Comment 16

[Magnus Melin [:mkmelin]]

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

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

Yes, this fixes the immediate issue.

Comment 17

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #15)
> (In reply to :aceman from comment #13)
> > It is probably fine that we disable New message while in Downloads tab. 
> 
> I don't see why it should be disabled, it makes perfect sense for it to
> always work. E.g. you're looking at some attachments you got and you want to
> write someone about an issue in a file etc.

+1. New message should never be disabled.

Comment 18

[:aceman]

Even though the user does not see in which account he is? Yes, he will see it in the compose window (if he is familiar with the From dropdown). But is it enough?

Anyway, do we agree the patch here to be a quick fix for TB38? Then we can argue about enabling New message in a followup bug (if anybody files it). Because the reason why New message/Write gets disabled (and all the other menuitems) is still unknown to me so it won't be as quick. Probably something to do with which controller governs commands being per-tab.

Comment 19

[Magnus Melin [:mkmelin]]

Yes, I think that's enough.

And sure, let's do this patch now for 38 and possibly uplift a patch later that makes it always enabled.

Comment 20

[:aceman]

Thanks.
Even if we make the item always enabled in future, the patch here will not need to be reverted. It does not decide whether the New message should be enabled by itself, it just adds some missing command update events. So that New message item gets updated (and determined that it needs to be enabled by existing code elsewhere) when we leave the Downloaded files tab.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/comm-central/rev/9ba853d8e5aa

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

[Approval Request Comment]
Regression caused by (bug #): bug 1087233
User impact if declined: 'Write' button sometimes disabled
Testing completed (on c-c, etc.):  ?
Risk to taking this patch (and alternatives if risky): ?

Comment 23

[Kent James (:rkent)]

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

https://hg.mozilla.org/releases/comm-aurora/rev/620ea4a1d3fe
https://hg.mozilla.org/releases/comm-beta/rev/034d3c44fa94