add 'none' value to font-variant-ligatures

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=CSS])

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
Blocks: css3test
Assignee

Updated

6 years ago
Blocks: css-fonts-3
Assignee

Comment 1

6 years ago
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
Assignee: nobody → jdaggett
Attachment #829938 - Flags: review?(dbaron)
Assignee

Comment 2

6 years ago
Posted patch patch, reftest for none value (obsolete) — Splinter Review
Add a simple, explicit reftest for this.  (The main patch includes in valid/invalid combinations for font-variant-ligatures).
Attachment #829939 - Flags: review?(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 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
Attachment #829938 - Flags: review?(dbaron) → review+
Assignee

Comment 5

6 years ago
Add pref for font features being enabled.
Attachment #829939 - Attachment is obsolete: true
Attachment #829939 - Flags: review?(jfkthame)
Attachment #8333606 - Flags: review?(jfkthame)
Attachment #8333606 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/1a93c5ec2aea
https://hg.mozilla.org/mozilla-central/rev/12f8868df18c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [DocArea=CSS]
You need to log in before you can comment on or make changes to this bug.