Lint rule for using Ahem as a web font. #18877
Conversation
wpt-pr-bot
requested review from
BenjaminSchaaf,
birtles,
dbaron,
dirkschulze,
drott,
fantasai,
fred-wang,
frivoal,
gsnedders,
jgraham,
jugglinmike,
kojiishi and
litherum
wpt-pr-bot
requested review from
silviapfeiffer,
svgeesus,
svillar,
tabatkins,
zcorpan and
jgraham
and removed request for
r12a
|
Unfortunately this is going to be a bit more complicated as https://github.com/web-platform-tests/wpt/blob/master/css/vendor-imports/mozilla/mozilla-central-reftests/README says "THIS DIRECTORY IS NOT THE MASTER COPY! DO NOT EDIT TESTS HERE!" @dbaron @Ms2ger how should we go about making the Ahem changes to those tests? |
lint.whitelist
Outdated
|
|
||
| # Tests that are false positives for using Ahem as a system font | ||
| AHEM SYSTEM FONT: acid/acid3/test.html | ||
| AHEM SYSTEM FONT: preload/single-download-preload.html |
There was a problem hiding this comment.
This test doesn't even use Ahem, perhaps s/ahem/Foo/ or something to avoid the lint exception?
There was a problem hiding this comment.
Good point, done.
Update them in mozilla-central, and @dbaron will manually push the changes upstream at some point. |
include the /fonts/ahem.css stylesheet (which loads this font as a web font). This prevents tests from assuming that Ahem is installed as a system font. Also includes some unit tests. There are a few tests that are incorrectly flagged as violating this rule, these were added to lint.whitelist. A bunch of other tests were fixed to load Ahem as a web font.
LukeZielinski
force-pushed
the
ahem-lint-rule
branch
from
3c7cc26
to
2bca5e0
Compare
wpt-pr-bot
requested review from
snuggs and
yoavweiss
and removed request for
plinss and
rachelandrew
There was a problem hiding this comment.
Unfortunately this is going to be a bit more complicated as https://github.com/web-platform-tests/wpt/blob/master/css/vendor-imports/mozilla/mozilla-central-reftests/README says "THIS DIRECTORY IS NOT THE MASTER COPY! DO NOT EDIT TESTS HERE!"
how should we go about making the Ahem changes to those tests?Update them in mozilla-central, and @dbaron will manually push the changes upstream at some point.
Thanks, I reverted these. All of these tests did already load Ahem as a web font, just from their own local copies. I think they're fine just being left as is - they do the right thing, and I don't actually see /fonts/ in mozilla-central, so not sure updating them would work.
lint.whitelist
Outdated
|
|
||
| # Tests that are false positives for using Ahem as a system font | ||
| AHEM SYSTEM FONT: acid/acid3/test.html | ||
| AHEM SYSTEM FONT: preload/single-download-preload.html |
There was a problem hiding this comment.
Good point, done.
|
Oh, also, I'm keeping track of the failures introduced by this PR here: crbug.com/1001210 Looks like the failures from the checks here are all tracked. @foolip could you please force-merge this if you get a chance? |
| # The following are imported from mozilla-central and can't be modified in WPT. | ||
| # They do load Ahem as a web font, but they use their own copy which trips the | ||
| # lint rule. Basically false positives. | ||
| AHEM SYSTEM FONT: css/vendor-imports/mozilla/mozilla-central-reftests/text3/segment-break-transformation-rules-003.html |
There was a problem hiding this comment.
Since we don't control these tests and the lint isn't run where they are maintained, listing individual tests isn't too useful, and will probably just mean that this list has to be updated on each sync, without driving down the number of exceptions. I recommend a blanket css/vendor-imports/mozilla/mozilla-central-reftests/* here instead.
There was a problem hiding this comment.
I disagree with this. We should lint those tests like normal since they should be moved out of that directory and we don't want to pile up problems that have to be addressed during that move.
There was a problem hiding this comment.
That's fine with me, it's Mozilla that maintains this. I assumed the wpt lint wouldn't run when modifying these at the upstream location so that it'd just be a burden on @dbaron when syncing, but if that's fine, I'll send a revert of my simplification.
|
@LukeZielinski can you also file a Gecko bug for the regressions in https://wpt.fyi/results/?diff&filter=ADC&run_id=306760008&run_id=299020008? I've logged https://bugzilla.mozilla.org/show_bug.cgi?id=1579498 for the changes we see in firefox. |
|
Sent #18901. |
This squashes all the previous commits and syncs to HEAD to include the lint rule PR (web-platform-tests#18877)
|
@LukeZielinski can you please update the docs as well? we should have the lint rule documented pending #17189 landing, and we shouldn't just say that you can rely on Ahem being installed in various places. |
|
@gsnedders Yes, the docs changes were in a pending PR, but I carved them off into #18972 to expedite. |
This squashes all the previous commits and syncs to HEAD to include the lint rule PR (web-platform-tests#18877)
Add a lint rule to check for tests that use the Ahem font but do not
include the /fonts/ahem.css stylesheet (which loads this font as a web
font). This prevents tests from assuming that Ahem is installed as a
system font. Also includes some unit tests.
There are a few tests that are incorrectly flagged as violating this
rule, these were added to lint.whitelist.
A bunch of other tests were fixed to load Ahem as a web font.