Closed Bug 1387594 Opened 3 years ago Closed 2 years ago

Add a chrome-only CSS property to select the font smoothing background color

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(8 files)

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.
Blocks: 1389518
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.
Priority: -- → P3
Assignee: nobody → mstange
Status: NEW → ASSIGNED
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
Attachment #8907548 - Flags: review?(jmuizelaar) → review+
Markus, any chance this work will land in 57? Wondering if bug 1389518 should block 57.
Flags: needinfo?(mstange)
Blocks: 1389476
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.
Flags: needinfo?(mstange)
Comment on attachment 8907546 [details]
Bug 1387594 - Stop getting the font smoothing background color from the theme.

https://reviewboard.mozilla.org/r/179252/#review184610
Attachment #8907546 - Flags: review?(matt.woodrow) → review+
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
Attachment #8907547 - Flags: review?(matt.woodrow) → review+
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!)
Attachment #8907545 - Flags: review?(dbaron) → review+
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?
Attachment #8907549 - Flags: review?(dbaron) → review+
Comment on attachment 8907550 [details]
Bug 1387594 - Sprinkle -moz-font-smoothing-background-color declarations over the CSS.

https://reviewboard.mozilla.org/r/179260/#review184920
Attachment #8907550 - Flags: review?(dao+bmo) → review+
(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
Depends on: 1402416
Depends on: 1445671
No longer depends on: 1445671
Regressions: 1445671
You need to log in before you can comment on or make changes to this bug.