Closed Bug 1366405 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: mstange, Assigned: dao)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: perf, Whiteboard: [reserve-photon-visual])

Attachments

(2 files, 1 obsolete file)

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)
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.
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.
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.
Blocks: 1366240
Blocks: 1370064
Flags: needinfo?(dao+bmo)
Depends on: 1344910
Depends on: 1379938
Blocks: 1378097
Blocks: 1387354
Flags: needinfo?(dao+bmo)
Blocks: 1391209
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Component: Layout → Theme
Flags: qe-verify-
Priority: -- → P1
Product: Core → Firefox
Whiteboard: [reserve-photon-visual]
Attachment #8911468 - Flags: review?(nhnt11) → review?(jhofmann)
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+
Attachment #8911468 - Flags: review?(jhofmann)
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
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.
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 :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Blocks: 1389569
Depends on: 1403951
Depends on: 1403973
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
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
Depends on: 1404074
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 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
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)
(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)
No longer depends on: 1403973
Depends on: 1405228
Attached patch patch for uplift (deleted) — Splinter Review
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+
Depends on: 1405593
No longer depends on: 1405593
Depends on: 1405593
Depends on: 1468335
Depends on: 1496322
Regressions: 1614218
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: