Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of legacy components #9

Open
jobisoft opened this issue Feb 15, 2024 · 8 comments
Open

Get rid of legacy components #9

jobisoft opened this issue Feb 15, 2024 · 8 comments

Comments

@jobisoft
Copy link

Hi, saw your effort to bring this back to life. Thanks!

The add-on you started to work on uses ancient technology and will break again soon. There are a few steps you could do to get rid of them:

  1. Get rid of legacy locales (get rid of all "locale" entries here)
  2. Get rid of legacy preferences (drop this and use local storage)
  3. Convert your XHTML options page to HTML and use an standard options_ui manifest entry instead of this.
  4. Get rid of the WindowListener API and instead use dedicated / less complex load mechanism (something comparable to this).

We have many community channels which you can use if you seek further help:
https://developer.thunderbird.net/add-ons/community

Note: There will be no updated version of the WindowListener API for the next ESR. This was intended as a hot fix for the transition to TB78 and became unmaintainable by now. Also, using it does not allow developers to actually develop real APIs, which could be merged into Thunderbird.

Looking forward to seeing you around,
John

@Vigilans
Copy link
Owner

Vigilans commented Feb 16, 2024

Hi John,

I brought back this addon for modern Thunderbird mainly for my personal use (instead of being an active maintainer), so I wouldn't put much effort on refactoring it once it works on current version. But I can provide help if someone wants to work on this, so PRs are welcome and I will review in time!

However, I am trying to build another addon to automatically detect avatar for each message, and add avatar div to messages in thread pane card view. So, although I wouldn't start getting rid of legacy components on this project, I would opt in the modern way if there is a good solution to implement my addon in it.

What I have not found in modern addon development is the way to inject script and CSS to about:3pane and about:message like WindowListener API. After some investigation, I think the modern alternative should be contentScript.

However in Thunderbird:

  1. For native tabs, content script only supports compose and messageDisplay. When trying to use tabs.executeScript or tabs.insertCSS on about:3pane tab, I got "host" permission required error. compose and messageDisplay window works by adding compose and messsagesModify permission. This seems to be controlled by these patching code, which does not include logic for about:3pane tab.

  2. For messageDisplay content script, the code is actually injected into message body browser, instead of the whole about:message browser that contains message header. This makes messageDisplay content script cannot be used to customize message header element. This seems to be controlled by these code, which uses about:message -> tabOrWindow instead of about:message window for injecting.

So, I think there should be (experiment) APIs for content scripts / CSS of the whole about:3pane and about:message browser. WindowListener API actually implemented support for them before stopping maintenance, but Thunderbird seems did not provide modern alternative for it.

For reference: Compact Header's solution are not good enough compared to WindowListener API / Content Script in some points:

  1. onMessageDisplayed is an event somewhat late for injecting logic. If I am to patch some class methods, WindowListener will take effect before rendering logic happened, but in onMessageDisplayed browser is already fully ready before patching, makes it have to re-apply the patched logic to existing elements.
  2. No simple API to inject CSS like WindowListener API and Content Script.

Regards,
Vigilans

@jobisoft
Copy link
Author

We probably should move this discussion to a different space :-)

So foremost you are right, that we still have areas not accessible to WebExtension APIs, and it will take more time to get there. However, the Windowlistener API is not the only way to patch the core UI of Thunderbird, in fact it is a very discouraged way, because it keeps you stuck in an old pattern of how to code. Specifically locales, prefs and options. You do not need the Windowlistener API just to be able to react to windows/tabs being opened.

The Windowlistener API is just a very complex wrapper around the core ExtensionSupport.registerWindowListener concept. You can of course - if needed - still use that. But we highly suggest writing a dedicated Experiment API for your use case, instead of using my Windowlistener API. An example is in our example repository.

But before you use ExtensionSupport.registerWindowListener, it is always worth checking, if any of the already existing WebExtension events and functions, which give you a tabId or a windowId, allow you to apply your UI patches. If you have the tabId or the windowId, you can pass that as a parameter to your Experiment and then use code like this, to convert it into a native tab or window, which you can modify:

function getMessageWindow(tabId) {
      // Get about:message from the tabId.
      let { nativeTab } = context.extension.tabManager.get(tabId);
      if (nativeTab instanceof Ci.nsIDOMWindow) {
        return nativeTab.messageBrowser.contentWindow
      } else if (nativeTab.mode && nativeTab.mode.name == "mail3PaneTab") {
        return nativeTab.chromeBrowser.contentWindow.messageBrowser.contentWindow
      } else if (nativeTab.mode && nativeTab.mode.name == "mailMessageTab") {
        return nativeTab.chromeBrowser.contentWindow;
      }
      return null;
    }

or

function getAbout3Pane(tabId) {
   // We assume the tabId is a valid mailTab, no error checking.
   let tab = context.extension.tabManager.get(tabId);
   let about3Pane = tab.nativeTab.chromeBrowser.contentWindow;
   return about3Pane;
}

@Vigilans Vigilans pinned this issue Feb 16, 2024
@Vigilans
Copy link
Owner

Vigilans commented Feb 16, 2024

function getMessageWindow(tabId) {
      // Get about:message from the tabId.
      let { nativeTab } = context.extension.tabManager.get(tabId);
      if (nativeTab instanceof Ci.nsIDOMWindow) {
        return nativeTab.messageBrowser.contentWindow
      } else if (nativeTab.mode && nativeTab.mode.name == "mail3PaneTab") {
        return nativeTab.chromeBrowser.contentWindow.messageBrowser.contentWindow
      } else if (nativeTab.mode && nativeTab.mode.name == "mailMessageTab") {
        return nativeTab.chromeBrowser.contentWindow;
      }
      return null;
    }

