search

web-platform-tests / rfcs

web-platform-tests RFCs
83 stars 67 forks source link

Create RFC to load Ahem as a webfont #22

Closed LukeZielinski closed 5 years ago

LukeZielinski commented 5 years ago

Rendered

jugglinmike commented 5 years ago

@frivoal may have helpful perspective on this.

foolip commented 5 years ago

@LukeZielinski have you come across any tests that use Ahem not for its known glyph metrics, but to test a system font as opposed to a loaded font? If any such tests exist, I wonder if as an alternative to requiring Ahem to be installed could require some font to be installed as a system font, and have a way for the font family name to be provided to tests as a variable or through the wptserve template system?

foolip commented 5 years ago

@drott do you have thoughts about this?

foolip commented 5 years ago

Copying over @drott's comment from https://github.com/web-platform-tests/wpt/pull/16951#issuecomment-495191812:

What's the intention of the change? To make the tests more robust due to issue with Ahem not being available from the system?

I don't have objections to this change in particular, but I think it'd be beneficial to improve our system font testing story, as outlined in https://github.com/web-platform-tests/wpt/issues/13203#issuecomment-463979709 or it would generally help if control over system fonts available to WPT became more reliable, or just .. available.

What triggered this proposal was @LukeZielinski running into trouble getting system fonts installed on Chromium CQ. Given your comments in https://github.com/web-platform-tests/wpt/issues/13203#issuecomment-463979709, perhaps there is a way to do this after all?

drott commented 5 years ago

@drott do you have thoughts about this?

No additional thoughts other than the ones you quoted. I think it'd be beneficial if we have more robust WPT harness mechanisms to control what system fonts are installed on the system, or at least available to the rendering engine in a sideloaded way that is equivalent in behavior to system fonts. We have sideloading mechanisms in Chromium for Windows, Android and Linux that do not require a "real" installation on the system.

LukeZielinski commented 5 years ago

@foolip I have not come across any tests that cares about a system font being installed. They all seemed to work fine when switching to a webfont.

@drott The side-loading mechanism you're talking about is only for content shell, right? For the use case here we'd be interested in running WPT against full Chrome inside Chromium CI, which is why this conversation got started.

frivoal commented 5 years ago

This seems like a good idea.

Certainly, going forward, I see no reason not to require usage of Ahem as a webfont, that's a lot more reliable.

For past tests, although the change does seem safe in principle, I haven't been able to review the entire diff to check that it was true in all cases, and I doubt anybody will find it practical to do manually.

If we could do a sanity check, verifying that the before/after state looks the same it all tests, that'd be great. Quite possibly there are a few tests in there that are manual, and might not be suitable for automated before/after verification, but if we could just check the before/after of all ref tests (and all refs), that would leave us with a much smaller set of things to review manually.

To make sure we don't create more tests with Ahem not used as a webfont, we should also:

As a side note, since most CSS tests typically don't need a server side component, it's quite common while developing them to run the directly from the file system rather than from wpt serve, and the proposed approach is nice in that it doesn't break that. Ahem.css is loaded via an absolute url which will fail when using the file:// protocol, but since the font-family name is still Ahem, it will load it (as usual) from the system fonts for people who have it.

LukeZielinski commented 5 years ago

As far as the mechanics of actually submitting this, I plan to break up the massive CL into two parts. The first will be the reviewable things like Ahem.css, docs change, lint rule, and a handful of representative test updates (there are 3 distinct flavours). A follow-up change will update the remaining ~2k reftests that just need to link to Ahem.css.

For correctness, I've run this change through Chromium CQ and it passed (this link may time out since the CL is so big, ymmv). So there's at least some signal that this doesn't break anything new. For the PR, we're going to get failing checks due to already-failing tests that are being updated. I think I should be able to upload a run manually to wpt.fyi for a side-by-side comparison though.

gsnedders commented 5 years ago

create a lint that complains if you've used font-family: Ahem and not included Ahem.css (or something along these lines)

I guess we could, though probably with something even more naïve at first attempt (given font is used in plenty, and I don't really want to add a CSS parser to the lint… Probably allowing either ahem.css or Ahem.ttf (I don't think we need to force people to use ahem.css, even if it's probably the easiest way to add this to all of the tests).

jgraham commented 5 years ago

AIUI this matches how gecko-internal reftests already work, so I don't think we have any issues with this going forward. I do think there needs to be a lint (even a relatively basic one) as part of the initial patch, to ensure we don't regress this straight away. It should probably also make --install-fonts a no-op in wptrunner to ensure that tests not following the new style fail in CI. The easiest way to do this is simply to remove Ahem from the list of fonts that we install. We could also delete the code if we want, but the command line option should still be supported so we don't break scripts (it can print a warning about how it does nothing). We may end up wanting that option again if we require system fonts for testing system-font-only codepaths, but I think that problem is not well understood at this time.

gsnedders commented 5 years ago

@jgraham I expect in reality we want to be able to install fonts on a per-test basis, given I think this is closer to what content_shell already does, and there are various advantages to this (and I think we have tests that have contrary requirements here: some that need to install a font called "sans-serif", and some that need to uninstall a font called "sans-serif").

jgraham commented 5 years ago

Sure, but the actual code to install fonts doesn't change in that case (although the chance of it working on various platforms is greatly diminished). It just means we need to arrange for the fonts to be part of the environment in the same way that gecko prefs are currently.

LukeZielinski commented 5 years ago

To summarize the state of this as I see it so far:

There doesn't seem to be any opposition to this change in principal. The following specific tweaks were suggested, and I'm happy to make them:

I'll break this up into 2 PRs. The first will include the above changes with a handful of representative test updates. The second will include the ~2k remaining test updates.

In addition there are some legitimate concerns about WPT's support for testing using specific system fonts. Those concerns are more general than this Ahem discussion and we should consider those as a separate feature request.

Can I consider this approved then?

jgraham commented 5 years ago

I think the formal process may require waiting another couple of days, but at this point it seems like there's consensus, so I would suggest you proceed as if this is approved.

emilio commented 5 years ago

So this was discussed before in www-style and it didn't seem to have consensus. This point by @MatsPalmgren is still legit, fwiw: https://lists.w3.org/Archives/Public/www-style/2017Jan/0053.html

emilio commented 5 years ago

Though I guess it's ok per the rest of the thread... Oh well. I think I did prefer having ahem as a local font, but...

zcorpan commented 5 years ago

If the tests use local() and inline the CSS, it can use the system font without waiting for an external stylesheet, I believe. Would that work, @emilio @MatsPalmgren ? (If yes, it would allow running tests with and without system Ahem, too.)

@font-face { font-family: 'Ahem'; src: local('Ahem') url('/fonts/Ahem.ttf'); }
gsnedders commented 5 years ago

Also note that some systems, iOS most obviously, don't allow installing system fonts, and some browsers (notably Safari) don't allow websites to use anything except preinstalled fonts (to remove user-installed fonts as an additional fingerprinting metric).

jgraham commented 5 years ago

If the tests use local() and inline the CSS, it can use the system font without waiting for an external stylesheet, I believe.

I don't like the idea of adding an implcit branch into the tests depending on the system configuration.

MatsPalmgren commented 5 years ago

@zcorpan Yeah, that seems like a good compromise to me.

zcorpan commented 5 years ago

OK. But @jgraham doesn't like the idea. Can we make it an explicit branch somehow? I don't know how worthwhile it is to test locally installed Ahem in wpt.

If what Safari does is something that other browsers will follow, then the test would be "make sure that a locally user-installed font does not work", which could be a single test.

MatsPalmgren commented 5 years ago

The reason I want to include local(Ahem) is that we've had multiple rendering bugs in Gecko which only occurred when using a system font. So with the current setup for Ahem in WPT those bugs would not be caught by these tests.

I really don't see what harm including local(Ahem) does. If it's not installed we'll just load the url() one instead. (Obviously, anyone that wants to run WPT with a local Ahem MUST make sure it's identical to the one in WPT.)

foolip commented 5 years ago

Is it important for those tests that the font is Ahem, or could it be tested using any system font? If any font would do, we could perhaps arrange a mechanism to reference some system font, but not have it be the same one cross-platform.

