Closed Bug 913264 Opened 7 years ago Closed 7 years ago

add 'none' value to font-variant-ligatures

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jtd, Assigned: jtd)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])

Attachments

(2 files, 1 obsolete file)

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.
Blocks: css3test
Blocks: css-fonts-3
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)
Attached 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+
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
Closed: 7 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.