1647768 - MX API browser.windows.create() titlePreface not displayed

Status: RESOLVED FIXED

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

Description

[Dave Royal]

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

Steps to reproduce:

I added 'titlePreface' to the warning popup in my addon:
https://github.com/revad/limit_non-BCC_recipients
(background.js line 161)

(Set pref to 1 and send to 2 people.)

Actual results:

The warning popup title was "- Thunderbird"

Expected results:

It should have been prefaced with the localised extension name.

This may be related to, or a duplicate of bug 1632307

Using TB78.0b2

Comment 2

[Geoff Lankow (:darktrojan)]

It is a dupe, but I like this one better.

Comment 3

[John Bieling (:TbSync)]

Confirmed that this works in FF78 but not in TB78.

Comment 4

[John Bieling (:TbSync)]

Attachment: bug1647768_fix_titlePreface

This adds support for titlePreface in windows.update API function for popup windows, which was forgotten in bug 1544793 (see diff). It also adds missing support for titlePreface in windows.create API function.

Comment 5

[John Bieling (:TbSync)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=afd7dbe2b66443baf4562a0755a9070a20fd5a4b

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9189403 [details] [diff] [review]
bug1647768_fix_titlePreface

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

::: mail/components/extensions/extensionPopup.js
@@ +75,5 @@
>    },
> +  updateTitlebar() {
> +    const e = new Event("DOMTitleChanged");
> +    this.selectedBrowser.dispatchEvent(e);
> +  },

I'd prefer to have the code from the event listener moved here rather than faking an event.

Comment 7

[John Bieling (:TbSync)]

Attachment: bug1647768_fix_titlePreface.patch

This also no longer uses

let browser = this.selectedBrowser

as there is already a browser getter which does the same.

Comment 8

[Geoff Lankow (:darktrojan)]

Comment on attachment 9189569 [details] [diff] [review]
bug1647768_fix_titlePreface.patch

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

This'll do. I'm tempted to make you write a test for this one too, but since there's almost no existing tests of windows.create, I won't.

::: mail/components/extensions/extensionPopup.js
@@ +24,1 @@
>    });

This function doesn't need the curly braces.

Comment 9

[John Bieling (:TbSync)]

Attachment: bug1647768_fix_titlePreface.patch

I have to learn how to write these tests anyway. How is this?

Comment 10

[John Bieling (:TbSync)]

Attachment: bug1647768_fix_titlePreface.patch

I think this is a better approach for the test.

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9189748 [details] [diff] [review]
bug1647768_fix_titlePreface.patch

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

::: mail/components/extensions/test/browser/browser_ext_windows.js
@@ +147,5 @@
> +        window.setTimeout(resolve, delay);
> +      });
> +    // The title might need some time to be updated after the event has been received.
> +    for (let trys = 0; trys < 10; trys++) {
> +      await sleep(20);

You should avoid waiting for things to happen like this when possible. In the first case the title should be ready when the window load event fires. In the second, listen for the title attribute changing with a MutationObserver.

With that done this function would be down to a few lines, which would be better put inline at the bottom. Extension tests are confusing enough by being in two parts, don't force the reader to jump back up here to figure out what's going on.

@@ +176,5 @@
> +            type: "popup",
> +          });
> +          await windowCreatePromise;
> +          browser.test.sendMessage("checkTitle", titlePreface);
> +          await window.waitForMessage();

You can combine these two lines by using window.sendMessage instead of browser.test.sendMessage.

