We currently have a bad heuristic in FrameLayerManager in order to compute the font smoothing background color for a piece of text when Mac vibrancy is involved. This heuristic was good enough for the sidebar and for context menus, but it breaks down once you have more complicated layer structures like in the tab bar. I think we should get rid of the brittle heuristic and let front-end explicitly pick the correct font smoothing background color for an element. This will avoid glitchy behavior such as: - using the wrong font smoothing background color for foreground tabs (bug 1386643) - loss of subpixel AA for background tabs on top of vibrancy while the tab bar scroll frame is active - loss of subpixel AA for background tabs on top of vibrancy while the hover animation is playing The current heuristic is: If you have an element with -moz-appearance: -moz-mac-vibrancy-light/dark, then the first PaintedLayer on top of that element will use a font smoothing background color that's informed by that -moz-appearance for all the text inside that layer. Instead, we could have a CSS property like this: -moz-font-smoothing-background-color: <color>; This would be transparent by default, and front-end CSS would set it to a hardcoded color that matches the background under the text. For example, for background tabs on top of dark vibrancy, it would set it to black, and for the foreground tab it would set it to a very light gray. It would only be respected in chrome stylesheets. Backstory about the font smoothing background color: Drawing subpixel-antialiased text usually only works correctly if you draw the text on top of an opaque background. The color of the destination pixel must be known in order to apply proper subpixel font smoothing. However, sometimes you need to draw text to an intermediate surface, and then at some later stage, composite that intermediate surface onto something that's opaque. And if the intermediate surface has a transparent background, that means that the destination pixels during the text drawing operation are completely transparent. This usually leads to loss of subpixel AA. One case where this happens is on Mac if you have text on top of vibrancy. In the vibrant parts of the window, the background inside the window's surface is completely transparent. The vibrant background gets added only at the WindowServer level, when all windows are composited to the screen. In order to get subpixel AA for text on top of vibrancy, macOS has a private API that lets you specify a "font smoothing background color" for the text, in addition to the color of the text itself. This lets the system apply subpixel AA to the text in such a way that, if the text is later composited on top of opaque pixels that are reasonably close to the predicted background color, things still look good. We obtain such a font smoothing background color based on the vibrancy type. E.g. for light vibrancy (used in the sidebar), it's a light gray; for dark vibrancy (used in the tab bar), it's black. By drawing our text with the correct font smoothing background color, text on top of vibrant backgrounds still looks good with proper subpixel AA even though it's drawn to completely transparent pixels internally.

The text on tabs look bad even if "Reduce transparency" in MacOS's accessibility preference is switched on. This settings remove vibrancy.

A screenshot with vibrancy switched off.

Comment on attachment 8907548 [details] Bug 1387594 - Respect the font smoothing background color in pushed layers again. This backs out bug 1386643. https://reviewboard.mozilla.org/r/179256/#review184362

Markus, any chance this work will land in 57? Wondering if bug 1389518 should block 57.

Yes, I'm targeting 57 with this work. I've tried to make these patches very low risk. I'm not 100% happy with them because of the small amount of extra maintenance work they mean when writing frontend CSS which uses vibrancy, but I believe I've chosen the least bad of all the options we have.

Comment on attachment 8907546 [details] Bug 1387594 - Stop getting the font smoothing background color from the theme. https://reviewboard.mozilla.org/r/179252/#review184610

Comment on attachment 8907547 [details] Bug 1387594 - Set the font smoothing background color based on the -moz-font-smoothing-background-color property. https://reviewboard.mozilla.org/r/179254/#review184612

Comment on attachment 8907545 [details] Bug 1387594 - Add a chrome-only CSS property called -moz-font-smoothing-background-color. https://reviewboard.mozilla.org/r/179250/#review184704 So r=dbaron with the comments below, if the special test run passes as described below. But I suspect it won't since you've skipped a pretty large swath of tests... I'm assuming it's OK to not have stylo changes for now given that this is chrome only (and given nsIDocument::UpdateStyleBackendType()), but you should probably run this by heycam or xidorn anyway. ::: layout/style/nsRuleNode.cpp:5330 (Diff revision 2) > StyleComplexColor::Auto(), > mPresContext, > ui->mCaretColor, conditions); > > + // -moz-font-smoothing-background-color: > + const nsCSSValue* fsbColor = aRuleData->ValueForFontSmoothingBackgroundColor(); fsbColor should probably be fsbColorValue ::: layout/style/test/property_database.js:8301 (Diff revision 2) > + > + gCSSProperties["-moz-font-smoothing-background-color"] = { > + // domProp: "MozFontSmoothingBackgroundColor", > + inherited: true, > + type: CSS_TYPE_LONGHAND, > + initial_values: [ "white", "#fff", "#ffffff", "rgb(255,255,255)", "rgba(255,255,255,1.0)", "rgba(255,255,255,42.0)" ], This looks wrong; the initial value should be transparent. Please fix this, and then do a try run with: - the property exposed to content - this section outside of the "if (false)" and make sure the tests pass in that configuration. (There's quite a bit of test coverage for this, and you're not benefiting from any of it!)

