1366405 - We're forcing the window's main layer to be transparent on Windows 10 because we think it has a glass effect

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P1)

Description

[Markus Stange [:mstange]]

As far as I know, there is no glass effect on Windows 10. Nevertheless, our browser chrome is setting -moz-appearance: -moz-win-glass on the <window> element, which causes us to hit this code:
http://searchfox.org/mozilla-central/rev/02cae69058d41e2c6b635edccbec91a6ca2cb57f/layout/painting/nsDisplayList.cpp#3819

This causes us to use a transparent surface for the main layer of the window, which may have a performance cost and also makes it harder for us to use subpixel text AA inside masks, which are used for overflowing tabs and the URL bar, see bug 1307833.

Comment 1

[Markus Stange [:mstange]]

Dão, how would you prefer this be fixed?

Comment 2

[Dão Gottwald [:dao]]

Bug 1169911 contains some background info, but be warned that the discussion might be in parts misleading rather than enlightening. jimm might know more.

One thing to note is for Firefox 57, this might not be a problem:

> At least on win10, without it the titlebar becomes the "accent" color
(bug 1169911 comment 16)

... because we actually want to use the accent color (bug 1196266).

Comment 3

[Markus Stange [:mstange]]

I'd prefer to fix this sooner than 57 because it would fix the tab overflow regression.

Comment 4

[Markus Stange [:mstange]]

I mean the regression where the new fade-out effect on overflowing tab text makes us lose subpixel AA.

Comment 5

[Dão Gottwald [:dao]]

Right... I think jimm has the expertise you're looking for. As you can tell by reading bug 1169911, gijs and I were mostly guessing. We don't really know what's going on on the widget side. I can play around with this and see what breaks if we remove -moz-appearance: -moz-win-glass, but I'm not sure this will lead anywhere.

Comment 6

[Dão Gottwald [:dao]]

It seems that removing -moz-appearance: -moz-win-glass only removes the window's top border, which I guess is an unavoidable consequence of setting chromemargin="0,2,2,2". With bug 1344910 fixed, we could potentially add that border manually via CSS.

Comment 7

[Dão Gottwald [:dao]]

Attachment: Bug 1366405 - Stop using -moz-appearance: -moz-win-glass with the Windows 10 default theme.

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

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8911468 [details]
Bug 1366405 - Stop using -moz-appearance: -moz-win-glass with the Windows 10 default theme.

https://reviewboard.mozilla.org/r/182930/#review189106

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4cf992aa65c
Stop using -moz-appearance: -moz-win-glass with the Windows 10 default theme. r=nhnt11

Comment 10

[Eddward]

Looks like this (accidentally?) also fixed Bug 1389569. Is it possible? I can not reproduce it using latest Autoland build. Bug 1378097 and Bug 1391209 are also fixed, but it is already known AFAIK. 
Perhaps we should consider this also for 57? Looks like it fixed a lot Win 10 bugs.

Comment 11

[Johann Hofmann [:johannh]]

(In reply to Eddward from comment #10)
> Looks like this (accidentally?) also fixed Bug 1389569. Is it possible?

That sounds plausible. Yes, we'll certainly uplift if it sticks on Nightly.

Comment 12

[Eddward]

Thanks for positive info. Fingers crossed :)
I tried build before this patch and build with this patch and it indeed resolved Bug 1389569.
Moreover I noticed one more bug which is fixed by this patch. There was some kind of font jittering in URL bar when hovering mouse over other tabs and now it is perfectly stable text without any visible glitch. Not sure if Bug 1366240 is about it, but anyway, awesome patch :)

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Joel]

This glass effect they are talking about in Windows 10 is the acrylic (Microsoft Fluent Design System), right?
Like Edge and other apps have with the Fall creators Update???
https://fluent.microsoft.com/

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1403951 and leaving many users with broken nightlies.

https://hg.mozilla.org/mozilla-central/rev/3177c1b64ffe7ba5c08851791bd495421dcedb05

Comment 16

[Markus Stange [:mstange]]

(In reply to Joel from comment #14)
> This glass effect they are talking about in Windows 10 is the acrylic
> (Microsoft Fluent Design System), right?
> Like Edge and other apps have with the Fall creators Update???
> https://fluent.microsoft.com/

Firefox doesn't have support for the new acrylic effect. Once it adds support for it, the window's main layer will probably need to become transparent again. But for now, having it be transparent was only an unintended side-effect caused by leftover code from the Windows 7 glass effect, and no actual transparency was used.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93561ea081ff
Stop using -moz-appearance: -moz-win-glass with the Windows 10 default theme. r=nhnt11

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1366405 - Stop using -moz-appearance: -moz-win-glass with the Windows 10 default theme.

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

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8913350 [details]
Bug 1366405 - Stop using -moz-appearance: -moz-win-glass with the Windows 10 default theme.

Sorry, I screwed up a push to mozreview.

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/93561ea081ff

Comment 21

[Ionuț Goldan [:igoldan]]

Improvement noticed:

== Change summary for alert #9705 (as of September 27 2017 09:30 UTC) ==

Improvements:

  6%  tresize windows10-64 pgo e10s     9.43 -> 8.86

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9705

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Is this something you intend to ride the 58 train or did you want to uplift to Beta to fix the various deps affecting 57?

Comment 23

[Dão Gottwald [:dao]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Is this something you intend to ride the 58 train or did you want to uplift
> to Beta to fix the various deps affecting 57?

Going to request approval soon.

Comment 24

[Dão Gottwald [:dao]]

Attachment: patch for uplift

Approval Request Comment
[Feature/Bug causing the regression]: old bug that surfaces in new ways with photon
[User impact if declined]: bug 1366240, bug 1370064, bug 1378097, bug 1387354, bug 1389569, bug 1391209, plus tresize perf (comment 21)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: we should verify the bugs that this fixed
[List of other uplifts needed for the feature/fix]: I'm including bug 1403951's and bug 1404386's patches here
[Is the change risky?]: somewhat
[Why is the change risky/not risky?]: This already caused regressions but they're taken care of. Bug 1404074 is still open but a minor issue. I'm not too concerned about further regressions at this point.
[String changes made/needed]: /

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914651 [details] [diff] [review]
patch for uplift

Sounds like a must fix for 57, has some risk which we can tolerate in early part of this cycle, Beta57+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7c374b1db6da