foolip commented 5 years ago

(Note that we can't make local(Ahem) work on macOS Mojave and later.)

MatsPalmgren commented 5 years ago

Is it important for those tests that the font is Ahem, or could it be tested using any system font?

The bugs I'm referring to were extreme edge cases that weren't covered by any existing tests. You can't plan ahead and write a test for unknown future bugs in edge cases like that. The only way to catch them is to maximize the test coverage for that particular code path. This is what including local(Ahem) is about - to maximize the number of tests run to increase the chance of finding such a bug. Yes, they would likely also be found with any other system font. But excluding all tests that use the Ahem font significantly reduces the chance of detecting it, which is what I'm worried about. (Minimizing the use of Ahem in WPT doesn't seem like a good solution, since it's so convenient to use for particular types of tests and we don't want to make it harder to write reliable tests.)

(Note that we can't make local(Ahem) work on macOS Mojave and later.)

Running WPTs on any system that doesn't have Ahem installed will simply use the url() font instead. So, I see no harm in adding it.

foolip commented 5 years ago

If we're talking about incidental coverage by lots of tests that aren't trying to target that bug, then the default configuration can only cover either system fonts or webfonts. That is, unless we run a bunch of tests twice.

What is the concrete thing that we should do to get better coverage of system fonts?

foolip commented 5 years ago

I guess the concrete suggestion is https://github.com/web-platform-tests/rfcs/pull/22#issuecomment-500964750. But even with that we have to either install Ahem or not by default, and not have coverage for the other option. Would anyone run the tests twice to cover both?

jgraham commented 5 years ago

We (i.e. gecko CI) could theoretically spread it over platforms in some way e.g. having something different between opt and debug or Linux and Windows. But I don't see a way that isn't degenerate with some other interesting difference. We could theoretically run both configurations on one platform e.g. on linux64, but that would at least require some justification for the extra CI load.

foolip commented 5 years ago

I guess we change ahem.css when there's a concrete plan to install Ahem on some CI setups but not others. @MatsPalmgren is that something you'd like to pursue, or convince someone else to work on?

MatsPalmgren commented 5 years ago

Would anyone run the tests twice to cover both?

That's a bad assumption. As @jgraham hinted, there's a whole spectrum of options for running these tests. For Gecko, the best option by far is to always use a system-installed Ahem. (There are plenty of other WPTs that covers testing the @font-face feature specifically.) So, please add local(Ahem).

jgraham commented 5 years ago

FWIW given the difficulty we've had with system fonts on Mac I'd be pretty keen to use the @font-face loaded copy at least there. But sure if you think there's enough advantage from the current setup that we should keep it where possible, I think we should add local() to the stylesheet and ensure that --install-fonts keeps working.

foolip commented 5 years ago

@LukeZielinski can you add local(Ahem) to the stylesheet?

@jgraham do you think we should keep installing fonts in our Taskcluster runs?

jgraham commented 5 years ago

I think if the feature is supposed to keep working, at least in some browsers, we should have some kind of CI coverage for it. I don't know what's better in terms of getting good results in wpy.fyi for master runs.

foolip commented 5 years ago

Probably local(Ahem) is a bit faster? Sent https://github.com/web-platform-tests/wpt/pull/19371 while wondering about just that.

How about running infrastructure/ tests without Ahem installed to make sure that works, but run with it installed for the full runs?

MatsPalmgren commented 5 years ago

How about running infrastructure/ tests without Ahem installed to make sure that works, but run with it installed for the full runs?

Having Ahem both uninstalled/installed in the same run sounds unnecessarily complicated to me. The @font-face feature itself already has good coverage, in css/css-fonts/* and elsewhere. There's nothing special about Ahem in this respect. Also, there already are tests that load url(/fonts/Ahem.ttf) but under a different family name than Ahem, so testing that url works is already covered. I guess you could another one under infrastructure/ if you want, but then just skip including ahem.css and use something like:

      @font-face {
          font-family: TestThatAhemURLWorks;
          src: url(/fonts/Ahem.ttf);
      }
foolip commented 5 years ago

Those are different jobs, so it's just a matter of removing the step to install fonts from one of them.