Comment on attachment 8907549 [details] Bug 1387594 - Add system colors for use in conjunction with -moz-font-smoothing-background-color and vibrant -moz-appearances. https://reviewboard.mozilla.org/r/179258/#review184710 ::: servo/components/style/properties/longhand/color.mako.rs:43 (Diff revision 2) > + -moz-mac-vibrancy-light -moz-mac-vibrancy-dark -moz-mac-menupopup > + -moz-mac-menuitem -moz-mac-active-menuitem -moz-mac-source-list > + -moz-mac-source-list-selection -moz-mac-active-source-list-selection > + -moz-mac-tooltip This isn't the right way to land things in servo. I believe you need to make a pull to servo in github. (Also see previous review.) ::: widget/cocoa/nsLookAndFeel.mm:309 (Diff revision 2) > break; > case eColorID__moz_nativehyperlinktext: > // There appears to be no available system defined color. HARDCODING to the appropriate color. > aColor = NS_RGB(0x14,0x4F,0xAE); > break; > + // The following colors are supposed to be used as font-smoothing background do you need to make these changes to widget/uikit as well?

Comment on attachment 8907550 [details] Bug 1387594 - Sprinkle -moz-font-smoothing-background-color declarations over the CSS. https://reviewboard.mozilla.org/r/179260/#review184920

(In reply to David Baron :dbaron: ⌚️UTC-7 (catching up on backlog from vacation) from comment #21) > I'm assuming it's OK to not have stylo changes for now given that this is > chrome only (and given nsIDocument::UpdateStyleBackendType()), but you > should probably run this by heycam or xidorn anyway. Looking at the style sheets it's used in, should be OK. Though do test that they have the desired effect even with stylo pref on, just to be sure. Please file a blocker of bug 1294570 to implement this in Servo.

Comment on attachment 8907545 [details] Bug 1387594 - Add a chrome-only CSS property called -moz-font-smoothing-background-color. https://reviewboard.mozilla.org/r/179250/#review184704 I filed bug 1399834 about implementing this property in stylo. > This looks wrong; the initial value should be transparent. > > Please fix this, and then do a try run with: > - the property exposed to content > - this section outside of the "if (false)" > and make sure the tests pass in that configuration. (There's quite a bit of test coverage for this, and you're not benefiting from any of it!) I've changed it to [ "transparent" ]. I've also kicked off a try push with the property enabled for content at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83e2ecaa416c5354fb1aa7a949d5a773ef0665f . I'm wouldn't be surprised if there are some failures in the animation tests because some of those tests have their own lists of properties, which I didn't check or adjust.

Comment on attachment 8907549 [details] Bug 1387594 - Add system colors for use in conjunction with -moz-font-smoothing-background-color and vibrant -moz-appearances. https://reviewboard.mozilla.org/r/179258/#review184710 > This isn't the right way to land things in servo. I believe you need to make a pull to servo in github. (Also see previous review.) I've removed this part from the patch and will create a pull request as soon as the servo build finishes. > do you need to make these changes to widget/uikit as well? No; iOS does not have subpixel AA and won't benefit from font smoothing background colors.

(In reply to Cameron McCormack (:heycam) from comment #24) > (In reply to David Baron :dbaron: ⌚️UTC-7 (catching up on backlog from > vacation) from comment #21) > > I'm assuming it's OK to not have stylo changes for now given that this is > > chrome only (and given nsIDocument::UpdateStyleBackendType()), but you > > should probably run this by heycam or xidorn anyway. > > Looking at the style sheets it's used in, should be OK. Though do test that > they have the desired effect even with stylo pref on, just to be sure. > Please file a blocker of bug 1294570 to implement this in Servo. The patch has the desired effect both with both settings of the stylo pref. I filed bug 1399834.

(In reply to Markus Stange [:mstange] from comment #31) > I've also kicked off a try push with the property enabled for content at > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83e2ecaa416c5354fb1aa7a949d5a773ef0665f . New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e6540598c0322af65c45dea0f055ad0795bc332 In the previous one I had forgotten re-enable the line `domProp: "MozFontSmoothingBackgroundColor",`.

Try push looks good. The servo PR was reviewed so I'm just waiting for that to merge to autoland now.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/67ab42945b98 Add a chrome-only CSS property called -moz-font-smoothing-background-color. r=dbaron https://hg.mozilla.org/integration/autoland/rev/45225685bdbc Stop getting the font smoothing background color from the theme. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/7fae5971e174 Set the font smoothing background color based on the -moz-font-smoothing-background-color property. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/cc52c89aeb32 Respect the font smoothing background color in pushed layers again. This backs out bug 1386643. r=jrmuizel https://hg.mozilla.org/integration/autoland/rev/b049c00f7a8f Add system colors for use in conjunction with -moz-font-smoothing-background-color and vibrant -moz-appearances. r=dbaron https://hg.mozilla.org/integration/autoland/rev/3dcafbdc27cf Sprinkle -moz-font-smoothing-background-color declarations over the CSS. r=dao

https://hg.mozilla.org/mozilla-central/rev/67ab42945b98 https://hg.mozilla.org/mozilla-central/rev/45225685bdbc https://hg.mozilla.org/mozilla-central/rev/7fae5971e174 https://hg.mozilla.org/mozilla-central/rev/cc52c89aeb32 https://hg.mozilla.org/mozilla-central/rev/b049c00f7a8f https://hg.mozilla.org/mozilla-central/rev/3dcafbdc27cf