Failures on Edge due to anti-aliasing of Ahem (formerly also Safari)

smfr commented 4 years ago

Digging into Safari CSS test failures, many of them are caused by differences in font rendering in tests that assume that Ahem glyphs will produce sharp-edged boxes that can be compared with CSS backgrounds. Some random examples:

https://wpt.fyi/results/css/CSS2/text/painting-order-underline-001.xht?label=master&label=experimental&aligned https://wpt.fyi/results/css/CSS2/text/white-space-applies-to-008.xht?label=master&label=experimental&aligned https://wpt.fyi/results/css/CSS2/linebox/line-height-039.xht?label=master&label=experimental&aligned https://wpt.fyi/results/css/css-multicol/multicol-basic-001.html?label=master&label=experimental&aligned

Interestingly, many tests that both Edge and Safari fail but Chrome and Firefox pass are in this category.

Adding <meta name=fuzzy> to all these tests will be very laborious.

I wonder if this behavior changed with Ahem-the-web-font (@gsnedders)?

smfr commented 4 years ago

@foolip since we were talking about this at the webkit contributors meeting.

smfr commented 4 years ago

See also issue #5498

smfr commented 4 years ago

@litherum does https://github.com/web-platform-tests/wpt/issues/5498#issuecomment-292548863 apply to Cocoa platforms?

foolip commented 4 years ago

https://github.com/web-platform-tests/wpt/issues/5498 is about hidpi and our Safari runs on Azure Pipelines actually have a 1:1 pixel ratio, but perhaps there’s another connection?

Is there a way to disable font antialiasing in Safari or in macOS system-wise? If so we could to a trial run and compare how many tests are affected by it.

Do WebKit layout tests run with any extra settings to avoid this issue, or are these tests failing in the WebKit imported copy too?

jgraham commented 4 years ago

We spent a bunch of time looking at this for Firefox runs on MacOS. I think the final approach was to disable antialiasing specificaly for Ahem in test runs:

https://searchfox.org/mozilla-central/source/testing/profiles/web-platform/user.js#26 https://searchfox.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#795

@gsnedders and I also spent some time looking at modifying Ahem itself to disable AA, but without any success; the relevant features seemed to be set already just ignored by the renderers.

@jfkthame might also have some insight here.

gsnedders commented 4 years ago

@gsnedders and I also spent some time looking at modifying Ahem itself to disable AA, but without any success; the relevant features seemed to be set already just ignored by the renderers.

Here's a run where the gasp table is set to not do grayscale or grid-fitting, which clearly makes no difference here.

clopez commented 4 years ago

Related: #13010

clopez commented 4 years ago

A possible solution for Safari is disabling font antialiasing with the non-standard CSS -webkit-font-smoothing property.

The following patch makes the test css/CSS2/text/painting-order-underline-001-ref.xht pass on Safari TP

diff --git a/css/CSS2/text/painting-order-underline-001-ref.xht b/css/CSS2/text/painting-order-underline-001-ref.xht
index bec88a5a9b..b222bc1cd6 100644
--- a/css/CSS2/text/painting-order-underline-001-ref.xht
+++ b/css/CSS2/text/painting-order-underline-001-ref.xht
@@ -3,7 +3,11 @@
 <html xmlns="http://www.w3.org/1999/xhtml">

  <head>
-
+  <style>
+    html {
+      -webkit-font-smoothing: none;
+    }
+  </style>
   <title>CSS Reftest Reference</title>

   <link rel="author" title="Gérard Talbot" href="http://www.gtalbot.org/BrowserBugsSection/css21testsuite/" />
diff --git a/css/CSS2/text/painting-order-underline-001.xht b/css/CSS2/text/painting-order-underline-001.xht
index 9c01cbaa18..3c9d8a6e87 100644
--- a/css/CSS2/text/painting-order-underline-001.xht
+++ b/css/CSS2/text/painting-order-underline-001.xht
@@ -3,7 +3,11 @@
 <html xmlns="http://www.w3.org/1999/xhtml">

  <head>
-
+  <style>
+    html {
+      -webkit-font-smoothing: none;
+    }
+  </style>
   <title>CSS Test: 'underline' decoration painting order and descender</title>

   <link rel="author" title="Gérard Talbot" href="http://www.gtalbot.org/BrowserBugsSection/css21testsuite/" />

It is needed to set -webkit-font-smoothing: none both on the test and the reference because once you set it on the test (to the whole document) it not only applies to the Ahem font, but also to the other fonts that draw the test text (the usual: "Test passes if ...").

I have two ideas to automate this:

Idea 1: wptrunner would inject the required CSS to disable font-smoothing before running the test. Maybe this can be done with JS via WebDriver script execution. Something like

var elHead = document.getElementsByTagName('head')[0];
var elStyle = document.createElement('style')
elStyle.type = 'text/css';
elHead.appendChild(elStyle);
elStyle.innerHTML = 'html { -webkit-font-smoothing: none; }';

