1486202 - Port bug 1443561 to TB: WebExtension themes additional backgrounds alignment should be relative to the toolbox

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

Attachment: Sample WX-theme with multiple backgrounds

With LW- and WX-themes using multiple backgrounds and positioning them does not work well because the image uses the whole window height and not the toolbar height.

Try the sample WX-theme. There are three TB icons: one on top right, one on center and one on bottom left. The centered icon isn't visible because it is in the center of the window behind the content. With visible statusbar the icon on bottom left can be spotted in the statusbar on the left.

These icons should be all positioned inside the tab- and toolbar. The same is for the AB- and composer window.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: background-image.patch

This ports bug 1443561, see https://hg.mozilla.org/releases/mozilla-esr60/rev/2d5cc6eecb7c and https://hg.mozilla.org/releases/mozilla-esr60/rev/4c33709c2271
mozilla-esr60 changesets because a later m-c check-in is merged in this changeset.

TB is different because the menu- and tabbar are in adifferent toolbox than the main toolbar. This makesit complicaet because we need a box which contains the background images and spans the height of both toolboxes. because of this I added a calculation for the height in msgMail3PaneWindow.js. This works for me but could make, that initially the box isn't correctly calculated. If you see such things, I hope we (or somebody with more JS knowledge) can fix this in a follow-up bug.

Because of bug 1443561 comment 56 and following, I added content CSS files for the AB and composer.

FX doesn't have a statusbar but we still have it. Depending of LW-/WX-theme it could be that the statusbar is empty but the text isn't readable because of the colours defined in main toolbar. Because of this, I decided to copy the background images of the top to the statusbar. With the BG test extension of this bug the three TB icons are now also visible in the statusbar. Normal LW-themes show still their LW-footer background image.

Comment 2

[Richard Marti (:Paenglab)]

Ah, I forgot to mention that bug 1486018 is already implemented in this patch.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: background-image.patch

This patch needs no new CSS files in AB and composer content directories. It's enough to have the rules in content/messenger.css.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003999 [details] [diff] [review]
background-image.patch

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

I installed the sample theme in TB 60 ESR and in a local build with the patch. I don't see a difference. Where can I see the "... does not work well ..." which is fixed by the patch?

::: mail/base/content/messenger.css
@@ +225,5 @@
> +  z-index: -1;
> +  height: 60px;
> +  position: absolute;
> +  pointer-events: none;
> +  top: 0%;

Just 0?

::: mail/base/content/msgMail3PaneWindow.js
@@ +1840,5 @@
>  
>      let titlebar = $("titlebar");
>      let menubar = $("mail-toolbar-menubar2");
>  
> +    // Calculate the LW-backgroundBox height to place the images correctly

Nit: fullstop.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: background-image.patch

(In reply to Jorg K (GMT+2) from comment #4)
> Comment on attachment 9003999 [details] [diff] [review]
> background-image.patch
> 
> Review of attachment 9003999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I installed the sample theme in TB 60 ESR and in a local build with the
> patch. I don't see a difference. Where can I see the "... does not work well
> ..." which is fixed by the patch?

You see three TB icons in the main toolbar on TB 60 and not only one on top right and one on the left of the statusbar? With the patch you should see three icons in the main toolbar and three in the statusbar.

> ::: mail/base/content/messenger.css
> @@ +225,5 @@
> > +  z-index: -1;
> > +  height: 60px;
> > +  position: absolute;
> > +  pointer-events: none;
> > +  top: 0%;
> 
> Just 0?

Yes.

> ::: mail/base/content/msgMail3PaneWindow.js
> @@ +1840,5 @@
> >  
> >      let titlebar = $("titlebar");
> >      let menubar = $("mail-toolbar-menubar2");
> >  
> > +    // Calculate the LW-backgroundBox height to place the images correctly
> 
> Nit: fullstop.

Fixed, and some other without full stop.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004459 [details] [diff] [review]
background-image.patch

Thanks, yes, TB 60 ESR doesn't show all the icons. Now it works after rebuilding.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ff6f03d6611e
Port bug 1443561 to TB: WebExtension themes additional backgrounds alignment should be relative to the toolbox. r=jorgk

Comment 8

[Jorg K (CEST = GMT+2)]

Since all the M-C stuff got backported to m-esr60, this needs uplift too, right? Can you please check that the patch applies.

Comment 9

[Richard Marti (:Paenglab)]

I'm on it. But this needs some changes because 60 still uses overlays.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #1)
> Because of this I added a calculation for the height in
> msgMail3PaneWindow.js. This works for me but could make, that initially the
> box isn't correctly calculated. If you see such things, I hope we (or
> somebody with more JS knowledge) can fix this in a follow-up bug.
Hmm, if you hide the mail toolbar, the three images aren't placed properly any more. Follow-up?

Comment 11

[Jorg K (CEST = GMT+2)]

Also in tabs where there is no toolbar, like in add-ons, options, etc.

Comment 12

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #10)
> (In reply to Richard Marti (:Paenglab) from comment #1)
> > Because of this I added a calculation for the height in
> > msgMail3PaneWindow.js. This works for me but could make, that initially the
> > box isn't correctly calculated. If you see such things, I hope we (or
> > somebody with more JS knowledge) can fix this in a follow-up bug.
> Hmm, if you hide the mail toolbar, the three images aren't placed properly
> any more. Follow-up?

Yes, also by hiding the tabs toolbar. It seems for hiding/showing toolbars the function isn't called. This should be done in a new bug.

Comment 13

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #11)
> Also in tabs where there is no toolbar, like in add-ons, options, etc.

I don't agree here because the main toolbar is still open in the main tab. If we would calculate the height on every tab the background could jump by switching the tabs. The only issue could be when the main tab's toolbar is hidden and the chat or calendar tab toolbars are shown. Then they aren't filled correctly. This should also be addressed in the follow-up bug, probably by calculating with the tallest toolbar of all open tabs.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Bug1486202-fup.patch

This follow-up addresses the hidden tabs toolbar, especially on Mac by moving the LW-background-box outside of the navigation-toolbox. This makes it also easier because we don't need the z-index which made problems on the stand-alone message window.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: background-image-ESR.patch

Patch for ESR60 with the follow-up patch included.

Test build: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0d52e3b1304c6ebe3b0a61d9a6bd4b72ce5fecfb

Comment 16

[Jorg K (CEST = GMT+2)]

Hmm, what's a "hidden tabs toolbar"?

In the stand-alone window, the left-most icon is cut off at the bottom.

I noticed that when showing/hiding the menu bar, the icons get repositioned, but not when showing/hiding the mail toolbar.

Comment 17

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #16)
> Hmm, what's a "hidden tabs toolbar"?

With mail.tabs.autoHide to true the tabbar hides when only one tab is open.

> In the stand-alone window, the left-most icon is cut off at the bottom.

Yes, the calculation doesn't work here because it does only in main window. Moving out the function of the tabsintitlebar function could fix this. But where to put it that it works? Again, something for the follow-up bug.

Found this issue when I worked on the ESR patch. I never use the stand-alone window and forgot about it.

> I noticed that when showing/hiding the menu bar, the icons get repositioned,
> but not when showing/hiding the mail toolbar.

Yes the function isn't triggered when hiding/showing the toolbars.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004781 [details] [diff] [review]
Bug1486202-fup.patch

Not ideal do to all the variations that don't quite work and the missing update when showing/hiding the mail toolbar, but let's take it.

Please file a follow-up bug.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/104cd92a6d70
Follow-up: Move the LW-background-box outside of the navigation-toolbox. r=jorgk

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #15)
> Patch for ESR60 with the follow-up patch included.
> Test build:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=0d52e3b1304c6ebe3b0a61d9a6bd4b72ce5fecfb
That build doesn't include the relevant m-esr60 changesets:
https://hg.mozilla.org/releases/mozilla-esr60/rev/2d5cc6eecb7c
https://hg.mozilla.org/releases/mozilla-esr60/rev/4c33709c2271
https://hg.mozilla.org/releases/mozilla-esr60/rev/a8e3e6431942

These are mostly browser/ and toolkit test changes, but this change
https://hg.mozilla.org/releases/mozilla-esr60/rev/4c33709c2271#l2.2
is in a toolkit file. Maybe it's not necessary for TB. Anyway, we should test a "real" TB 60 ESR build with all changes included when the time comes.

Comment 21

[Jorg K (CEST = GMT+2)]

TB 60.1 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/10871e0f08bbab7721fb2e64af851494d3ba7496

