Closed
Bug 1366405
Opened 7 years ago
Closed 6 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)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 58
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)
59 bytes,
text/x-review-board-request
|
nhnt11
:
review+
|
Details |
7.80 KB,
patch
|
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•7 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)
Reporter | ||
Comment 3•7 years ago
|
||
I'd prefer to fix this sooner than 57 because it would fix the tab overflow regression.
Reporter | ||
Comment 4•7 years ago
|
||
I mean the regression where the new fade-out effect on overflowing tab text makes us lose subpixel AA.
Assignee | ||
Comment 5•7 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•7 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•7 years ago
|
Flags: needinfo?(dao+bmo)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Component: Layout → Theme
Flags: qe-verify-
Priority: -- → P1
Product: Core → Firefox
Whiteboard: [reserve-photon-visual]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8911468 -
Flags: review?(nhnt11) → review?(jhofmann)
Comment 8•6 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•6 years ago
|
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
Comment 10•6 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.
Comment 11•6 years ago
|
||
(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•6 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c4cf992aa65c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 14•6 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/
Comment 15•6 years ago
|
||
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 → ---
Reporter | ||
Comment 16•6 years ago
|
||
(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.
Updated•6 years ago
|
Status: REOPENED → ASSIGNED
Comment 17•6 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 (mozreview-request, typo) |
Comment hidden (typo) |
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/93561ea081ff
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 21•6 years ago
|
||
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•6 years ago
|
||
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?
status-firefox57:
--- → affected
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 23•6 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 | ||
Comment 24•6 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+
Comment 26•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/7c374b1db6da
You need to log in
before you can comment on or make changes to this bug.
Description
•