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

Remove font-variant @font-face descriptor from Fonts 4 #2531

Closed
litherum opened this issue Apr 11, 2018 · 18 comments
Closed

Remove font-variant @font-face descriptor from Fonts 4 #2531

litherum opened this issue Apr 11, 2018 · 18 comments
Assignees

Comments

@litherum
Copy link
Contributor

WebKit is the only implementation, and our implementation has serious architectural-level issues. Given we are the only implementation, we should remove this descriptor from the spec (and remove the busted support from WebKit)

@faceless2
Copy link

Although our implementation isn't ready for public release yet, we do have full support for this, and we have recently added some test-cases to reflect this at web-platform-tests/wpt#9397, with accompanying discussion at #2265.

@css-meeting-bot
Copy link
Member

The Working Group just discussed font-variant @font-face descriptor.

The full IRC log of that discussion <dael> Topic: font-variant @font-face descriptor
<dael> github: https://github.com//issues/2531
<dael> myles: Is the commentor in the room?
<dael> ChrisL: It's Mike Gramford (sp?) and I don't know whose impl it refers to. They did contribute some tests so there's a test and that's where it was discovered the font-varient descriptor was under impl.
<dael> myles: It's mostly in the issue. Similar to the @font-face descriptors there's one that lets you turn on/off features. Nothing to do with font selection. I thought webkit was only impl, but there's some other behind a flag.
<ChrisL> test case for font-variant descriptor https://www.w3.org/People/chris/fwf/font-variant-descriptor-01.html
<dael> myles: Webkit is only shipping and our impl has archetectural problems and there's bifircation of text code paths and we make that before we inspect @font-face. we need to know if there are features to turn on before we decide. If you're on the wrong path it migh or might not work.
<dael> myles: I'd like to remove it.
<dael> astearns: Does anyone claim Mike?
<dael> TabAtkins: I think it's a pdf renderer
<dael> astearns: We could defer.
<dael> ChrisL: I posted this on www-style. This is one of the non-passing tests. Do we defer? Do we delete? It's holding us from rec
<dael> astearns: this ability seems useful.
<dael> ChrisL: I thought so too but myles talked about how it can conflict with cascade and if you put this in @font-face you have fixed positions
<dael> fantasai: property overrides descriptor so no cascade problem.
<dael> myles: physiologically features should be applied to everything in the cascade.
<dael> fantasai: There are fonts out there that are always small caps
<dael> myles: But turning it on on small case will be fine.
<ChrisL> s/physiologically/philosophically/
<dael> fantasai: But there are other features where if I have this font I want historical ligatures but not on this one because it's ugly. It's abigger issue for stylistic alternates.
<dael> astearns: We have a spec we want to push and a feature we don't ahve 2 impl I think we should defer and not remove until we talk.
<dael> astearns: Objections to defer font varient descriptor to L4?
<dael> resolved: defer font variant descriptor to L4

@astearns
Copy link
Member

@faceless2 in the minutes above, “until we talk” means until we talk to you. We aren’t yet removing this, just postponing deciding whether it’s specified to the next module level.

@faceless2
Copy link

Thank you for your consideration! But no real need. All I wanted to add was that there was another implementation out there, as I'm aware that that can make a difference to the decision process. Yes, ours is (or will be) a PDF renderer, along the lines of Prince or PDF Reactor.

We do have a working implementation of this part of the spec as it stands, although (IMHO) the spec could be modified to better serve the purpose it was intended for - some of this was covered in the issue I referenced and I know everyone is aware of those.

To sum up: it doesn't bother us in the slightest deferring this until L4, please do.

@svgeesus
Copy link
Contributor

I made an equivalence test for font-variant descriptor vs. property
https://www.w3.org/People/chris/fwf/font-variant-descriptor-01.html
https://www.w3.org/People/chris/fwf/font-variant-descriptor-01-ref.html

Now merged into WPT
web-platform-tests/wpt#10415
and in CSS WG test harness
https://test.csswg.org/harness/test/css-fonts-3_dev/single/font-variant-descriptor-01/

Chrome (Canary) Firefox (Nightly) fail as they don't implement the descriptor. Safari (Tech Preview) passes the descriptor subtests but fails one of the property subtests. Edge (17 preview) fails.

svgeesus added a commit that referenced this issue Apr 12, 2018
fergald pushed a commit to fergald/csswg-drafts that referenced this issue May 7, 2018
@svgeesus svgeesus changed the title Remove font-variant @font-face descriptor Remove font-variant @font-face descriptor from Fonts 3 May 14, 2018
svgeesus added a commit to web-platform-tests/wpt that referenced this issue May 16, 2018
@svgeesus
Copy link
Contributor

svgeesus commented May 16, 2018

