1775246 - browserAction buttons missed in messageDisplay windows

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[max]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:101.0) Gecko/20100101 Firefox/101.0

Steps to reproduce:

There is an annoying inconsistency related to browser_action toolbar buttons. They are missed in messageDisplay windows but they present in messageDisplay tabs of normal windows. The problem is still actual for Thunderbird-103.

• Install or load temporary an extension having browser_action.
• Open some message in new tab and in new window.
• Compare the tab and the window created on the previous step.

Actual results:

• Hamburger menu is present in both cases but add-ons browses action buttons are added in normal window, but not in messageDisplay one.

Expected results:

browserAction buttons are available in messageDisplay windows to have consistent UI.

Adding both browser_action and message_display_action makes interface of extension cluttered since action is almost the same. Using only message_display_action is not an ideal option when the add-on has a context menu item for message list. Disabling messageDisplayAction in normal windows is a kind of workaround, but things becomes more tricky as soon as context menu for message content is involved. It is impossible to just set _execute_browser_action as its command.

Comment 1

[John Bieling (:TbSync)]

Using only message_display_action is not an ideal option when the add-on has a context menu item for message list.

Can you explain that in more detail? The idea of the message_display_button is to appear whenever a message is displayed. The browser_action_button is shown whenever a mail:3pane window is displayed.

Comment 2

[max]

