1439734 - Allow setting the tab line color

Status: RESOLVED FIXED

(WebExtensions :: Themes, enhancement)

Description

[Mike Conca [:mconca]]

It would be nice to independently style the line at the top of the active tab.

Right now, it appears that the tab line color inherits the color set for accentcolor. Some themes that use background images set both the accentcolor and text color to the same value, so that both the tab line and tab text are visible.

Unfortunately, other add-ons that try to "do the right thing" and read the accent color and text color and use those values end up "invisible" because those properties are set to the same value.

The largest example of this is trying to use Martell's Quantum Lights Theme (https://addons.mozilla.org/en-US/firefox/addon/quantum-lights-dynamic/) with the Tree Style Tabs add-on (https://addons.mozilla.org/en-US/firefox/addon/tree-style-tab/).  Martell uses a background image and sets both the accentcolor and textcolor to white, so both will show up against his background.

Tree Style Tabs tries to use the accentcolor and textcolor of the current theme to render its tabs, but the tabs are unreadable since those values are exactly the same.

Allowing themes to set the tab line color separately permits the line to show up against background images while simultaneously permitting contrasting accent and text colors so that other extensions don't break.

Comment 1

[Mike de Boer [:mikedeboer]]

This sounds like a good-next-bug to me, not terribly complicated. :dao and/ or :johannh would be able to provide help, if necessary, and review patches.

Comment 2

[Mike Conca [:mconca]]

:mconley I'm wondering if we could get this added to the Dark Themes Darkening project for MSU. It seems very similar to a lot of the bugs they are already working on.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I'm helping to mentor the students, but I'd say I'm less able to say with certainty what should and should not be exposed via our Theme API. This sounds reasonable to me, but we might want a third opinion.

dao, what say you? Does this sound like a reasonable thing to expose via the WebExtension API? Is this line at the top of the tab something that could be covered by a pre-existing variable? Do we foresee any future-proofing difficulties if the UI were to change in 5 years time and the line doesn't exist anymore?

Comment 4

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #3)
> I'm helping to mentor the students, but I'd say I'm less able to say with
> certainty what should and should not be exposed via our Theme API. This
> sounds reasonable to me, but we might want a third opinion.
> 
> dao, what say you? Does this sound like a reasonable thing to expose via the
> WebExtension API?

Yeah, we could.

> Is this line at the top of the tab something that could be
> covered by a pre-existing variable?

Not really. Our options right now are to use the accent color (we do this as of bug 1388138), the text color, the OS-provided highlight color, or a fixed photon color.

> Do we foresee any future-proofing
> difficulties if the UI were to change in 5 years time and the line doesn't
> exist anymore?

That's not a problem. If we decided to remove this UI element at some point, the theme property would just stop doing anything.

Comment 5

[Tim Nguyen :ntim]

Attachment: Bug 1439734 - Allow WebExtension themes to set the tab line color.

Review commit: https://reviewboard.mozilla.org/r/224162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224162/

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8954976 [details]
Bug 1439734 - Allow WebExtension themes to set the tab line color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224162/diff/1-2/

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8954976 [details]
Bug 1439734 - Allow WebExtension themes to set the tab line color.

https://reviewboard.mozilla.org/r/224162/#review230482

::: toolkit/components/extensions/ext-theme.js:151
(Diff revision 2)
>            this.lwtStyles.icon_color = cssColor;
>            break;
>          case "icons_attention":
>            this.lwtStyles.icon_attention_color = cssColor;
>            break;
> +        case "selected_tab_line":

Note to self: Rename to tab_selected_line or simply tab_line to be consistent with tab_background_text

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8954976 [details]
Bug 1439734 - Allow WebExtension themes to set the tab line color.

https://reviewboard.mozilla.org/r/224162/#review230800

::: toolkit/components/extensions/test/browser/browser_ext_themes_tab_line.js:12
(Diff revision 2)
> +  const TAB_LINE_COLOR = "#9400ff";
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "theme": {
> +        "images": {
> +          "headerURL": "image1.png",

headerURL is now optional so you can remove it from the test.

::: toolkit/components/extensions/test/browser/browser_ext_themes_tab_line.js:21
(Diff revision 2)
> +    files: {
> +      "image1.png": BACKGROUND,
> +    },

Ditto.

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8954976 [details]
Bug 1439734 - Allow WebExtension themes to set the tab line color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224162/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7f3f1c04fcd
Allow WebExtension themes to set the tab line color. r=jaws

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/f7f3f1c04fcd

Comment 12

[Will Bamberg [:wbamberg]]

:ntim, I think you have updated the docs page and data for this, so if you are happy with the screenshots we can mark it dev-doc-complete.