1008424 - Remove unintended gradient from the tab toolbar's background

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

This is causing bug 1000754 on Windows.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

So... I don't know what adding the negative margins in the title bar height calculation was good for. On Windows, we need the titlebar to extend behind the border between the tab bar and the nav bar, as that border has transparency (see toolbarShadowColor).

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

fixed a small mistake in the previous patch

Comment 4

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

Comment on attachment 8421508 [details] [diff] [review]
patch

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

This looks pretty good - but I am noticing a likely unintentional inner highlight on the nav-bar on Windows XP.

I'll have a closer look on Tuesday (Monday is a holiday in Canada), but what I'm going to do is run MattN's screenshot tool and do a before and after comparison, and make sure we're not introducing anything new here.

I'll post a report on Tuesday.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

unbitrotted

Comment 6

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

Ok, running this through some paces...

Comment 7

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

Comment on attachment 8425507 [details] [diff] [review]
patch

The code looks fine, but I think this reintroduces bug 885062 (to a lesser, but still noticeable degree).

Specifically, please see the disconnect between the titlebar and the top of the nav-bar in the padding area while in customize mode: http://i.imgur.com/4NMspLr.png

This doesn't appear to affect Windows Classic themes.

The inner-highlight issue I mentioned earlier has vanished.

I have not yet found any other issues.

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2

Is this better? Unfortunately and strangely, I don't find my old laptop where I had Windows XP installed...

Comment 9

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

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

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

Ok, I tested this Windows XP (Luna, Classic), Windows 7 (Classic, Aero Basic, Aero Glass) and Windows 8. Looks good to me.

You'll probably want someone from core to look at the widget/ change.

Comment 10

[Dão Gottwald [:dao]]

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

Jim, could you please have a look at the tiny change in widget code? I think you wrote this. It's not clear to me why you excluded the bottom pixel row when drawing the title bar; dealing with this is pretty confusing on the front-end side.

Comment 11

[Jim Mathies [:jimm]]

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

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

Hmm, not sure and unfortunately I didn't add a comment. The rect was probably slightly off and the adjustment compensated for that. Regardless, if it looks right now, fine with me.

Hopefully in a few years we can depreciate all this old drawing code now that classic mode is officially dead.

Comment 12

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/2236b3fc98ae

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2236b3fc98ae