908665 - Title bar gradient on the tab bar shouldn't care about the sizemode or menu bar state

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: gradient.diff

The selector is still in the pre-Australis state and doesn't match Australis reality with regards to tabs being rendered in the title bar.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch v2

... and during self-review, I realized that [sizemode="maximized"] also excluded full-screen mode, which AFAIK we need to keep excluding.

Comment 2

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #1)
> Created attachment 794677 [details] [diff] [review]
> patch v2
> 
> ... and during self-review, I realized that [sizemode="maximized"] also
> excluded full-screen mode, which AFAIK we need to keep excluding.

Can you put up some screenshots and get ui-review on this first, especially regarding restored windows with menubars?

Comment 3

[Dão Gottwald [:dao]]

I don't think this needs ui-review. The selector is obviously just broken, so we should fix that. If somebody comes up with a better design, we can consider that, but that's orthogonal to the fact that this code should no longer care about the menu bar state or whether windows are restored or maximized.

Comment 4

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #3)
> I don't think this needs ui-review. The selector is obviously just broken,
> so we should fix that. If somebody comes up with a better design, we can
> consider that, but that's orthogonal to the fact that this code should no
> longer care about the menu bar state or whether windows are restored or
> maximized.

The gradient's impact is going to be very different in a context where the window isn't maximized, and/or has a menubar above it without that gradient (but *with* a native titlebar gradient, leading to something like a bottom-left-to-top-right combined gradient). It also seems from the discussion when it was originally added (bug 624292 comment 12) that it was only chosen because we didn't yet have code to extend the titlebar down the right number of pixels. I.e., it seems like it was a low-risk workaround for an issue that we would have liked to have fixed otherwise (and in fact, is now fixed). Now extending the reach of that workaround without reconsidering seems imprudent.

We're not in a rush, as it's not like the current situation has had people screaming about it. We've found issues using automated testing of various combinations, and we should work to correct the issues. Let's get UI/UX input on this and figure out what we actually want, and then get it right.

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > I don't think this needs ui-review. The selector is obviously just broken,
> > so we should fix that. If somebody comes up with a better design, we can
> > consider that, but that's orthogonal to the fact that this code should no
> > longer care about the menu bar state or whether windows are restored or
> > maximized.
> 
> The gradient's impact is going to be very different in a context where the
> window isn't maximized, and/or has a menubar above it without that gradient
> (but *with* a native titlebar gradient, leading to something like a
> bottom-left-to-top-right combined gradient).

I don't really see that difference. The combined gradient exists in maximized windows as well, and I actually agree it's kind of ugly. Again, that's a matter of figuring out a better design, which can happen independently from what this bug is about.

> It also seems from the
> discussion when it was originally added (bug 624292 comment 12) that it was
> only chosen because we didn't yet have code to extend the titlebar down the
> right number of pixels. I.e., it seems like it was a low-risk workaround for
> an issue that we would have liked to have fixed otherwise (and in fact, is
> now fixed).

Yes, the context has changed. The title bar now reaches down for enough, but at the same time background tabs became transparent, creating a new problem.

> Now extending the reach of that workaround without reconsidering
> seems imprudent.

I'm all for reconsidering. That's why I decided to file a new bug for this patch rather than hijacking bug 907336.

Comment 6

[Dão Gottwald [:dao]]

> The title bar now reaches down for enough

s/for/far/

I should sanity-check my stuff before submitting it...

Comment 7

[:Gijs (he/him)]

Comment on attachment 794677 [details] [diff] [review]
patch v2

OK, but then let's please make sure we fix this situation before we hit aurora... :-)

Comment 8

[Dão Gottwald [:dao]]

https://hg.mozilla.org/projects/ux/rev/e2a6a3c10b0d

Comment 9

[:Gijs (he/him)]

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