(In reply to John Bieling (:TbSync) from comment #1)

Using only message_display_action is not an ideal option when the add-on has a context menu item for message list.

Can you explain that in more detail? The idea of the message_display_button is to appear whenever a message is displayed. The browser_action_button is shown whenever a mail:3pane window is displayed.

From my point of view appearance of message display tabs of 3pane windows is more close to message display windows than to 3pane views of the same window. So it is rather strange when single message without folder or message lists is displayed in one case with browser actions and without such buttons in another case.

Moreover it is inconvenient to deal with such difference in the code. I started with a menu entry having

{
  contexts: [ "message_list", "page", "frame", "selection", "link", "image", "video", "audio", ],
  command: "_execute_browser_action",
  // ...
}

The intention is to query user notes for presence of links to selected objects (using nativeMessaging) and use popup to provide some info and a menu for actions (I consider it as a more convenient variant than second-level entries especially in the case of extremely long context menu in Thunderbird). In 3pane windows there is no point to add extra message_display_action that will open with exactly same popup (in respect to code and even with common menu entries).

If message display action is hidden in 3pane window (since browser action does the same) by code that checks type of window on startup or on window creation then it is not possible to use command for message body menu entry since it must be _execute_browser_action in 3pane windows and _execute_message_display_action in message display windows. Fortunately at least browserAction.openPopup in Mozilla does not require manifest v3.

So I have managed to avoid cluttering of UI by redundant buttons having the same action, but inconsistency of message display tab vs. message display window remains.

P.S. Actually I do not mind to open popup even in response to context menu for homepage links in add-on manager.

Comment 3

[John Bieling (:TbSync)]

From my point of view appearance of message display tabs of 3pane windows is more close to message display windows than to 3pane views of the same window. So it is rather strange when single message without folder or message lists is displayed in one case with browser actions and without such buttons in another case.

I can understand that, but it is not how it was intended. Making browser_display_action buttons appear in message display windows would change a lot of add-ons, having their browser action buttons appear in message display windows, where they may or may not work. I cannot change that.

I started with a menu entry having

Why can you not always use _execute_message_display_action and always use the message display button? Why do you need the browser_display_action button at all, if the information you want to display is message related?

If you consider the "message_list" context to be wrong for the message_display_action popup, you could open a real popup window (not an action popup) for that case (or all cases).

Edit: Referring to message display windows instead of message display tabs

Comment 4

[max]

(In reply to John Bieling (:TbSync) from comment #3)

I can understand that, but it is not how it was intended. Making browser_display_action buttons appear in message tabs would change a lot of add-ons, having their browser action buttons appear in message display tabs, where they may or may not work. I cannot change that.

browser_action buttons are currently displayed in message display tabs of 3pane windows. If extensions handle them in some way, I do not see any problem with browser_action in message display windows.

Why can you not always use _execute_message_display_action and always use the message display button? Why do you need the browser_display_action button at all, if the information you want to display is message related?

If you consider the "message_list" context to be wrong for the message_display_action popup, you could open a real popup window (not an action popup) for that case (or all cases).

1. Message pane may be hidden in 3pane tab (F8).
2. I decided that it might be confused for users that action in message list opens popup below (over message pane).
3. I expect to get more vertical space when popup is opened for browser_action.

As to real windows, I consider them too invasive. On the other hands browser action popups disappear on attempt to change keyboard layout under Gnome+X11. In addition I have a hope after experiments with thunderbird add a similar popup to my Firefox add-on and I am unsure concerning real popup windows on Android (however Android is a rather distant goal this case). So UI of extensions is rather limited and is always a trade-off.

Comment 5

[John Bieling (:TbSync)]

I corrected my faulty usage of "message display tabs" in Comment #3.

Comment 6

[max]

(In reply to John Bieling (:TbSync) from comment #5)

I corrected my faulty usage of "message display tabs" in Comment #3.

I still do not see difference between message display windows ("open message in new window") and message display tabs of 3pane windows ("open message in new tab"). They display just message and look rather same. It is the origin of my expectation that behavior in respect to browser_action should be the same as well.

Comment 7

[John Bieling (:TbSync)]

My remark was not against you, but I used the wrong term in my response and needed to correct that, but since you quoted it, I could not just change it.

As I said, I can understand your request. But currently I am not sure I can change it. In 114 we will get a (right) sidebar with vertical space for add-ons to use, maybe that would be a better place for your UI. I am still thinking about this.

Comment 8

[max]

(In reply to John Bieling (:TbSync) from comment #7)

My remark was not against you

Actually I am curious in which way extensions handling messageDisplay tab (so ready to get browser action event) may be broken by browser action button in a message display window. I just could not figure out a realistic scenario. My case was the opposite one, I had to fix an addon when I discovered the lack of this button. After you comments I suspect that I missed more issues with my addon.

Comment 9

[max]

(In reply to John Bieling (:TbSync) from comment #3)

Making browser_display_action buttons appear in message display windows would change a lot of add-ons, having their browser action buttons appear in message display windows, where they may or may not work. I cannot change that.

I have another idea: add a boolean field to browser_action manifest object that allows to enable browserAction button in messageCompose and messageDisplay windows.

Currently I have the following code to open popup.

	browser.browserAction.openPopup();
	browser.messageDisplayAction.openPopup();
	browser.composeAction.openPopup();

No async action allowed before openPopup otherwise Firefox/Thunderbird fails to consider the call as having user action context. When action is invoked through a command (shortcut) even current tab object is not passed to the listener, not to mention that decision concerning particular popup type requires window object.

I consider attempt to call all possible openPopup methods as a fragile solution. Some change in the code may either cause appearance of 2 popups in 3 pane windows or all of them may be blocked due to protection against spamming users using popup windows.

Comment 10

[John Bieling (:TbSync)]

Please do not add additional bug reports into existing bugs. To keep this bug focused on the browser_action being missing in message display windows, I do not want to discuss the openPopup() issue here. Please create a dedicated bug.

I have another idea: add a boolean field to browser_action manifest object that allows to enable browserAction button in messageCompose and messageDisplay windows.

Adding the browser_action to a stand-alone message window is something we can think about, but having it in the compose window is really wrong.

Comment 11

[max]

(In reply to John Bieling (:TbSync) from comment #10)

Please do not add additional bug reports into existing bugs. To keep this bug focused on the browser_action being missing in message display windows, I do not want to discuss the openPopup() issue here. Please create a dedicated bug.

I am sorry that I was not clear enough, but *_action toolbar buttons play 2 roles:

1. They allow to initiate some user action.
2. They allow to present to the user result of the action in the popup "attached" to the button.

Besides the toolbar button, an action may be activated through a context menu item or using a command (keyboard shortcut). When the button is missed, it is impossible to open its popup programmatically. That is why I consider complications related to opening proper popup tightly close to presence of the browser action button (title of this bug and one of the reasons why I filed it).

I have another idea: add a boolean field to browser_action manifest object that allows to enable browserAction button in messageCompose and messageDisplay windows.

Adding the browser_action to a stand-alone message window is something we can think about, but having it in the compose window is really wrong.

Please, consider some general action that does not depend whether it is a message from some folder or a currently composed one, in my case the action is to check if a user contain a link in their notes. Context menu items appears in all contexts: a 3 pane tab, a message display tab, a message display window. To display result, it is necessary to determine type of window. If browserAction button were always available (including messageCompose windows), the code would be straightforward, currently I am trying to figure out how many addition special cases I need to add to the code to not fail trying to open a popup.

Comment 12

[John Bieling (:TbSync)]

The use case you are describing is on the edge of what those buttons are for. Have you considered real popup windows or the notifications API to show information to the user independent of what window he is currently looking at?

To get around the user event handler restrictions (not being able to use await to find out, which action popup to open), you can use two different menu entries for compose and browser actions and do the required checks in the menus.onShown event to hide the "wrong" menu entry. This way the function triggered by the click on the menu entry does not need to do any asynchronous checks any more.

Comment 13

[max]

Tricks with hiding menu items may work for context menu, but I am against different keyboard shortcuts (commands) for each type of windows.

Notifications are not suitable, they are not interactive enough. The aim of popup is to allow the user to justify particular action based on presented results.

As I said in comment #4, real windows besides advantages have disadvantages. Currently I am happy with transient popups, but after your suggestion in comment #3 I started to think about a button in the popup that allows to convert it into a normal window or a tab when user feels that their should inspect output with more attention. Currently I prefer popups that does not proliferate on the screen just because the user have not explicitly closed them. OK, perhaps onblur handler may make "real" popup window transient to the desired degree but not more.

For browser extensions I have seen complains that "real" windows become full screen when the main window is in the fullscreen mode. It seems, thunderberd has nether the feature no the problem.

Comment 14

[John Bieling (:TbSync)]

I just realized, that the trick with the hidden menu item is not needed, the tab is part of the onClicked event. So what you need is bug 1787203, to have the same information available for the onCommand event.

Comment 15

[John Bieling (:TbSync)]

Attachment: Bug 1775246 - Add browser_action to messageWindow.xhtml. r=mkmelin

Depends on D155583

Comment 16

[John Bieling (:TbSync)]

Re-reading your Comment 6, this is actually a valid point:

I still do not see difference between message display windows ("open message in new window") and message display tabs of 3pane windows ("open message in new tab"). They display just message and look rather same. It is the origin of my expectation that behavior in respect to browser_action should be the same as well.

The attached patch adds support for a new manifest entry default_windows, which defaults to [normal] if not specified. But you can include messageDisplay in that array as well to have the browser_action also be displayed there. The window type names are taken from the windows API

Let's see if the try-run breaks any other test:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f2810f9c73fb350e9fbbd9f2c9ad5a5e0c1d4e0f

That try-run also includes Bug 1787203, so this should give you now everything you need, to be able to decide whether to use openPopup() on a browser_action or a compose_action.

Comment 17

[max]

(In reply to John Bieling (:TbSync) from comment #16)

The attached patch adds support for a new manifest entry default_windows, which defaults to [normal] if not specified. But you can include messageDisplay in that array as well to have the browser_action also be displayed there. The window type names are taken from the windows API

John, thank you for your attempt to fix the issue. Unfortunately as it currently implemented, it has limited usefulness for me.

• I do not see any way to detect in runtime if the feature is available (e.g. by checking if some method is not undefined). Fortunately at least unknown manifest key is just a warning, it does not prevent loading of the add-on.
• At first I expected that all window types would be available, so anyway I have to handle various special cases as messageCompose windows. While filing the issue I forgot about compose windows.

Likely I can not avoid windows.create in the case of special tabs that may have links as well. At first glance it is possible to mimic transient browserAction popups through window.onblur listener and some code to determine position of the screen for the custom popup window. Such approach even allows to create a workaround for the Bug #1683145.

So the change allows to make UI of messageDisplay tab and messageDisplay window more consistent, however for me it is better to wait till the new feature will be landed to ESR.

Comment 18

[John Bieling (:TbSync)]

Why would you need to feature detect this? It is your addon, you know whether you enabled the feature or not. Once this landed, you can release a new version of your add-on, which has a strict_min_version setting, which makes it compatible only with Thunderbird version which support it. Or use runtime version checking.

With onClicked and onCommand event both giving you the tab info now, you can decide for all your use cases during runtime if you are in a composer or not and can trigger the correct popup. You really have all you need now.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/43de3d1aa4e7
Add browser_action to messageWindow.xhtml. r=mkmelin

Comment 20

[max]

(In reply to John Bieling (:TbSync) from comment #18)

Why would you need to feature detect this? It is your addon, you know whether you enabled the feature or not. Once this landed, you can release a new version of your add-on, which has a strict_min_version setting, which makes it compatible only with Thunderbird version which support it. Or use runtime version checking.

Stable or long time support Linux distributions do not get updates immediately. I would prefer to not rule out their users. That is why I believe that it is better to have some workarounds for older version while using available features of latest Thunderbird versions. It is unfortunate when changes of behavior are hidden from addon. For example, disabling message compose action in v91 hides the button, but in 102 the button is present in disabled state.

Example of available versions:
https://packages.debian.org/thunderbird
https://packages.ubuntu.com/thunderbird

With onClicked and onCommand event both giving you the tab info now, you can decide for all your use cases during runtime if you are in a composer or not and can trigger the correct popup. You really have all you need now.

Your commit allows to get uniform behavior for messageDisplay window and messageDisplay tab that is great. Unfortunately messageCompose window is not the only special remaining case. Context menu may be invoked for a link in e.g. add-on manager tab having no browser action button. Is such decision documented somewhere? I can create a dedicated issue to discuss it. Even in the case of WONTFIX resolution it will clarify the reasons behind such decision.

Another case is context menu handler invoked for a link inside popup of another add-on. Tab passed to the handler tells nothing about actual context. undefined in Firefox has some sense in this case, however there is no plenty of action buttons (OK, page action may be hidden for particular tab). So browser action in message display windows does not solve all issues.

Comment 21

[John Bieling (:TbSync)]

Comment on attachment 9291717 [details]
Bug 1775246 - Add browser_action to messageWindow.xhtml. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: No messageDisplayAction in stand-alone message windows.
Testing completed (on c-c, etc.): One month on BETA.
Risk to taking this patch (and alternatives if risky): Should be low. But needs a dedicated patch, because the comm-central patch does not apply cleanly.

Comment 22

[John Bieling (:TbSync)]

Attachment: Bug1775246.ESR102.patch

Patch for ESR 102.

Comment 23

[John Bieling (:TbSync)]

Comment on attachment 9297823 [details] [diff] [review]
Bug1775246.ESR102.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: No messageDisplayAction in stand-alone message windows.
Testing completed (on c-c, etc.): One month on BETA.
Risk to taking this patch (and alternatives if risky): Should be low. But needs a dedicated patch, because the comm-central patch does not apply cleanly.

Comment 24

[Wayne Mery (:wsmwk)]

Comment on attachment 9297823 [details] [diff] [review]
Bug1775246.ESR102.patch

[Triage Comment]
Approved for esr102

Comment 25

[Rob Lemley [:rjl]]

Thunderbird 102.3.3:
https://hg.mozilla.org/releases/comm-esr102/rev/157d888645e7