Running nightly 58.0a1 See attached screenshot. This is from our internal slack. The menu fonts are more affected than regular text. However, comparing webrender on/off fonts are definitely less clear overall with webrender enabled. Please let me know what other information I can provide.

This is us not implementing `-moz-osx-font-smoothing: grayscale;` yet, which websites use to get "thinner" fonts. (note that the left image has greyscale AA, and the right has subpixel AA)

The overall thrust of this fix is that we don't pass in AA to WebRender when we call PushGlyphs. At the time of TextDrawTarget::FillGlyphs we have this AA info, so we just need to pass it in. In particular, this is where the grayscale setting for Mac fonts would normally be getting passed in. What we are also missing, however, is tracking of the nsDisplayItem's mDisableSubpixelAA setting, which I needed to make sure gets passed to TextDrawTarget's constructor when we set it up. Also to make the BulletRenderer work properly I had to modify DrawTargetCapture::ContainsOnlyColoredGlyphs to finally export the AA information it has collected too.

Comment on attachment 8920151 [details] [diff] [review] pass AA via GlyphOptions to WR PushGlyphs Review of attachment 8920151 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, just needs to delete some code we won't need. :) ::: gfx/2d/2D.h @@ +1448,5 @@ > */ > virtual bool ContainsOnlyColoredGlyphs(RefPtr<ScaledFont>& aScaledFont, > Color& aColor, > + std::vector<Glyph>& aGlyphs, > + AntialiasMode& aAntialiasMode) = 0; So with my patch in Bug 1407767 (which is approved but not yet checked in) we shouldn't be using ContainsOnlyColoredGlyphs at all anymore. I suggest basing this patch on that one, since that will minimize churn (sorry for the wasted work!) ::: gfx/layers/wr/WebRenderBridgeChild.h @@ +131,5 @@ > gfx::ScaledFont* aFont, const wr::ColorF& aColor, > const StackingContextHelper& aSc, > const wr::LayerRect& aBounds, const wr::LayerRect& aClip, > + bool aBackfaceVisible, > + const wr::GlyphOptions* aGlyphOptions = nullptr); And if ContainsOnlyColoredGlyphs is out of the picture, we should always be passing this, so I suggest making this a mandatory const reference. ::: layout/generic/TextDrawTarget.h @@ +54,5 @@ > nsDisplayItem* aItem, > nsRect& aBounds) > : mBuilder(aBuilder), mSc(aSc), mManager(aManager) > { > + SetPermitSubpixelAA(!aItem->IsSubpixelAADisabled()); Mild concern that this will get clobbered by anyone calling SetPermitSubpixelAA afterwards. However it *should* be fine, since no one has any reason to set it to true? ::: layout/generic/nsBulletFrame.cpp @@ +317,5 @@ > RefPtr<nsFontMetrics> mFontMetrics; > nsPoint mPoint; > RefPtr<ScaledFont> mFont; > nsTArray<layers::GlyphArray> mGlyphs; > + AntialiasMode mAntialiasMode; Can also be removed in new patch (no state).

v2, rebased on top of bullet renderer changes and much simplified.

Pushed by lsalzman@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/51d3273fdfc0 pass AA via GlyphOptions to WR PushGlyphs. r=gankro

I think this patch conflates two ways of grayscale rendering that need to stay separate on Mac. We made the same mistake with DrawTargetSkia first but then fixed it in bug 1260454. As a result, if you ask DrawTargetSkia on Mac to render text, the following settings will cause the text to look different: - SetPermitSubpixelAA(false), AntialiasMode::SUBPIXEL: causes gamma-aware blending of regular-thickness text - SetPermitSubPixelAA(any), AntialiasMode::GRAY: causes non-gamma-aware blending of thin text The regular-thickness text mask was obtained by asking CG to render with subpixel AA and then averaging the color channels. The thin text was obtained by asking CG to render with CGContextSetShouldSmoothFonts(cg, false).

(In reply to Markus Stange [:mstange] from comment #7) > I think this patch conflates two ways of grayscale rendering that need to > stay separate on Mac. We made the same mistake with DrawTargetSkia first but > then fixed it in bug 1260454. As a result, if you ask DrawTargetSkia on Mac > to render text, the following settings will cause the text to look different: > - SetPermitSubpixelAA(false), AntialiasMode::SUBPIXEL: causes gamma-aware > blending of regular-thickness text > - SetPermitSubPixelAA(any), AntialiasMode::GRAY: causes non-gamma-aware > blending of thin text > > The regular-thickness text mask was obtained by asking CG to render with > subpixel AA and then averaging the color channels. The thin text was > obtained by asking CG to render with CGContextSetShouldSmoothFonts(cg, > false). That sounds insane and would require some other way to signal to WR what is essentially a new, hidden, platform-dependent level of AA for Mac only. There's essentially no way to tell it this right now. :( It would ideally be better if those collapsed to the same type of AA, but if we need it I guess I could go back and add something to FontInstancePlatformOptions to work around it.

I would prefer to add something to FontInstancePlatformOptions. People clearly care about their thin text; all Mac browsers support it and we get bugs filed every time we break it. And we don't want to remove the other way of rendering grayscale text because that other way can be used when we want subpixel AA but have to fall back due to e.g. filters or opacity; since it looks a lot closer to the subpixel AA rendering, the transition between subpixel AA and fallback grayscale is not as jarring as a transition between subpixel AA and thin grayscale would be.

Reopening to deal with the fallout as Markus described.

WebRender PR necessary to communicate smoothing options from Gecko: https://github.com/servo/webrender/pull/1925

Nothing crazy going on here, just plumbing the setting from gfxMacFont into ScaledFontMac so that we can then finally chuck it over to WebRender within FontInstancePlatformOptions. This depends on the WebRender PR https://github.com/servo/webrender/pull/1925 which adds the font_smoothing option for the receiving side and fixes how Mac handles grayscale AA.

Comment on attachment 8921600 [details] [diff] [review] send gfxMacFont font smoothing setting to WebRender Review of attachment 8921600 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! ::: gfx/webrender_bindings/webrender_ffi_generated.h @@ +798,5 @@ > #endif > > #if defined(XP_MACOSX) > struct FontInstancePlatformOptions { > + bool font_smoothing; Oh, so you folded the binding change into this patch. So this patch will need to become part of the next webrender update patch set?

(In reply to Markus Stange [:mstange] from comment #13) > Comment on attachment 8921600 [details] [diff] [review] > send gfxMacFont font smoothing setting to WebRender > > Review of attachment 8921600 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, thanks! > > ::: gfx/webrender_bindings/webrender_ffi_generated.h > @@ +798,5 @@ > > #endif > > > > #if defined(XP_MACOSX) > > struct FontInstancePlatformOptions { > > + bool font_smoothing; > > Oh, so you folded the binding change into this patch. So this patch will > need to become part of the next webrender update patch set? Oh, I guess that's an accident. If whoever updates WR (kats?) also runs cbindgen, then this part can be skipped, but presumably patch should be smart enough to notice.

Yeah the webrender_ffi_generated.h changes will end up in the WR update patchset, and then we can land this after that's in. When this patch gets rebased on top of the WR update it will lose the changes to webrender_ffi_generated.h since they will already be made.

This can now be landed.

Pushed by lsalzman@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9f8970cfe1 send gfxMacFont font smoothing setting to WebRender. r=mstange