Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
smfr opened this issue Nov 2, 2019 · 20 comments
Open

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

smfr opened this issue Nov 2, 2019 · 20 comments

Comments

@smfr
Copy link
Contributor

smfr commented Nov 2, 2019

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
Copy link
Contributor Author

smfr commented Nov 2, 2019

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

@smfr
Copy link
Contributor Author

smfr commented Nov 2, 2019

See also issue #5498

@smfr
Copy link
Contributor Author

smfr commented Nov 2, 2019

@litherum does #5498 (comment) apply to Cocoa platforms?

@foolip
Copy link
Member

foolip commented Nov 3, 2019

#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
Copy link
Contributor

jgraham commented Nov 3, 2019

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
Copy link
Member

@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
Copy link
Contributor

clopez commented Nov 13, 2019

Related: #13010

@clopez
Copy link
Contributor

clopez commented Nov 13, 2019

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:

  1. run ref-test as usual
  2. if test fails and test uses Ahem and browser is safari:
  3. 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
Copy link
Contributor

clopez commented Nov 13, 2019

* 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
Copy link
Contributor

jgraham commented Nov 13, 2019

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
Copy link
Member

foolip commented Nov 14, 2019

@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
Copy link
Contributor Author

smfr commented Nov 14, 2019

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

@litherum
Copy link
Contributor

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

@foolip
Copy link
Member

foolip commented Nov 15, 2019

That's great, thanks @smfr!

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

@gsnedders
Copy link
Member

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
Copy link
Contributor

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

@foolip
Copy link
Member

foolip commented Nov 22, 2019

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

@clopez
Copy link
Contributor

clopez commented Jan 17, 2020

@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
Copy link
Member

foolip commented Jan 17, 2020

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
Copy link
Contributor

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.

@gsnedders gsnedders changed the title Many tests fail on Safari and Edge because of font antialiasing differences Failures on Edge due to anti-aliasing of Ahem (formally also Safari) Aug 11, 2020
@gsnedders gsnedders changed the title Failures on Edge due to anti-aliasing of Ahem (formally also Safari) Failures on Edge due to anti-aliasing of Ahem (formerly also Safari) Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants