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

RESOLVED FIXED in Thunderbird 63.0

Status

defect
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

(Depends on 1 bug)

unspecified
Thunderbird 63.0

Thunderbird Tracking Flags

(thunderbird_esr6062+ fixed, thunderbird63 fixed)

Details

Attachments

(7 attachments, 4 obsolete attachments)

(Assignee)

Description

8 months ago
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.
(Assignee)

Comment 1

8 months ago
Posted patch background-image.patch (obsolete) — Splinter Review
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9003986 - Flags: review?(jorgk)
(Assignee)

Comment 2

8 months ago
Ah, I forgot to mention that bug 1486018 is already implemented in this patch.
(Assignee)

Comment 3

8 months ago
Posted patch background-image.patch (obsolete) — Splinter Review
This patch needs no new CSS files in AB and composer content directories. It's enough to have the rules in content/messenger.css.
Attachment #9003986 - Attachment is obsolete: true
Attachment #9003986 - Flags: review?(jorgk)
Attachment #9003999 - Flags: review?(jorgk)

Comment 4

8 months ago
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.
(Assignee)

Comment 5

8 months ago
(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.
Attachment #9003999 - Attachment is obsolete: true
Attachment #9003999 - Flags: review?(jorgk)
Attachment #9004459 - Flags: review?(jorgk)

Comment 6

8 months ago
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.
Attachment #9004459 - Flags: review?(jorgk) → review+
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 7

8 months ago
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

8 months ago
Target Milestone: --- → Thunderbird 63.0

Comment 8

8 months ago
Since all the M-C stuff got backported to m-esr60, this needs uplift too, right? Can you please check that the patch applies.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 9

8 months ago
I'm on it. But this needs some changes because 60 still uses overlays.
Flags: needinfo?(richard.marti)

Comment 10

8 months ago
(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

8 months ago
Also in tabs where there is no toolbar, like in add-ons, options, etc.
(Assignee)

Comment 12

8 months ago
(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.
(Assignee)

Comment 13

8 months ago
(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.
(Assignee)

Comment 14

8 months ago
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.
Attachment #9004781 - Flags: review?(jorgk)
(Assignee)

Updated

8 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

8 months ago
Patch for ESR60 with the follow-up patch included.

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

Comment 16

8 months ago
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.
(Assignee)

Comment 17

8 months ago
(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

8 months ago
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.
Attachment #9004781 - Flags: review?(jorgk) → review+

Comment 19

8 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/104cd92a6d70
Follow-up: Move the LW-background-box outside of the navigation-toolbox. r=jorgk
Status: REOPENED → RESOLVED
Last Resolved: 8 months ago8 months ago
Resolution: --- → FIXED
(Assignee)

Updated

8 months ago
Depends on: 1487021

Updated

8 months ago
Attachment #9004788 - Flags: approval-comm-esr60? → approval-comm-esr60+

Comment 20

8 months ago
(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

8 months ago
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

8 months ago
Richard, can you please try that build. The background isn't working as well as on trunk.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 23

8 months ago
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.
Flags: needinfo?(richard.marti)

Comment 24

8 months ago
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

8 months ago
Posted patch ESR-FF.diff (obsolete) — Splinter Review
(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

8 months ago
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 28

8 months ago
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.
Attachment #9005717 - Attachment is obsolete: true
Attachment #9005735 - Flags: review?(richard.marti)
(Assignee)

Comment 29

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

This works, thanks.
Attachment #9005735 - Flags: review?(richard.marti) → review+

Updated

8 months ago
Duplicate of this bug: 1488024
(Assignee)

Comment 33

8 months ago
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.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 34

8 months ago
Posted patch Bug1486202-fup.patch (obsolete) — Splinter Review
This one works for me under Daily.
Attachment #9005879 - Flags: review?(jorgk)

Comment 35

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

Comment 36

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

Comment 37

8 months ago
Doesn't help on Mac. Must be something different.

Comment 38

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

Comment 39

8 months ago
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.
(Assignee)

Comment 40

8 months ago
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.
Attachment #9005879 - Attachment is obsolete: true
Attachment #9005879 - Flags: review?(jorgk)
Attachment #9005889 - Flags: review?(jorgk)
(Assignee)

Comment 41

8 months ago
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
Attachment #9005890 - Flags: approval-comm-esr60?

Comment 42

8 months ago
(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?
(Assignee)

Comment 43

8 months ago
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.
(Assignee)

Comment 44

8 months ago
(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

8 months ago
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?
(Assignee)

Comment 46

8 months ago
(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

8 months ago
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.
Attachment #9005890 - Flags: approval-comm-esr60? → approval-comm-esr60+

Comment 48

8 months ago
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.
(Assignee)

Comment 49

8 months ago
(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

8 months ago
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?
Attachment #9005889 - Flags: review?(jorgk) → review+
(Assignee)

Comment 51

8 months ago
Yes and unfortunately no.
Keywords: checkin-needed

Comment 52

8 months ago
Pushed by mozilla@jorgk.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.