This build
https://treeherder.mozilla.org/#/jobs?repo=comm-esr60&revision=819bb7b39c4d27fb88138fbf5d0a263b91b3886b
also includes the m-esr60 changesets since I have included them now on the THUNDERBIRD_60_VERBRANCH.

So maybe a good idea to download it and check that it works as expected.

Comment 22

[Jorg K (CEST = GMT+2)]

Richard, can you please try that build. The background isn't working as well as on trunk.

Comment 23

[Richard Marti (:Paenglab)]

I was already trying. A issue is, that the height calculation isn't done when mail.tabs.drawInTitlebar is on false. So a later change in the drawintitlebar calculation made it also working with disabled drawintitlebar.

Comment 24

[Jorg K (CEST = GMT+2)]

Right. My mistake, I was switching the menu bar on and off, and I thought that repositions the icons, but it's not.

So can we locate the (FF/Core?) change that triggers the recalculation when mail.tabs.drawInTitlebar is false.

Comparing mail/base/content/msgMail3PaneWindow.js on trunk and comm-esr60 I can see that they are very different, mostly to my repair in bug 1446151. That ported bug 1445737, bug 1448613, bug 1448303 and bug 1449689. But that's all mozilla61 stuff which doesn't affect TB 60.

What we should do is compare TabsInTitlebar in comm-esr60 and mozilla-esr60.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: ESR-FF.diff

(In reply to Jorg K (GMT+2) from comment #24)
> What we should do is compare TabsInTitlebar in comm-esr60 and mozilla-esr60.
Here it is. Not too different.

Comment 26

[Jorg K (CEST = GMT+2)]

OK, here's my verdict.

On trunk you have the "Calculate the LW-backgroundBox" in _layOutTitlebar() which is called in update().

_layOutTitlebar() doesn't exist in ESR 60, but the equivalent of update() is updateTitlebarDisplay(). Try moving the calculation to the end of that function.

If that doesn't help, then I need to build and debug ESR 60 :-(

Comment 27

[Jorg K (CEST = GMT+2)]

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

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1486202-esr-follow-up.patch

OK, this moves the code and wrote. Tried locally by unpacking omni.ja. Try build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5055888e752957baba8b6bcd31f21f933ce4b038

Note: the previous one wasn't right since it was lacking the 'rect' variable.

Comment 29

[Richard Marti (:Paenglab)]

Comment on attachment 9005735 [details] [diff] [review]
1486202-esr-follow-up.patch

This works, thanks.

Comment 30

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/8efe7cd0a9b16472344f26a389d4f66b3eb107d1

Comment 32

[Jorg K (CEST = GMT+2)]

I'll have to backout
https://hg.mozilla.org/releases/comm-esr60/rev/10871e0f08bbab7721fb2e64af851494d3ba7496
https://hg.mozilla.org/releases/comm-esr60/rev/8efe7cd0a9b16472344f26a389d4f66b3eb107d1
since they obliterated Calendar, see bug 1488024.

Comment 33

[Richard Marti (:Paenglab)]

Lightning has problems when #tabmail-container has position:relative set. Maybe because of the loading of Lightning as WE it works under Daily. I have a fix for Daily but I am still on ESR because it's a bit different with the layering because of the overlays, it seems.

Comment 34

[Richard Marti (:Paenglab)]

Attachment: Bug1486202-fup.patch

This one works for me under Daily.

Comment 35

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #34)
> This one works for me under Daily.
But Daily isn't broken, or is it?

Comment 36

[Richard Marti (:Paenglab)]

Lightning on Daily under Mac looks weird now for me. I need to test with the patch if it's because of this.

Comment 37

[Richard Marti (:Paenglab)]

Doesn't help on Mac. Must be something different.

Comment 38

[Jorg K (CEST = GMT+2)]

Hmm, I'm confused now. What is the patch for? I see no problem with Daily on Windows, so what does the patch fix?

Comment 39

[Richard Marti (:Paenglab)]

First I hoped we can use this patch for ESR too and all is good. But it's not the case. I removed the position: relative; to be sure we don't fall in a similar issue like on ESR.

Comment 40

[Richard Marti (:Paenglab)]

Attachment: Bug1486202-fup.patch

This patch for Daily is a cleanup with moving the position:relative rule to messageWindow.css because this rules are only neeeded in the stand-alone message window. Like this we don't mess with position:relative in the main window and could lead to issues like on ESR.

Comment 41

[Richard Marti (:Paenglab)]

Attachment: Bug1486202-ESR-fup2.patch

Like the Daily patch but a little bit different with not using z-index but one position:relative. This works for me also with Lightning.

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

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #41)
> Like the Daily patch but a little bit different with not using z-index but
> one position:relative. This works for me also with Lightning.
So why that difference? The z-index was first added here:
https://hg.mozilla.org/comm-central/rev/ff6f03d6611e#l2.43
and then removed here:
https://hg.mozilla.org/comm-central/rev/104cd92a6d70#l2.12
and now it's back, but only in Daily?

Comment 43

[Richard Marti (:Paenglab)]

The first one was when I developed it on Daily only. The second one was after I adapted it to ESR because here it doesn't work and I tried to be as synchronized as possible. But ESR and Daily are too different in this case that I reverted it now again on Daily.

Comment 44

[Richard Marti (:Paenglab)]

(In reply to Richard Marti (:Paenglab) from comment #41)
> Created attachment 9005890 [details] [diff] [review]
> Bug1486202-ESR-fup2.patch
> 
> Like the Daily patch but a little bit different with not using z-index but
> one position:relative. This works for me also with Lightning.
> 
> Try:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=0ef7a1c5494a9e7e4450858969e09b5e3b73e540

Yep, try works for me.

Comment 45

[Jorg K (CEST = GMT+2)]

Comment on attachment 9005890 [details] [diff] [review]
Bug1486202-ESR-fup2.patch

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

Works for me, too.

::: mail/base/content/messenger.css
@@ +232,5 @@
>    width: -moz-available;
>  }
>  
> +/* Set position: relative to let the LW-background-box be behind. */
> +#tabmail-container {

You don't need/want the :-moz-lwtheme here?

Comment 46

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #45)
> Comment on attachment 9005890 [details] [diff] [review]
> Bug1486202-ESR-fup2.patch
> 
> Review of attachment 9005890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Works for me, too.
> 
> ::: mail/base/content/messenger.css
> @@ +232,5 @@
> >    width: -moz-available;
> >  }
> >  
> > +/* Set position: relative to let the LW-background-box be behind. */
> > +#tabmail-container {
> 
> You don't need/want the :-moz-lwtheme here?