@@ +208,5 @@
> +
> +  extension.onMessage("checkTitle", async titlePreface => {
> +    let win = await popupWindow;
> +    let titleOk = await checkTitle(win, titlePreface);
> +    Assert.ok(titleOk, `Title starts with ${titlePreface}`);

Since we the page we're loading is ours, set a title on it and test that all of the window's title is correct.

Comment 12

[John Bieling (:TbSync)]

Attachment: bug1647768_fix_titlePreface_v2.patch

Rebased to current tip.

I switched away from sleep to a mutation observer but the observer returns an empty MutationRecord in the test environment, so I have to guess if it is the title or not (saw that in a calendar test as well). I do not know if that is better. As you told me to not get too fancy in tests, I let it run into the overall test timeout instead of using a pomise race to have it exit and explicitly fail the title test after a manual timeout.

Since we the page we're loading is ours, set a title on it and test that all of the window's title is correct.

I do not understand that. I copied the usage of BrowserTestUtils.domWindowOpened()
from other parts of the same test. Many other tests await the window they open using this method. Why am I not allowed to use that? I would have to implement a full blown windowlistener to catch all opened windows manually.

I think I have not understood at all what you mean here. Could you rephrase your comment? I am setting the title on the window I opened and check if the title has the correct preface.

Comment 13

[Geoff Lankow (:darktrojan)]

By page I meant the content.html you're loading, it does not have a title. If it had a title, you could test that the window's whole title was "PREFACE1 - this is the page title" or whatever.

Comment 14

[John Bieling (:TbSync)]

Attachment: bug1647768_fix_titlePreface_v3.patch

Understood. I decided that I do not like the mutation observer approach at all, as it returns an empty MutationRecord in the test environment and one has to guess if it is the title or not. I therefore added a dedicated event which the test can listen for. Everything else was just a mess.

Comment 15

[Geoff Lankow (:darktrojan)]

Comment on attachment 9190492 [details] [diff] [review]
bug1647768_fix_titlePreface_v3.patch

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

::: mail/components/extensions/test/browser/browser_ext_windows.js
@@ +260,5 @@
> +  extension.onMessage("checkTitle", async titlePreface => {
> +    let win = await popupWindow;
> +    let titleChange = new Promise(resolve => {
> +      win.document.addEventListener("extension-window-title-changed", resolve);
> +    });

let titleChange = BrowserTestUtils.waitForEvent(win.document, "extension-window-title-changed");

::: mail/components/extensions/test/browser/data/content.html
@@ +1,5 @@
>  <!DOCTYPE html>
>  <html>
>  <head>
>    <meta charset="utf-8"/>
> +  <title>Thunderbird Test</title>

Don't use the word "Thunderbird" here. There's a (quite small) chance that it could come from the program itself. Maybe "A test document"?

Comment 16

[John Bieling (:TbSync)]

Attachment: bug1647768_fix_titlePreface_v4.patch

Changed the items mentioned in the last (approved) review. Setting this to r+

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/85c59491398b
Add support for titlePreface in windows.create API function and fix broken support for titlePreface in windows.update API function for popup windows. r=darktrojan

Comment 18

[klaus]

supposed to work in 78.6.0? Not on my Win 10 home.

... what does this mean: tracking-thunderbird_esr78: ? → + backported, or not?

Klaus

Comment 19

[John Bieling (:TbSync)]

Its only in beta yet. I will request uplift after it has backed on beta a bit.

Comment 20

[Mark Banner (:standard8)]

(In reply to klaus from comment #18)

... what does this mean: tracking-thunderbird_esr78: ? → + backported, or not?

Quick summary: it is being tracked by Thunderbird release drivers for consideration on the esr78 branch. If you click the "Tracking flags" title, there's more info.

Comment 21

[John Bieling (:TbSync)]

Comment on attachment 9190978 [details] [diff] [review]
bug1647768_fix_titlePreface_v4.patch

[Approval Request Comment]
User impact if declined:
Fixes a bug in our WebExtension API. Add-on developers would have to wait till next ESR to use this feature properly. As we want to move (new) developers away from the Experiment approach towards the proper WebExtension approach, improvements should be uplifted.

Testing completed (on c-c, etc.):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a0811bf90663c708f20a1c65ceee55639c74ca24

Risk to taking this patch (and alternatives if risky):
None that I know of. Patch applies on current comm-esr78 (for 78.7.0)

Comment 22

[John Bieling (:TbSync)]

Remark: I have created a try run which includes all bugs I want to uplift for TB 78.7 and shows a working patch order. This bug is the 6th one:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=3cdff293d5d1bd77777f5da371caaa02c4b05de3

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9190978 [details] [diff] [review]
bug1647768_fix_titlePreface_v4.patch

[Triage Comment]
Approved for esr78

Comment 24

[Dave Miller [:justdave]]

Thunderbird 78.7.0:
https://hg.mozilla.org/releases/comm-esr78/rev/08953ac6b24a