913264 - add 'none' value to font-variant-ligatures

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[John Daggett (:jtd)]

Add support for the 'none' value to font-variant-ligatures.  This will also need to be added to the shorthand once the pref is dropped.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, add none value for font-variant-ligatures

Adds the 'none' value to font-variant-ligatures which explicitly disables all ligature features.

Spec reference:
http://dev.w3.org/csswg/css-fonts/#font-variant-ligatures-none-value

Comment 2

[John Daggett (:jtd)]

Attachment: patch, reftest for none value

Add a simple, explicit reftest for this.  (The main patch includes in valid/invalid combinations for font-variant-ligatures).

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 829939 [details] [diff] [review]
patch, reftest for none value

Review of attachment 829939 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/font-features/reftest.list
@@ +27,5 @@
>  # compare feature specified within @font-face to same feature in style rule
>  HTTP(..) == font-features-hlig-2.html font-features-hlig.html
>  HTTP(..) == font-features-hlig-4.html font-features-hlig.html
>  HTTP(..) != font-features-hlig-5.html font-features-hlig.html
> +HTTP(..) == font-features-ligatures-none.html font-features-noliga.html

Doesn't this need the font-features pref to be enabled? Otherwise it'll fail on release channels, I think.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 829938 [details] [diff] [review]
patch, add none value for font-variant-ligatures

property_database.js:
You should also have an invalid_values item with "none" as the
first value, but also a second value.

(Maybe the same for 
 layout/reftests/font-features/font-variant-features.js ?)

nsFont.cpp:
It would probably help to have comments for the three items in
ligDefaults that have extra code associated with them below, pointing
to that code.  Also maybe even make all 3 special cases have comments
of the form:
>+      // liga already disabled, need to disable dlig, hlig, calt, clig
like you do for the new one.

>+    if ((val & NS_FONT_VARIANT_LIGATURES_NONE) &&
>+        (val & ~NS_FONT_VARIANT_LIGATURES_NONE)) {

Feels a little safer to write this as either:
+    if ((val & NS_FONT_VARIANT_LIGATURES_NONE) &&
+        (val != NS_FONT_VARIANT_LIGATURES_NONE)) {
or:
+    if ((val & NS_FONT_VARIANT_LIGATURES_NONE) &&
+        (val & ~int32_t(NS_FONT_VARIANT_LIGATURES_NONE))) {
just in case the type of the constant doesn't (in the future) match
 the type of val.

r=dbaron with that

Comment 5

[John Daggett (:jtd)]

Attachment: patch, reftest for none value

Add pref for font features being enabled.

Comment 6

[John Daggett (:jtd)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a93c5ec2aea
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f8868df18c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a93c5ec2aea
https://hg.mozilla.org/mozilla-central/rev/12f8868df18c

Comment 8

[Jean-Yves Perrier [:teoli]]

Doc up-to-date:
https://developer.mozilla.org/en-US/docs/Web/CSS/font-variant-ligatures
https://developer.mozilla.org/en-US/Firefox/Releases/28