@litherum could you please review the trivial test changes which just re-assigns these two tests to Fonts 4. I checked that all the links correctly resolve in Fonts 4.

font-variant-05 is one of only two non-passing tests for Fonts 3 at this point.

plehegar pushed a commit to web-platform-tests/wpt that referenced this issue May 22, 2018
* font-variant descriptor was moved to Fonts 4 w3c/csswg-drafts#2531

* font-variant descriptor was moved to Fonts 4
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 25, 2018
…ts 4 , a=testonly

Automatic update from web-platform-testsfont-variant descriptor was moved to Fonts 4  (#11035)

* font-variant descriptor was moved to Fonts 4 w3c/csswg-drafts#2531

* font-variant descriptor was moved to Fonts 4

--

wpt-commits: 232137f0fdacdeed99a7df5dd229d23020b0bccc
wpt-pr: 11035
@svgeesus
Copy link
Contributor

This test got moved to the Fonts 4 testsuite. There are still zero passing implementations

@svgeesus
Copy link
Contributor

@litherum besides the "remove from Fonts 3" component (already done) I also get the impression this issue is "should it be there at all" so I am leaving it open. Can you confirm?

@svgeesus
Copy link
Contributor

Still no passing implementations for font-variant descriptor. Do we want to keep this? Is there implementer interest?

@svgeesus svgeesus self-assigned this Aug 29, 2019
@drott
Copy link
Collaborator

drott commented Sep 16, 2019

At this point we don't implement other descriptors in @font-face, such as font-feature-settings and font-variation-settings - @litherum, do you suggest to keep those and only remove font-variant?

@litherum
Copy link
Contributor Author

litherum commented Sep 17, 2019

Let's remove them all, if possible!!! (For the same reason I listed in the top post here)

We may have compat problems, because we've been shipping the other ones for a while. I'm willing to try it, though! I have received bug reports about the font-feature-settings descriptor, so I know it's used at least somewhat.

@svgeesus
Copy link
Contributor

svgeesus commented Sep 17, 2019

@drott wrote

At this point we don't implement other descriptors in @font-face, such as font-feature-settings and font-variation-settings - @litherum, do you suggest to keep those and only remove font-variant?

I prefer to discuss those in a separate issue.

Edit: this issue

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Remove font-variant @font-face descriptor from Fonts 3, and agreed to the following:

  • RESOLVED: Remove font-variant @font-face descriptor from Fonts 3
The full IRC log of that discussion <mstange> Topic: Remove font-variant @font-face descriptor from Fonts 3
<mstange> github: https://github.com//issues/2531
<mstange> myles: We have two text paths. In WebKit. The distinction which path to go to has to happen before font fallback.
<mstange> ... Sometimes we detect something too late and can't go back and re-do everything, so we just do our best.
<mstange> ... If this is not necessary for the web platform, let's remove it.
<mstange> fantasai: It does look like there is an implementation (faceless2 says it in the issue)
<mstange> ... If the feature doesn't have a problem, we should leave it. We do not need implementations to move this spec at the moment.
<mstange> myles: When is the deadline? It's been years.
<mstange> Rossen_: This is a non-verifiable implementation. How do we evaluate it?
<Rossen_> q?
<mstange> drott: We would support removing it because the semantics are unclear in some cases.
<mstange> heycam: We would also be fine with removing it. We haven't heard much demand from authors.
<mstange> ... In principle it seems like a useful thing, but we would be fine with removing it.
<mstange> Rossen_: Any objections to removing it?
<mstange> Rossen_: No objections.
<mstange> RESOLVED: Remove font-variant @font-face descriptor from Fonts 3

@drott
Copy link
Collaborator

drott commented Sep 17, 2019

I prefer to discuss those in a separate issue.

Makes sense, let's track those separately.

@heycam
Copy link
Contributor

heycam commented Sep 17, 2019

This means we should also remove variant from the FontFace interface in the Font Loading API spec.

heycam added a commit that referenced this issue Sep 17, 2019
This implements the Font Loading API part of #2531.
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…ts 4 , a=testonly

Automatic update from web-platform-testsfont-variant descriptor was moved to Fonts 4  (#11035)

* font-variant descriptor was moved to Fonts 4 w3c/csswg-drafts#2531

* font-variant descriptor was moved to Fonts 4

--

wpt-commits: 232137f0fdacdeed99a7df5dd229d23020b0bccc
wpt-pr: 11035

UltraBlame original commit: 9efa69e38c8c79e251e51fa84d909e6980437d11
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…ts 4 , a=testonly

Automatic update from web-platform-testsfont-variant descriptor was moved to Fonts 4  (#11035)

* font-variant descriptor was moved to Fonts 4 w3c/csswg-drafts#2531

* font-variant descriptor was moved to Fonts 4

--

wpt-commits: 232137f0fdacdeed99a7df5dd229d23020b0bccc
wpt-pr: 11035

UltraBlame original commit: 9efa69e38c8c79e251e51fa84d909e6980437d11
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…ts 4 , a=testonly

Automatic update from web-platform-testsfont-variant descriptor was moved to Fonts 4  (#11035)

* font-variant descriptor was moved to Fonts 4 w3c/csswg-drafts#2531

* font-variant descriptor was moved to Fonts 4

--

wpt-commits: 232137f0fdacdeed99a7df5dd229d23020b0bccc
wpt-pr: 11035

UltraBlame original commit: 9efa69e38c8c79e251e51fa84d909e6980437d11
@litherum
Copy link
Contributor Author

@heycam only edited the font loading spec, not the fonts spec.

@litherum
Copy link
Contributor Author

Oh, I see, @svgeesus did it in d3db869 in fonts-4, and it was never in fonts-3 (meaning this issue is incorrectly titled).

@litherum litherum changed the title Remove font-variant @font-face descriptor from Fonts 3 Remove font-variant @font-face descriptor from Fonts 4 Oct 18, 2019
@litherum
Copy link
Contributor Author

Wow, it looks like I accidentally removed the relevant section from the "Feature and Variation Precedence" section in 0170cbc. Oops! Good thing we eventually deleted it anyway!

ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=203179

Reviewed by Simon Fraser.

Source/WebCore:

As per w3c/csswg-drafts#2531

Deleted relevant tests.

* css/CSSFontFace.cpp:
(WebCore::CSSFontFace::font):
(WebCore::CSSFontFace::setVariantLigatures): Deleted.
(WebCore::CSSFontFace::setVariantPosition): Deleted.
(WebCore::CSSFontFace::setVariantCaps): Deleted.
(WebCore::CSSFontFace::setVariantNumeric): Deleted.
(WebCore::CSSFontFace::setVariantAlternates): Deleted.
(WebCore::CSSFontFace::setVariantEastAsian): Deleted.
* css/CSSFontFace.h:
* css/CSSFontFaceSource.cpp:
(WebCore::CSSFontFaceSource::load):
(WebCore::CSSFontFaceSource::font):
* css/CSSFontFaceSource.h:
* css/CSSFontSelector.cpp:
(WebCore::CSSFontSelector::addFontFaceRule):
* css/FontFace.cpp:
(WebCore::FontFace::create):
(WebCore::FontFace::setVariant): Deleted.
(WebCore::FontFace::variant const): Deleted.
* css/FontFace.h:
* css/FontFace.idl:
* loader/cache/CachedFont.cpp:
(WebCore::CachedFont::createFont):
(WebCore::CachedFont::platformDataFromCustomData):
* loader/cache/CachedFont.h:
* loader/cache/CachedSVGFont.cpp:
(WebCore::CachedSVGFont::createFont):
(WebCore::CachedSVGFont::platformDataFromCustomData):
* loader/cache/CachedSVGFont.h:
* platform/graphics/FontCache.cpp:
(WebCore::FontPlatformDataCacheKey::FontPlatformDataCacheKey):
(WebCore::FontPlatformDataCacheKey::operator== const):
(WebCore::FontPlatformDataCacheKeyHash::hash):
(WebCore::FontCache::getCachedFontPlatformData):
(WebCore::FontCache::fontForFamily):
* platform/graphics/FontCache.h:
(WebCore::FontCache::fontForFamily):
(WebCore::FontCache::getCachedFontPlatformData):
(WebCore::FontCache::createFontPlatformDataForTesting):
* platform/graphics/cocoa/FontCacheCoreText.cpp:
(WebCore::preparePlatformFont):
(WebCore::fontWithFamily):
(WebCore::FontCache::createFontPlatformData):
(WebCore::FontCache::systemFallbackForCharacters):
* platform/graphics/cocoa/FontCacheCoreText.h:
* platform/graphics/cocoa/FontFamilySpecificationCoreText.cpp:
(WebCore::FontFamilySpecificationCoreText::fontRanges const):
* platform/graphics/mac/FontCustomPlatformData.cpp:
(WebCore::FontCustomPlatformData::fontPlatformData):
* platform/graphics/mac/FontCustomPlatformData.h:

LayoutTests:

Delete tests for the removed feature.

* css3/font-variant-font-face-all-expected.html: Deleted.
* css3/font-variant-font-face-all.html: Deleted.
* css3/font-variant-font-face-override-expected.html:
* css3/font-variant-font-face-override.html:
* fast/text/font-face-empty-string-expected.txt:
* fast/text/font-face-empty-string.html:
* fast/text/font-face-javascript-expected.txt:
* fast/text/font-face-javascript.html:


Canonical link: https://commits.webkit.org/217740@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@252760 268f45cc-cd09-0410-ab3c-d52691b4dbfc
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