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

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: mstange, Assigned: dao)

Tracking

(Depends on 1 bug, Blocks 3 bugs, {perf})

Trunk
Firefox 58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Dão, how would you prefer this be fixed?
Flags: needinfo?(dao+bmo)
Assignee

Comment 2

2 years ago
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).
Flags: needinfo?(dao+bmo)
I'd prefer to fix this sooner than 57 because it would fix the tab overflow regression.
I mean the regression where the new fade-out effect on overflowing tab text makes us lose subpixel AA.
Assignee

Comment 5

2 years ago
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.
Assignee

Comment 6

2 years ago
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.
Assignee

Updated

2 years ago
Blocks: 1366240
Assignee

Updated

2 years ago
Blocks: 1370064
Assignee

Updated

2 years ago
Flags: needinfo?(dao+bmo)
Assignee

Updated

2 years ago
Depends on: 1344910
Assignee

Updated

2 years ago
Depends on: 1379938
Assignee

Updated

2 years ago
Blocks: 1378097
Assignee

Updated

2 years ago
Blocks: 1387354
Flags: needinfo?(dao+bmo)
Assignee

Updated

2 years ago
Blocks: 1391209
Assignee

Updated

2 years ago
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Component: Layout → Theme
Flags: qe-verify-
Priority: -- → P1
Product: Core → Firefox
Whiteboard: [reserve-photon-visual]
Assignee

Updated

2 years ago
Attachment #8911468 - Flags: review?(nhnt11) → review?(jhofmann)

Comment 8

2 years ago
mozreview-review
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
Attachment #8911468 - Flags: review+
Assignee

Updated

2 years ago
Attachment #8911468 - Flags: review?(jhofmann)

Comment 9

2 years ago
Pushed by dgottwald@mozilla.com:
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

2 years ago
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.
(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

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c4cf992aa65c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Assignee

Updated

2 years ago
Blocks: 1389569

Updated

2 years ago
Depends on: 1403951

Updated

2 years ago
Depends on: 1403973

Comment 14

2 years ago
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/
Backed out for causing bug 1403951 and leaving many users with broken nightlies.

https://hg.mozilla.org/mozilla-central/rev/3177c1b64ffe7ba5c08851791bd495421dcedb05
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 58 → ---
(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.
Status: REOPENED → ASSIGNED

Comment 17

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/93561ea081ff
Stop using -moz-appearance: -moz-win-glass with the Windows 10 default theme. r=nhnt11
Comment hidden (typo)
Assignee

Updated

2 years ago
Depends on: 1404074

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/93561ea081ff
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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
Assignee

Updated

2 years ago
Depends on: 1404386
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?
Flags: needinfo?(dao+bmo)
Assignee

Comment 23

2 years ago
(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.
Flags: needinfo?(dao+bmo)
Assignee

Updated

2 years ago
No longer depends on: 1403973

Updated

2 years ago
Depends on: 1405228
Assignee

Comment 24

2 years ago
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]: /
Attachment #8914651 - Flags: approval-mozilla-beta?
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+
Attachment #8914651 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Updated

2 years ago
Depends on: 1405593
Assignee

Updated

2 years ago
No longer depends on: 1405593
Assignee

Updated

2 years ago
Depends on: 1405593

Updated

11 months ago
Depends on: 1468335
Assignee

Updated

8 months ago
Depends on: 1496322
You need to log in before you can comment on or make changes to this bug.