Closed Bug 1450662 Opened Last year Closed Last year

With webextension theme the bottom toolbar border isn't drawn on AB and Composer

Categories

(Thunderbird :: Theme, enhancement)

enhancement
Not set

Tracking

(thunderbird60 fixed, thunderbird61 fixed)

RESOLVED FIXED
Thunderbird 61.0
Tracking Status
thunderbird60 --- fixed
thunderbird61 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(1 file, 2 obsolete files)

Like the summary says, there is no bottom border drawn.
Attached patch border-color.patch (obsolete) — Splinter Review
With this patch and my test theme you should see the bottom toolbar borders red like the one from main toolbar. No new test theme needed, but if you need it again, I can attach it.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8964264 - Flags: review?(jorgk)
Follow-up from which bug? Bug 1449707?
No much older, bug 1420254. And would be nice, when it can go to TB 60.
Comment on attachment 8964264 [details] [diff] [review]
border-color.patch

OK, that works, but the "toolbar_top_separator" is not applied to the toolbars but the tabs as bottom instead: ["--tabs-border-color", "toolbar_top_separator"]

So in effect the toolbar can have a configurable bottom colour but no top colour. Is that ideal? Like for me there is no line/separator/visual break between toolbar and menu above.
Attachment #8964264 - Flags: review?(jorgk)
Attachment #8964264 - Flags: review+
Attachment #8964264 - Flags: approval-comm-beta+
Attached patch border-color.patch (obsolete) — Splinter Review
This patch adds the top border. Actually it's not a border but a gradient to not grow the toolbox which would make the toolbar jump. But this could make the hovered/active menubar items overlay the border. On Linux and Win 10 this looks not bad and can be used. Mac has no menubar and this is no problem.

Jörg, I haven't obsoleted the first patch because I want that you check first on Win 7 if this overlay is acceptable. If yes, you can use this patch.
Attachment #8964385 - Flags: review?(jorgk)
Looks OK on Windows 10, I don't have Win 7 right now, then it will have to wait until later in the week.

So you don't style the border but instead put a 1px-high background image, right? But the new "border" isn't between the toolbar and the menu bar, but above the menu bar. I would have expected a line between the two.
I will review this in Win 7 tonight for Beta uplift tomorrow. Please answer the previous comment: The new "border" isn't between the toolbar and the menu bar, but above the menu bar. That's intentional?
Flags: needinfo?(richard.marti)
And if we go with the second patch, how about adding a comment to explain the hack. Something like:
  toolbar_top_separator is mapped to tabs-border-color, so here we create a top border
  as a background image to avoid jumping of the toolbar when the theme is enabled.
I hope I got this right.
Comment on attachment 8964385 [details] [diff] [review]
border-color.patch

On Win 7 it looks the same as in Win 10. Should there be a difference?

So we can use this patch, but please answer the question:
Why is the top border of the toolbar not below the menu. Also adding a comment would help.
Attachment #8964385 - Flags: review?(jorgk) → review+
Added this comment:

/* Uses a background image instead of a border to avoid jumping of the toolbar
   when a theme is enabled. */

(In reply to Jorg K (GMT+1) from comment #9)
> Comment on attachment 8964385 [details] [diff] [review]
> border-color.patch
> 
> On Win 7 it looks the same as in Win 10. Should there be a difference?

No, that's good.

> So we can use this patch, but please answer the question:
> Why is the top border of the toolbar not below the menu. Also adding a
> comment would help.

The border is to surround the whole toolbox. The main window is special with the tabs and the menubar is above the tabs, except win 7 Aero.
Attachment #8964264 - Attachment is obsolete: true
Attachment #8964385 - Attachment is obsolete: true
Flags: needinfo?(richard.marti)
Attachment #8964678 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8964678 [details] [diff] [review]
border-color.patch

Would be good for beta.
Attachment #8964678 - Flags: approval-comm-beta?
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/68ecbcd1c7b5
Use the WebExtension theme border color for AB and composer toolbox. r=jorgk DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
Attachment #8964678 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.