1057680 - Add support for specifying 'font-stretch' in the CSS 'font' shorthand

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Syoichi Tsuyuhara]

Attachment: Test Case for CSS Fonts Module Level 3's font shorthand property syntax parsing(about <font-stretch>)

CSS 2.1: [ [ <'font-style'> || <'font-variant'> || <'font-weight'> ]? <'font-size'> [ / <'line-height'> ]? <'font-family'> ] | caption | icon | menu | message-box | small-caption | status-bar
http://www.w3.org/TR/CSS21/fonts.html#font-shorthand

CSS 3: [ [ <‘font-style’> || <font-variant-css21> || <‘font-weight’> || <‘font-stretch’ ]? <‘font-size’> [ / <‘line-height’> ]? <‘font-family’> ] | caption | icon | menu | message-box | small-caption | status-bar
http://dev.w3.org/csswg/css-fonts-3/#font-prop

Now Gecko lacks "<font-weight>" support.
But, it seems that Blink supports new syntax parsing(I don't know about rendering).

If this feature is implemented, this MDN page should be updated.
https://developer.mozilla.org/en-US/docs/Web/CSS/font

Comment 1

[David Baron :dbaron:]

(In reply to Syoichi Tsuyuhara from comment #0)
> Now Gecko lacks "<font-weight>" support.

Do you mean <font-stretch>?

Comment 2

[Syoichi Tsuyuhara]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #1)
> (In reply to Syoichi Tsuyuhara from comment #0)
> > Now Gecko lacks "<font-weight>" support.
> 
> Do you mean <font-stretch>?

Ah, yes. It's correct. I'm sorry.

Comment 3

[Syoichi Tsuyuhara]

Comment on attachment 8477815 [details]
Test Case for CSS Fonts Module Level 3's font shorthand property syntax parsing(about <font-stretch>)

Fix Test Case's Description

Comment 4

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b56bb92baa7c

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - Add a font-stretch keyword to the valid values for the font shorthand in property_database.js

Here's a patch to add a font-stretch keyword to the shorthand in property_database. I've checked that this causes several mochitests to fail, as expected (so it will need to be folded together with the actual code patch for landing).

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Add support for font-stretch values in the font shorthand

And this should make the font shorthand accept the stretch keywords. If unit tests are happy with this, I'll also add a reftest to confirm that it has the expected effect on rendering.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - Add support for font-stretch values in the font shorthand

Oops - fixed copy/paste error in the patch.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: pt 2.1 - Fix up existing tests that depend on the shorthand not handling font-stretch values

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Reftest for use of font-stretch values in the font shorthand

Comment 10

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43113ee0c7a3

Comment 11

[John Daggett (:jtd)]

Comment on attachment 8656009 [details] [diff] [review]
pt 2 - Add support for font-stretch values in the font shorthand

r+ with minor cleanup below

> +  // Get optional font-style, font-variant, font-weight, font-stretch
> +  // (in any order)
> +  static const nsCSSProperty fontIDs[] = {
> +    eCSSProperty_font_style,
> +    eCSSProperty_font_variant_caps,
> +    eCSSProperty_font_weight,
> +    eCSSProperty_font_stretch
> +  };

> @@ -12249,16 +12251,20 @@ CSSParserImpl::ParseFont()
>    if ((found & 4) == 0) {
>      // Provide default font-weight
>      values[2].SetIntValue(NS_FONT_WEIGHT_NORMAL, eCSSUnit_Enumerated);
>    }
> +  if ((found & 8) == 0) {
> +    // Provide default font-stretch
> +    values[3].SetIntValue(NS_FONT_STRETCH_NORMAL, eCSSUnit_Enumerated);
> +  }
 
I think it would be a good idea to tidy this code up a bit by using explicit constants (e.g. STYLE_INDEX, WEIGHT_INDEX) and a comment to indicate that the indices are dependent upon the ordering of fontIDs[].

Comment 12

[John Daggett (:jtd)]

Comment on attachment 8656045 [details] [diff] [review]
pt 3 - Reftest for use of font-stretch values in the font shorthand

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

r+ with the fix below

::: layout/reftests/font-matching/reftest.list
@@ +71,5 @@
>  HTTP(..) == stretchmapping-137.html stretchmapping-137-ref.html
>  
>  # test for font-stretch using @font-face
>  skip-if(B2G||Mulet) skip-if(Android&&AndroidVersion>15) HTTP(..) == font-stretch-1.html font-stretch-1-ref.html # bugs 773482, 927602 # Initial mulet triage: parity with B2G/B2G Desktop
> +skip-if(B2G||Mulet) skip-if(Android&&AndroidVersion>15) HTTP(..) == font-shorthand-stretch-1.html font-stretch-1-ref.html

Copy the bug numbers from the line above

Comment 13

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f54b7bf6b3dbb8fb5fabe7821b21b7ca27fd23
Bug 1057680 - Add support for font-stretch values in the font shorthand. r=jdaggett

https://hg.mozilla.org/integration/mozilla-inbound/rev/8408b26adf818039e669e807f20d6c2e5d7c054a
Bug 1057680 - Reftest for use of font-stretch values in the font shorthand. r=jdaggett

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a0f54b7bf6b3
https://hg.mozilla.org/mozilla-central/rev/8408b26adf81

Comment 15

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/Firefox/Releases/43#CSS
and
https://developer.mozilla.org/en-US/docs/Web/CSS/font

Comment 16

[Paul Silaghi, QA [:pauly]]

For testing: the testcase should return TRUE on all lines.