Perhaps instead of doing this indiscriminately for every test, it can be done only for the tests using Ahem font after they fail. Like:

run ref-test as usual
if test fails and test uses Ahem and browser is safari:
retry a second-time after setting -webkit-font-smoothing: none

Idea 2:

Another idea is to patch WebKit to disable font smoothing (or at least to set it by default to disabled instead of auto) when some magic environment variable like WEBKIT_DISABLE_FONT_SMOOTHING is present. And then pass down this environment variable to webkit from the WPT runner. Or instead of an environment variable, using a run-time setting variable is another idea.

WDYT?

For MS Edge maybe it works disabling font smoothing for the whole system in the Azure CI? No idea if it will work, never tested.

clopez commented 4 years ago

* Idea 2:
Another idea is to patch WebKit to disable font smoothing (or at least to set it by default to disabled instead of auto) when some magic environment variable like WEBKIT_DISABLE_FONT_SMOOTHING is present. And then pass down this environment variable to webkit from the WPT runner. Or instead of an environment variable, using a run-time setting variable is another idea.

Note that the WebKit layout test test-runner run-webkit-tests does something like that. So it happens that any of this test will pass on on WebKit if run as layout test, but fail if run with the WPT test runner

jgraham commented 4 years ago

If we could set a pref or an environment variable to disable font smoothing — for all text or Ahem in particular — that seems better to me than trying to hack the CSS provided to the tests, in case the latter has unexpected side effects. The fact that the layout test runner already does that does seem like evidence that this should be exposed to wpt as well.

foolip commented 4 years ago

@smfr if we had a way to disable font smoothing with defaults write com.apple.SafariTechnologyPreview WebKitSomethingSomething something I think that could resolve this issue.

smfr commented 4 years ago

That's the plan. We'll implement this in WebKit.

litherum commented 4 years ago

Ah yes, the good old WebKitSomethingSomething default. That one’s my favorite. Very powerful.

foolip commented 4 years ago

That's great, thanks @smfr!

@clopez can you follow that issue and see how the same thing might be exposed in WebKitGTK?

gsnedders commented 4 years ago

Someone (me?) should probably sit down and go through all the prefs Gecko sets and figure out what we need as prefs to alter behaviour and ensure we have them all documented.

litherum commented 4 years ago

By the way, @smfr landed his patch. We now unconditionally disable antialiasing for Ahem.

foolip commented 4 years ago

That's great! Do you know which STP release this will be in?

clopez commented 4 years ago

@clopez can you follow that issue and see how the same thing might be exposed in WebKitGTK?

I implemented it for WebKitGTK on r254567

However, after looking at the diff on wpt.fyi between the versions of WebKitGTK without the fix (on the left: r254504) and the one with it (on the right: r254674), I can't find any diff on the test run related to this change.

After some investigation I discovered that the issue with the Ahem font only seems to happens if you turn antialising on and disable font hinting. But if you enable font hinting, then the issue doesn't happen: tests pass and the squares of Ahem render like without antialias, even if you still let antialiasing enabled (at least on Linux).

And that setting (antialiasing=1 + hinting=0) is how we run the layout tests in WebKitGTK (we disable font hinting via gtk_settings on the layout test runner), so we were seeing many failures related to the Ahem font antialiasing issue when running layout tests for WebKitGTK. It also happens to be how I have configured my fontconfig settings (I don't like hinting), so I could easily reproduce those failures.

However, most Linux distributions default to enable font hinting. Therefore, in the WPT CI (Ubuntu 18.04), which uses default settings, the tests are running by default with (antialiasing=1 + hinting=1), which (to my surprise) doesn't trigger the antialiasing issue with Ahem.

So I think that explains why the fix of disabling Antialiasing for Ahem for WebKitGTK has not caused new passes for wpt.fyi.

The good part is that this fix should fix the issue with Ahem for everybody, including in setups with hinting disabled. I verified that is that the case, by manually running the affected WPT tests before and after.

FTR, this seems to be also an issue affecting Chrome: When font hinting is disabled, I get failures on those tests (like css/css-text/text-align/text-align-start* and css/css-text/text-align/text-align-end*) with Chrome related to Ahem antialiasing. Not sure if report an issue about this ?

foolip commented 4 years ago

FTR, this seems to be also an issue affecting Chrome: When font hinting is disabled, I get failures on those tests (like css/css-text/text-align/text-align-start* and css/css-text/text-align/text-align-end*) with Chrome related to Ahem antialiasing. Not sure if report an issue about this ?

If you can't find an existing issue, then filing a new one would be good. It sounds like it may be an issue affecting many tests, assuming the tests you mentioned aren't doing anything unusual.

stephenmcgruer commented 4 years ago

So as far as I can tell from this discussion, we are now in a position where Safari is fixed but Edge is not. That is borne out by the wpt.fyi URLs in the original report now showing fails for just Edge.

web-platform-tests / wpt

Failures on Edge due to anti-aliasing of Ahem (formerly also Safari) #20058