No, with :-moz-lwtheme the problem appears again.

Comment 47

[Jorg K (CEST = GMT+2)]

Comment on attachment 9005890 [details] [diff] [review]
Bug1486202-ESR-fup2.patch

https://hg.mozilla.org/releases/comm-esr60/rev/8dea828db1014092337351181aaf7bb61bffb78c

Somehow I never believed you when you said: "low/no risk, CSS only". As we see, CSS can trigger severe bugs.

Comment 48

[Jorg K (CEST = GMT+2)]

Comment on attachment 9005889 [details] [diff] [review]
Bug1486202-fup.patch

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

::: mail/base/content/messenger.css
@@ +222,5 @@
>  }
>  
> +#messengerWindow[windowtype="mail:3pane"] > #LW-background-box:-moz-lwtheme {
> +  z-index: -1;
> +}

I tried the ESR code here
#tabmail-container {
  position: relative;
}
and it appears to be working, too.

Comment 49

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #48)
> I tried the ESR code here
> #tabmail-container {
>   position: relative;
> }
> and it appears to be working, too.

But it's not needed.

Comment 50

[Jorg K (CEST = GMT+2)]

Comment on attachment 9005889 [details] [diff] [review]
Bug1486202-fup.patch

So you really prefer the z-index? And that doesn't work in the ESR?

Comment 51

[Richard Marti (:Paenglab)]

Yes and unfortunately no.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8cbf3017d1d3
Follow-up: Move the 'position: relative' rule from messenger.css to messengerWindow.css and use z-index for #LW-background-box. r=jorgk

Comment 53

[Jorg K (CEST = GMT+2)]

TB 60 beta 11:
https://hg.mozilla.org/releases/comm-beta/rev/f1db5fa3f2fae4d9d09448512c06544be16e0fa1
https://hg.mozilla.org/releases/comm-beta/rev/b8fb9bf3792fde53eb189d6b27c2a1835c114dd8
https://hg.mozilla.org/releases/comm-beta/rev/370e9fd8b362865366fba3b9e13227c38f9e8a36