Take a look at this PR: dillenger/compact-headers#2. When starting Thunderbird, it shows the welcome page, in which case nativeTab.chromeBrowser.contentWindow.messageBrowser may be null.

The Windowlistener API is just a very complex wrapper around the core ExtensionSupport.registerWindowListener concept. You can of course - if needed - still use that. But we highly suggest writing a dedicated Experiment API for your use case, instead of using my Windowlistener API.

I'd like to try drafting a new experiment API named ContentScript, providing APIs like messageDisplayScripts.register and composeScripts.register, and works by 1) patch script.matchesWindowGlobal like what ext-mail do; 2) pick the implementation from ext-extensionScripts. I don't know whether arbitary permission string can be available everywhere in the code by trivially adding it to manifest.json, if not so, ContentScript may only allow native tab content scripts registered through directly calling its API.

@jobisoft
Copy link
Author

Take a look at this PR: dillenger/compact-headers#2. When starting Thunderbird, it shows the welcome page, in which case nativeTab.chromeBrowser.contentWindow.messageBrowser may be null.

True. The add-on I copied this example from is reacting to a messageDisplay.onMessageDisplayed event, so it is skipping any sanity checks. I should have added a comment just like I did in the other example. Sorry for not being clear enough.

@jobisoft
Copy link
Author

jobisoft commented Feb 16, 2024

I'd like to try drafting a new experiment API named ContentScript,

The term "content script" is already in use and reserved for scripts loaded into content (web-pages) - and supported by Thunderbird.

I think you want to modify the thread pane or the message header area? Both are chrome and not content (AFAIK). Ideal would be a mockup of where your avatar would end up, so I have a better understanding of what you are trying to do.

@Vigilans
Copy link
Owner

Vigilans commented Feb 16, 2024

Ideal would be a mockup of where your avatar would end up, so I have a better understanding of what you are trying to do.

I have already implemented the avatar div in thread card, using WindowListener API:

image

Avatar div is on the left of each thread card, can be toggled by Show Message Avatar. For GitHub notifications, I will generate an avatar of the associated GitHub user profile avatar by extracting the username from X-GitHub-Sender message header.

I think you want to modify the thread pane or the message header area? Both are chrome and not content (AFAIK)

Indeed all related browser are chrome (messenger.xhtml, messageWindow.xhtml, messengercompose.xhtml, about:3pane, about:message), so ChromeScripts would be better than ContentScripts?

@jobisoft
Copy link
Author

Nice.

ChromeScripts would be a better matching name, yes. But I am not so sure about the API itself. It will never be able to land. I do not know if you just want to work on this API to have something until someone else implements this in core, or if you aim at contributing.

An API that can land would be something which is related more to a specific use case, here the cards view. Something I have thought about for the message header area is a template definition with add-on callbacks (WebExtension events, to provide and cache the data for the custom elements in the template). This would work for the cards view as well, I think.

What I have not solved yet is a robust way to let multiple add-ons change the template. Maybe you have ideas.

The other concept of course is to turn the message header area and the threadpane into real content iframes, allowing standard content scripts to modify them. But I fear that this will cause trouble down the road.

@Vigilans
Copy link
Owner

Vigilans commented Feb 16, 2024

I do not know if you just want to work on this API to have something until someone else implements this in core, or if you aim at contributing.
An API that can land would be something which is related more to a specific use case, here the cards view.

ChromeScripts would be a tailored WindowListener API only focusing on custom script injection. It is expected to be reused for my addons, like for Message Avatar mentioned above, and for getting rid of WindowListener in this Account Colors codebase. So it's aimed for a wider use case at the beginning, not really expecting it to land, or to contribute back.

Something I have thought about for the message header area is a template definition with add-on callbacks (WebExtension events, to provide and cache the data for the custom elements in the template). This would work for the cards view as well, I think.
What I have not solved yet is a robust way to let multiple add-ons change the template. Maybe you have ideas.

I prefer custom script injection way like WindowListener and Content Script does, because developer's demand really varies. Scope to a pre-defined specification may cause inconvenience both in terms of flexibility and compatibility.

For compatibility, take my avatar div element as example:

To not change messenger's original HTML structure, the avatar div is inserted this way:

<tr is="thread-card">
  <div class="thread-card-avatar-container"> <- New avatar container div! used more CSS code to make it looks good
    <div class="recipient-avatar"></div>
  </div>
  <div class="thread-card-container">  <- Original container div
    <div class="thread-card-row"></div>
    <div class="thread-card-row"></div>
  </div>
</tr>

But a simpler and more intuitive structure would be to use nested flex elements:

<tr is="thread-card">
  <div> <- New parent div, row flex direction
    <div class="recipient-avatar"></div>
    <div class="thread-card-container">  <- Original container div, column flex direction
      <div class="thread-card-row"></div>
      <div class="thread-card-row"></div>
    </div>
  </div>
</tr>

However, the newly introduced parent div just breaks the structure of tr[is="thread-card"] > .thread-card-container! This might break another addon that relies on this path. Therefore, managing multiple addons on a template would be a hard problem to solve, and let developers use custom script to resolve different issues freely may be a not bad choice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants